[PATCH] cleanup branch.find_branch_root() (was Re: About merging)

[John A Meinel]

David Vanderson wrote:
...

>> Actually, I was recommending that it would use: branch =
>> find_branch('.')
>> not  branch = Branch(find_branch_root('.'))
>>
>> Especially since you start getting stuff like this:
>> out_name = os.path.basename(local_dir) + '-' +
>> str(bzrlib.Branch(bzrlib.branch.find_branch_root(local_dir)).revno())
>>
>> calling str(bzrlib.find_branch(local_dir).revno()) is a bit more
>> readable.
>> Though honestly both are a little bit excessively nested. (That is my
>> bad code, not yours :)
>> But actually, that one was actually expecting that it was getting
>> exactly local_dir, though only weakly.
>>
> I've reworked the patch to do this, and it is substantially more
> readable.  I'm a little worried because find_branch can give you back
> a RemoteBranch object, which was not possible with the previous code.
> Anyone think that's a problem?

Actually, it only gives you a RemoteBranch if you actually specify an
"http://" path. If someone tried to do that now, they would get an
exception (since Branch cannot recognize an http:// path). With you
patch, it might actually succeed, as long as you don't do any writing.
Which to me is a good thing, as it means more commands can work over a
remote connection.

...

> There we use realpath, but in find_branch_root (now
> find_local_branch_root) we have:
> if hasattr(os.path, 'realpath'):
>        f = os.path.realpath(f)
>    else:
>        f = os.path.abspath(f)
>
> which looks like protection for a platform that doesn't have realpath.
>
It might have been in the past that win32 didn't have
os.path.realpath(), but on my windows box os.path.realpath =
os.path.abspath. So it exists in python2.3 which is the lowest supported
version. I think this code is a relic and can be removed.
...

>> Well, other programs (specifically arch) do it this way. I think if you
>> do "bzr diff ." you only get the changes in that directory and
>> underneath. I can see possibly changing the output so that it knows you
>> are not in the same directory. Except that is rather complex to do, and
>> it means if you are in a subdir making changes, and do "bzr diff >
>> ../patch" the patch is non-standard.
>>
> Okay got it - I just thought the "." was implied.

That is arguably true. I suppose you could change cmd_diff.run()
something like:

if not file_list:
    file_list = ['.']
file_list = [b.relpath(f) ...] #and so on

Which would do what you want. I don't know how people feel.
...

> I think this is a larger issue.  Which commands should be picky about
> getting the exact path to the branch root?  In this case you'd want
> version, but we probably want others like revno, check, and upgrade to
> bail out if not given exactly a branch root.

I don't know. I don't mind 'bzr revno' to be able to work in a subdirectory.
But I agree that check & upgrade should probably be given an explicit path.

>
>>>
>>
...

>>>
>>> +
>>> +
>>> +def find_branch(f, **args):
>>> +    if f and (f.startswith('http://') or f.startswith('https://')):
>>> +        import remotebranch
>>> +        return remotebranch.RemoteBranch(f, **args)
>>> +    else:
>>> +        return Branch(find_branch_root(f), **args)
>>> +
>>>
>>>
>>>
>> I guess you did keep it. I would recommend not moving things unless you
>> have to. Because it shows up weird in the diff.
>>
> I put it back.  Sorry about that.

Not a big deal. Just something to be aware of.

>
>>
>> The rest all looks about the same, where I would replace your
>> Branch(find_branch_root()) with the simpler find_branch().
>>
>> John
>> =:->
>>
> Thanks,
> Dave

You new attachment looks like what *I* had in mind. Now lets see if
Martin has something to say about it. :)
John
=:->

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 253 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20050626/0e8173c6/attachment.pgp