[PATCH] bzrlib API to add pending revision properties to a working tree

John Arbash Meinel john at arbash-meinel.com
Sat Sep 2 15:13:03 BST 2006


James Henstridge wrote:
> On 02/09/06, John Arbash Meinel <john at arbash-meinel.com> wrote:
>> James Henstridge wrote:
>> > On 01/09/06, Martin Pool <mbp at canonical.com> wrote:
>>
>> ...
>>
>> >> +1 on the facility.
>> >>
>> >> I don't think they should be called 'pending'.  From the PoV of the
>> >> working tree, they're just *the* revprops.  (Everything about the
>> >> working tree is in this sense pending.)
>> >
>> > Okay, I chose the name to match "pending-merges", but I see those APIs
>> > seem to be wrappers around set/get_parent_ids() now.  So do you think
>> > get_revprops(), set_revprops() and add_revprop() would be better
>> > names?
>> >
>> > James.
>>
>>
>> By the way, you shouldn't use the extension '.bundle'. Because mail
>> programs don't show them as text. It is better to use .diff or .patch,
>> since then we can review the patch just by reading the email.
>>
>> I'll wait to review the second submission.
>>
>> I really think we would rather spell out the names, though:
>>
>> get_revision_properties()
>>
>> Though I would prefer:
>>
>> get_meta_properties() myself.
>>
>> get_properties() seems too minimal and unclear.
>>
>> I think pending-merges was back when we were deciding how we would
>> record merges (back in 0.0.1). Back when everything was in one .bzr/
>> directory (not split up by repository/branch/checkout). And it was
>> possible that we would have a 'pending-merges' for the working stuff,
>> and a 'merges' for the archived stuff. But we recorded it differently.
>>
>> So pending is just because of legacy. :)
> 
> Okay, I've attached an updated bundle that uses "revision_properties"
> instead of "pending_revprops".  I think "meta_properties" would cause
> confusion because (a) they are referred to as revision properties
> elsewhere in the codebase and (b) the equivalent concept in Subversion
> is called revision properties (no need to be different for the sake of
> it).
> 
> James.


v- The only thing I would add, is that you should test Unicode revision
properties. The general action ins that the key can only be ascii, but
that the value can be any unicode string.
I don't know if you want to assert the former (make sure it fails if you
try to use a non-ascii key), but you certainly should test the latter.

So one other thing that I'm seeing is that the key going in and coming
out is u'', but maybe it shouldn't be. (It happens to be a by-product of
how we serialize and unserialize).

Otherwise your tests look very thorough.

> +class TestRevisionProperties(TestCaseWithWorkingTree):
> +
> +    def test_get_revision_properties(self):
> +        t = self.make_branch_and_tree('.')
> +        self.assertEqual({}, t.get_revision_properties())
> +        t.set_revision_properties({u'key1': u'value1'})
> +        self.assertEqual({u'key1': u'value1'}, t.get_revision_properties())
> +        t.add_revision_property('key2', 'value2')
> +        self.assertEqual({u'key1': u'value1',
> +                          u'key2': u'value2'}, t.get_revision_properties()) 

...


>              # and now do the commit locally.
>              self.branch.append_revision(self.rev_id)
>  
> +            # reset the working tree revision properties.
> +            self.work_tree.set_revision_properties({})
> +

^- This probably shouldn't be done in commit. Rather it should be done
in 'WorkingTree.set_last_revision()' or one of its helper functions.
That way, anytime you modify the tree state, it will get reset. (commit,
pull, update --revision when we have it, etc)

This isn't critical if you can explain why you want it here.

...

>              return []
>          r = ['properties:\n']
>          for name, value in sorted(self.revprops.items()):
> -            assert isinstance(name, str)
> +            assert isinstance(name, basestring)
>              assert not contains_whitespace(name)
>              r.append('  %s:\n' % name)
>              for line in value.splitlines():

^- See, this is what I was mentioning, names are supposed to be ascii
only, not Unicode. So if they are unicode, we need to fix whatever
parsing bug allowed them to be. So this change needs to be reverted. And
probably some documentation needs to be updated to make it clear that
Rio is designed to use ascii-only keys. (Probably even more strict than
that, since ':' should not be used, and probably whitespace).

With Rio, if you need key and value to be unicode, you have to use 2
paired keys like:

source: foó
target: bår

Rather than doing:
foó: bår


v-- I agree with Robert, that we need a hook here. Some sort of
"validate_revprops()" function, that has some sort of list of validation
functions to call, passing in some of the important information.
That lets plugins that define specific revision properties act on them
before a commit goes through.

We may not want to block your patch on that, but it should at least be
put in the TODO.

>      def commit(self, message=None, revprops=None, *args, **kwargs):
>          # avoid circular imports
>          from bzrlib.commit import Commit
> -        if revprops is None:
> -            revprops = {}
> -        if not 'branch-nick' in revprops:
> -            revprops['branch-nick'] = self.branch.nick
> +        all_revprops = self.get_revision_properties()
> +        if revprops is not None:
> +            all_revprops.update(revprops)
> +        if not 'branch-nick' in all_revprops:
> +            all_revprops['branch-nick'] = self.branch.nick
>          # args for wt.commit start at message from the Commit.commit method,
>          # but with branch a kwarg now, passing in args as is results in the
>          #message being used for the branch
>          args = (DEPRECATED_PARAMETER, message, ) + args
> -        committed_id = Commit().commit( working_tree=self, revprops=revprops,
> -            *args, **kwargs)
> +        committed_id = Commit().commit(
> +            working_tree=self, revprops=all_revprops, *args, **kwargs)
>          self._set_inventory(self.read_working_inventory())
>          return committed_id
>  
> @@ -796,6 +799,47 @@
>                  merge_hashes[file_id] = hash
>          return merge_hashes
>  

...

v- I'm a little concerned that not all Transports will return a
file-like object that has a 'next()' function. Is it defined as part of
the file api for python? Do urllib files have them? I realize
WorkingTree._transport is pretty much always a LocalTransport, so it
should only return real file objects which have .next(), but I want to
be api safe.

If next() is officially part of the file api, then this is fine. If not,
you can use:
proplist = iter(self._controlfiles.get(...).readlines())

> +    @needs_read_lock
> +    def get_revision_properties(self):
> +        """Get the dictionary of revision properties.
> +
> +        These properties will be applied to the next committed revision.
> +        """
> +        try:
> +            propfile = self._control_files.get('revision-properties')
> +        except NoSuchFile:
> +            return {}
> +        try:
> +            if propfile.next() != REVISION_PROPERTIES_HEADER_1 + '\n':
> +                raise RevisionPropertiesFormatError()
> +        except StopIteration:
> +            raise RevisionPropertiesFormatError()
> +        revprops = {}
> +        for s in RioReader(propfile):
> +            name = s.get('name')
> +            value = s.get('value')
> +            revprops[name] = value
> +        return revprops

^- I see you used name: and value: pairs to make them both unicode. But
I think that violates the Revision constraints. Not because we can't
serialize a Unicode key, but because we don't want to.
Martin has the final say in this, but I'm pretty sure he wanted to
restrict the tag names to a subset of ascii.

So a couple things that we need to work out, but overall it looks really
good.

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/20060902/e2d866e7/attachment.pgp 


More information about the bazaar mailing list