Changesets feature complete

[John A Meinel]

Aaron Bentley wrote:
> Aaron Bentley wrote:
>> Michael Ellerman wrote:
> 
>>> # bzr cset -r 1..-1 > patch
>>>
>>> I was expecting this to be equivalent to the concatenation of all the
>>> individual changesets between 1 and -1. But it's not quite. The last
>>> changeset seems to contain stuff from way back in the past, which is
>>> confusing because it means the message doesn't accord with the diff.
>>> I'm not sure if that's the expected behaviour or not.
>>
>>
>> This is by design.  'Changeset' is a really bad word to describe what
>> these things have become-- they're more like a collection of revisions
>> in a textual format.
> 
> I should clarify that it's by the *current* design.  I'm not stuck on it.

First, let me say thanks for cleaning up my work, and getting it into a
usable state.

> 
> I want mailing changesets to be a viable alternative to publishing
> branches, so I think we need to do this *some of the time*, at least.
> But perhaps that should be reserved for the "bzr cset -r 6
> TARGET_BRANCH" form.
> 
> Making it optional would mean there are two kinds of changesets, which
> makes bzr a bit more complicated.
> 
> It's a fine balancing act, because changesets need to be both
> human-readable (so they're acceptable on mailing lists) and complete (so
> they aren't inferior to publishing branches).
> 
> 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.

> 
> Aaron
> 
> 

Some other comments:

Do you support verifying revisions by their testament before they are
added to the repository? 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. (It looks like you are doing
this correctly. I just wanted to double check).

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

     def get_last_changed(self, file_id):
         path = self.id2path(file_id)

=== modified file 'bzrlib/changeset/serializer/v07.py'
--- bzrlib/changeset/serializer/v07.py

+++ bzrlib/changeset/serializer/v07.py
@@ -194,7 +194,8 @@

         def do_meta(file_id):
             ie = new_tree.inventory[file_id]
-            w(' // executable:%s' % bool_text[ie.executable])
+            if ie.executable:
+                w(' // executable:%s' % bool_text[ie.executable])

         def do_target(target):
             w(' // target:%s' % target)

Originally my changeset code only put out the executable bit if it had
changed. But I think it is straightforward to just only include it if it
is true (just like we do with inventories).

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

There are some PEP8 issues in errors.py and iterablefile.py (there
should be a blank line between the comment and __init__). There are a
few other PEP8 things.

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

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.

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.

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.

Again, thanks for getting this stuff off the ground.

John
=:->

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