Changesets feature complete

[Aaron Bentley]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

John Arbash Meinel wrote:
| Aaron Bentley wrote:
|
|>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.

That's fine with me, but it does leave me perplexed about the
verify-changeset command, because I'm pretty sure "verify" doesn't imply
"install".

|>| 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'.

No, do_meta was only invoked when the tree delta showed that the execute
bit had changed.

| (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'.

Your two statements "I would default 'executable' to no, since that is
the common case" and "I prefer the 'set it only if it changes'" seem
inconsistent to me.

Perhaps you're not considering that 'add' is a change, but it is.  So if
we're setting it for every change, not defaulting it to 'no', then it
will be set for every added file.

|>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.

No, your patch changed the implementation of ChangesetTree.  I was
talking about changing only ChangesetReader._update_tree

|>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.

Yeah, I guess we should, so that mailing a changeset supplies all the
info that publishing a branch does.  I'm not very enthusiastic about
signing, especially when there's no verification.

But I'd like to encourage you to focus on getting it into a mergeable
state, rather than making it perfect.  I do want to get it into the core
so it doesn't bitrot again.  Once it's in, presumably we can update it.

|>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.

I don't really understand.  I guess I'll have to see what you come up with.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFEcNO80F+nu1YWqI0RAiulAJsGyZLCjaZHurst8z0B2BhQ0iBobQCfYfPR
BaDfcCudQ0faSNmWf889wpQ=
=2Dgh
-----END PGP SIGNATURE-----