Changesets feature complete

[Aaron Bentley]

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

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

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

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.

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

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.

| Your BRANCH.TODO still contains stuff that you have done (like encoding
| binary files)

No, you're working with an old copy.

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

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

| 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
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFEcK4t0F+nu1YWqI0RAhppAJ0bAj+i1u2u+6N/cIdMXDioqqeiQACgg2rV
9i+NJxKFdIfPDWhIftrgWcg=
=FKJi
-----END PGP SIGNATURE-----