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

[James Henstridge]

On 02/09/06, John Arbash Meinel <john at arbash-meinel.com> wrote:
> James Henstridge wrote:
> > 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).
>
>
> 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).

I took the code in Revision._check_properties() to be canonical w.r.t.
what are and aren't valid property names.  It currently just checks to
make sure it is an instance of basestring and contains no whitespace.

If the property names should be plain ascii, it should probably
include the stricter checks too.

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

As discussed on IRC, I see the working tree revision properties as
uncommitted changes.  Pull and update don't clobber uncommitted
changes, so I'd expect them not to clobber the revprops.

Is there a better place to reset the revprops given this?

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

As mentioned above, I changed this bit to match the validation rules
found in Revison._check_properties().  I am not sure if Rio's key
constraints apply here, since the testament doesn't seem to be in Rio
format.



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

It would probably be useful to have some kind of "call me before the
next commit" type API, but I'm not sure if its usage would be limited
to just revision properties.  I am not sure what such an API should
look like to tell the truth.

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

I modeled this method on WorkingTree.merge_modified(), which does
pretty much the same thing.  I guess it would probably be simpler to
just do hashfile.readline() here -- I'm not sure why merge_modified()
relies on iterator behaviour for reading the header.


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

I actually chose this serialisation in case there were colons in the
property name (I assume they're allowed in property names?).

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

Thank you for the review.

James.