[MERGE][bug #6700] diff on branches without working trees

Ian Clatworthy ian.clatworthy at internode.on.net
Mon Dec 17 01:17:16 GMT 2007


Aaron Bentley wrote:
> bb:approve
> 
> Ian Clatworthy wrote:
>>>> +    if relpath != '':
>>>> +        specific_files.append(relpath)
>>> Is it necessary to avoid appending relpath if blank?  If relpath is
>>> blank, then the user wants a whole-tree diff, and since diff is
>>> recursive, '' will induce a whole-tree diff.
>> I think this statement is required. Consider:
> 
>>   bzr diff tree/foo --old other_branch
> 
>> In this case, you only want to diff foo, not ['', foo].
> 
> I don't understand how you'd get ['', 'foo'] here.  Do you mean that the
> '' would come from other_branch?

Yes.

> Are you suggesting that diff tree/foo --old other_branch/bar would
> produce ['foo', 'bar']?  It seems odd to allow files to be specified as
> part of the old/new urls.

Agreed. But it turns out that way because the first argument is used as
the default for both --old and --new. For example, a user can specify a
file together with just --new and no --old.

>> I do think conversion to working tree relative paths is generally
>> required
> 
> I agree.  I was only reacting to the docstring.
> 
>> unless both an old branch and a new branch are explicitly
>> given, either via --old/--new or within the revspecs
> 
> Even then, I think you could argue that we should relativize the paths
> unless no working tree was present.

I'm in two minds about this, so I've left it as it is. FWIW, I did try
changing the code and a test broke that looked ok to me, so I put it back.

>> +    return rev_branch.repository.revision_tree(revision_id)
> 
> ^^^ technically, you could try WorkingTree.revision_tree before falling
> back to Repository.revision_tree.  But that would only accelerate diff
> -r -1, which is rarely used.

Interesting. How would you change that bit of code to do that? LBYL? I
don't want to make the code more complex that it need be or risk slowing
down common cases, but I'm happy to tweak it if you think it's worth it.

Ian C.



More information about the bazaar mailing list