Changesets feature complete

[John Arbash Meinel]

Aaron Bentley wrote:
> John A Meinel wrote:
> |>Suggestions are definitely welcome.  Unless they involve -p1 patches :-P
> |
> |
> | Well, I think we could provide the flag for "bzr cset --prefix" just
> | because we would want to support mailing lists that one -p1 patches. But
> | just as with diff, it shouldn't be the default.
> 
> Yes, I was kidding about that.  I have no problem with providing that
> option, and it shouldn't actually make a difference for applying
> changesets.
> 
> | Some other comments:
> |
> | Do you support verifying revisions by their testament before they are
> | added to the repository?
> 
> Yes, just barely in time.  Because it requires an inventory, that
> verification is done when you call ChangesetReader.revision_tree

Sure. Since XML isn't sufficient for generating sha hashes (which is why
we have Testaments), that is really all that you can do.

> 
> | I'm not sure exactly how to do this, but since
> | we can't really strip stuff out after the fact, we really should do the
> | verification step before we add anything.
> 
> This will ensure that only valid revisions are added, but it it may
> install some valid revisions, then encounter an invalid revision and stop.

Yeah. When I wrote ChangesetReader, I wanted it to be a little bit lazy
about parsing. So it would perform one pass and read everything, and get
it ready for parsing. Verify the sha hashes that it could. Then it would
actually parse the patches, and verify things again.
But now that inventory and revision hashes aren't really valid (the only
truly valid hashes are the text hashes, and then testament hash).

> 
> Validating the entire changeset before installing any of it would
> require creating ChangesetTrees from other ChangesetTrees, created from
> other ChangesetTrees.  While it's a fun exercise in recursion, I think
> it would probably be slow.

No, I'm fine with just installing what you can. As long as we don't
install bogus stuff, we can always install a little bit extra.

> 
> | I would default 'executable' to no, since that is the common case. Then
> | you don't need to add that information to every line 'added' line.
> | This is a simple patch:
> | === modified file 'bzrlib/changeset/read_changeset.py'
> | --- bzrlib/changeset/read_changeset.py
> |
> | +++ bzrlib/changeset/read_changeset.py
> | @@ -858,7 +858,7 @@
> |          if path in self._executable:
> |              return self._executable[path]
> |          else:
> | -            return self.base_tree.inventory[file_id].executable
> | +            return False
> 
> The semantic in ChangesetTree is that if a value is not supplied, it is
> unchanged from the base revision.  This patch changes it so that, for
> this item alone, the semantic is that if it's not supplied, it's false.
> ~ That means that we must supply it for every case where it is true, that
> is, we must note the executable bit for all unmodified executable files.
> ~ I think this is the wrong place to do this.

That is what the semantic used to be. You changed it so that it always
set 'executable'. (I had a whole lot of lines with // executable:no). I
prefer the 'set it only if it changes', but second best is 'set it only
if true'.


> 
> I think a better approach is to do handle this default in processing the
> the action lines.  So when processing adds, modifies, and renames, if
> the execute bit is not supplied, we set it to False in the
> ChangesetTree, and if it is supplied, we set it according to the
> supplied value.

That is what my patch did.

> 
> | Your BRANCH.TODO still contains stuff that you have done (like encoding
> | binary files)
> 
> No, you're working with an old copy.

I'll update and see what changed.

> 
> | Do you handle changing the executable bit? I saw that in the 'add' line
> | you say whether a file is executable or not, but I don't know if you
> | transmit changes properly. (You very well could, I just wondered if it
> | was tested).
> 
> Yes.  Every time it's supplied, that's a potential change.  It's tested
> around line 487 of test_changeset.py, the a at cset-0-3 revision.
> 
> | It was originally intended that bzrlib.changeset.serializer.* would both
> | write and read the different versions of changesets. You seem to still
> | be using the write side of it, but your read side still raises
> | NotImplemented.
> 
> Yes.  Once I realized what was going on, I decided to focus on getting
> it working and tested, before doing a lot of refactoring.
> 

Sure. It is nice to have working code to refactor, rather than something
broken. :) Now we just need to get it to add revision signatures.

> | Basically, I would move most of the code in
> | bzrlib.changeset.read_changeset.ChangesetReader into serializer.v07, and
> | then just have ChangesetReader read in the first line to determine what
> | version the changeset is, and then have it call the appropriate
> | serializer. By using the factory, we can support multiple versions of
> | changesets.
> 
> That's fine with me, although I don't think the ChangesetReader easily
> supports the API described for the serializer.

I think for 0.7, I'm just going to have the 0.7 serializer create a
ChangesetReader. But I don't think it should be part of the public API.
So stuff like merge_changeset() shouldn't take a ChangesetReader object.
> 
> | Part of the issue is that my old plugin didn't use a factory, so you
> | have a mix of the plugin code, and what the code was turning into.
> 
> Yes.  Took a while for me to understand what was going on, but I figured
> that.
> 
> | Again, thanks for getting this stuff off the ground.
> 
> No problem.
> 
> Aaron

John
=:->

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 254 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060521/dc0a4178/attachment.pgp