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

[John A Meinel]

David Vanderson wrote:

>
>
> John A Meinel wrote:
> <snip>
>
>> I think the preferred fix is to make a Branch() object always require an
>> explicit base, which must exist, and must be the root. And then change
>> all of the other places in the code to use "find_branch".
>>
>> Admittedly, the current code always wants to search, but in my mind,
>> once I'm instantiating an actual branch object, it should be fully
>> defined what branch I am getting, it shouldn't go and search.
>> If I am calling a function named "find_branch" then I obviously realize
>> that I am searching for the root.
>>
>>
> I agree.  I have reworked the patch.

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.

>
>> Also, I wouldn't change the name of the parameter from "base" to "f".
>> Base has quite a bit more meaning.
>>
>>
> Done.
>
>> Is there any reason that RemoteBranch uses "self.baseurl" instead of
>> "self.base"? It means that when you instantiate a RemoteBranch object,
>> you have to realize what type it is (or look for both).
>> If baseurl is important, maybe it could define both.
>>
>> John
>> =:->
>>
>>
>>
> I'm not sure.  Since this is my first real foray into the code, I
> figured I'd try to touch as little as possible.

I was more asking it of the group, hopefully other people have an idea.

>
> Thanks very much for the comments.  I have 2 questions:
>
> In find_branch_root, we check for os.path.realpath, but in
> Branch.__init__ we just use it.  One of those must be wrong, right?
>
In the code I have here, I see this:
        if init:
            self.base = os.path.realpath(base)
            self._make_control()
        elif find_root:
            self.base = find_branch_root(base)
        else:
            self.base = os.path.realpath(base)

Which looks like it always uses realpath(). I'm not sure what you see.
In general Branch.base should always be a fully qualified path.

> If you are in a subdirectory and do a "bzr diff", you'll get the same
> output as if you do "bzr diff" in the branch's root.  Is this
> intentional?

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.

>
> Thanks,
> Dave
>
>
>
Some comments inside your text:

>------------------------------------------------------------------------
>
>*** modified file 'bzrlib/__init__.py'
>--- bzrlib/__init__.py
>+++ bzrlib/__init__.py
>@@ -55,8 +55,8 @@
>     """If bzr is run from a branch, return (revno,revid) or None"""
>     from errors import BzrError
>     try:
>-        branch = Branch(__path__[0])
>-        rh = branch.revision_history()
>+        b = Branch(branch.find_branch_root(__path__[0]))
>+        rh = b.revision_history()
>
>
I would probably actually make this
b = Branch(os.dirname(__path__[0]))
Since the exact location should be known. And I wouldn't want it
accidentally going into a parent branch, and reporting the revision there.
(Say you kept track of $HOME, and had untarred a release version into
$HOME/bzr.dev, it would give you the history for $HOME instead of
thinking it was not in a dev branch.)

>         if rh:
>             return len(rh), rh[-1]
>         else:
>
>*** modified file 'bzrlib/add.py'
>--- bzrlib/add.py
>+++ bzrlib/add.py
>@@ -50,7 +50,7 @@
>
>     user_list = file_list[:]
>     assert not isinstance(file_list, basestring)
>-    b = bzrlib.branch.Branch(file_list[0], find_root=True)
>+    b = bzrlib.branch.Branch(bzrlib.branch.find_branch_root(file_list[0]))
>     inv = b.read_working_inventory()
>     tree = b.working_tree()
>     count = 0
>
>
here b = bzrlib.find_branch(file_list[0]) would be good.

>*** modified file 'bzrlib/branch.py'
>--- bzrlib/branch.py
>+++ bzrlib/branch.py
>@@ -36,16 +36,6 @@
> ## TODO: Maybe include checks for common corruption of newlines, etc?
>
>
>-
>-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(f, **args)
>-
>-
>-
>
>
Obviously I would leave this function. You especially need it, because
you need a way to handle remote branches.

> def _relpath(base, path):
>     """Return path relative to base, or raise exception.
>
>@@ -77,32 +67,43 @@
> def find_branch_root(f=None):
>     """Find the branch root enclosing f, or pwd.
>
>-    f may be a filename or a URL.
>-
>-    It is not necessary that f exists.
>-
>-    Basically we keep looking up until we find the control directory or
>-    run into the root."""
>+    f must be an existing path in a branch.
>+
>+    TODO: should we let f be non-existant?
>+    TODO: will f ever be a URL?"""
>+
>     if f == None:
>         f = os.getcwd()
>-    elif hasattr(os.path, 'realpath'):
>+
>+    if hasattr(os.path, 'realpath'):
>         f = os.path.realpath(f)
>     else:
>         f = os.path.abspath(f)
>+
>     if not os.path.exists(f):
>         raise BzrError('%r does not exist' % f)
>-
>
>     orig_f = f
>
>     while True:
>+        #check for the control directory
>         if os.path.exists(os.path.join(f, bzrlib.BZRDIR)):
>             return f
>+
>         head, tail = os.path.split(f)
>         if head == f:
>-            # reached the root, whatever that may be
>+            # reached the top level directory, whatever that may be
>             raise BzrError('%r is not in a branch' % orig_f)
>         f = head
>+
>+
>+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.

The rest all looks about the same, where I would replace your
Branch(find_branch_root()) with the simpler find_branch().

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