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

[James Henstridge]

On 05/09/06, Aaron Bentley <aaron.bentley at utoronto.ca> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> James Henstridge wrote:
> > 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.
>
> Your API sets the revprops, right?  So say your current revision, revno
> 2, has {"permitted_colors": "red"} and you decide you no longer want to
> permit "red".  You set a working tree revprop for {"permitted_colors":
> ""}.  Then you update to revno 3, where the revprop is set to
> {"permitted_colors": "red blue"}.  What should the working tree
> "permitted_colors" setting be?  I don't think we have enough information
> to answer this.  So there's a 50/50 chance it's wrong to
> permitted_colors to "" when we commit revno 4.  I think this should be
> considered a conflict.  Which makes me wonder: should merge be setting
> the revision properties for working trees?

The revision properties on the working tree would be for the revision
that is going to be committed.  It is not related to revision
properties set on any previous revision.

So a clean working tree would have no revision properties set: just
because rev 2 has permitted_colours=red doesn't mean that rev 3 will
have permitted_colours=red.

If I make some changes and set a working tree revprop, I am saying I
want that revprop set on the revision where I commit those changes.
Doing a "bzr update" or merging other revisions doesn't affect that.

Your permitted_colours example doesn't really match up with the
existing uses of revprops I've seen.  Do you have a more concrete
example where that sort of behaviour would be wanted?


> > 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.
>
> Well, it's partly because iterators are cheaper than lists, both in
> execution time and memory use, and partly because I just like iterators.
>   It seemed sensible to me to read the header line specially, and then
> pass the iterator on to code intended to consume the body.
>
> I don't think the code would be much simpler using readlines:
>
>         hashlines = hashfile.readlines()
>         if len(hashlines) < 1;
>             raise MergeModifiedFormatError()
>         if hashlines[0] != MERGE_MODIFIED_HEADER_1 + '\n':
>             raise MergeModifiedFormatError()
>         for s in RioReader(hashlines[1:]):
>             file_id = s.get("file_id")
>             if file_id not in self.inventory:
>                 continue
>             hash = s.get("hash")
>             if hash == self.get_file_sha1(file_id):
>                 merge_hashes[file_id] = hash
>         return merge_hashes

Well, it could be written as:
    if hashfile.readline() != MERGE_MODIFIED_HEADER_1 + '\n':
        raise MergeModifiedFormatError()
    for s in RioReader(hashfile):
        ...

Lastly, do you have any thoughts on what should be considered valid
revision property names?

James.