[PATCH] cleanup branch.find_branch_root() (was Re: About merging)
David Vanderson
dave.vanderson at cox.net
Mon Jun 27 01:24:02 BST 2005
John A Meinel wrote:
>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.
>
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?
>
>>>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.
>
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.
>
>>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.
>
Okay got it - I just thought the "." was implied.
>
>>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.)
>
>
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.
>> 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.
>
I put it back. Sorry about that.
>
>The rest all looks about the same, where I would replace your
>Branch(find_branch_root()) with the simpler find_branch().
>
>John
>=:->
>
Thanks,
Dave
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: cleanup_find_branch_root.patch.2
Url: https://lists.ubuntu.com/archives/bazaar/attachments/20050626/ca0432dd/attachment.diff
More information about the bazaar
mailing list