[PATCH] bookmarks: simplify code

Adrian Buehlmann adrian at cadifra.com
Wed Oct 17 16:19:12 CDT 2012


On 2012-10-17 22:17, Kevin Bullock wrote:
> On Oct 17, 2012, at 2:15 PM, Adrian Buehlmann wrote:
> 
>> On 2012-10-17 19:21, Kevin Bullock wrote:
>>> # HG changeset patch
>>> # User Kevin Bullock <kbullock at ringworld.org
>>> <mailto:kbullock at ringworld.org>>
>>> # Date 1350494123 18000
>>> # Node ID 4dc03350b9e8e0e6e25e3796465d0d65463d16da
>>> # Parent  9469bbd2f3a1e39c086383f3fe1543b801991b6f
>>> bookmarks: simplify code
>>>
>>> Remove some unnecessary return statements and collect some checks into
>>> one place. As requested by Thomas Arendsen Hein <thomas at intevation.de
>>> <mailto:thomas at intevation.de>>.
>>>
>>> diff --git a/mercurial/commands.py b/mercurial/commands.py
>>> --- a/mercurial/commands.py
>>> +++ b/mercurial/commands.py
>>> @@ -813,19 +813,18 @@ def bookmark(ui, repo, mark=None, rev=No
>>>         raise util.Abort(_("--rev is incompatible with --delete"))
>>>     if rename and rev:
>>>         raise util.Abort(_("--rev is incompatible with --rename"))
>>> +    if mark is None and (delete or rev):
>>> +        raise util.Abort(_("bookmark name required"))
>>>
>>>     if delete:
>>> -        if mark is None:
>>> -            raise util.Abort(_("bookmark name required"))
>>>         if mark not in marks:
>>>             raise util.Abort(_("bookmark '%s' does not exist") % mark)
>>>         if mark == repo._bookmarkcurrent:
>>>             bookmarks.setcurrent(repo, None)
>>>         del marks[mark]
>>>         bookmarks.write(repo)
>>> -        return
>>> -
>>> -    if rename:
>>> +
>>> +    elif rename:
>>>         if mark is None:
>>>             raise util.Abort(_("new bookmark name required"))
>>>         mark = checkformat(mark)
>>> @@ -837,9 +836,8 @@ def bookmark(ui, repo, mark=None, rev=No
>>>             bookmarks.setcurrent(repo, mark)
>>>         del marks[rename]
>>>         bookmarks.write(repo)
>>> -        return
>>> -
>>> -    if mark is not None:
>>> +
>>> +    elif mark is not None:
>>>         mark = checkformat(mark)
>>>         if inactive and mark == repo._bookmarkcurrent:
>>>             bookmarks.setcurrent(repo, None)
>>> @@ -854,17 +852,14 @@ def bookmark(ui, repo, mark=None, rev=No
>>>         bookmarks.write(repo)
>>>         return
>>>
>>> -    if mark is None:
>>> -        if rev:
>>> -            raise util.Abort(_("bookmark name required"))
>>> +    else: # mark is None
>>
>> IMHO, this is an example of a bad comment. Don't describe in different
>> words what the code is already doing. The comment may be a lie (perhaps
>> not today, but in the future).
> 
> If it were anywhere except on a bare 'else' that's a goodly distance
> removed from the other corresponding clauses, I'd agree. If the comment
> becomes a lie, then the code within the 'else' clause also becomes
> invalid. That being said, the 'else' isn't *so* far from the prior
> 'elif' that I'd be upset if the comment disappeared.

Basically, that comment is just a maintenance burden. Doesn't really serve
much.

It might be a bit less silly, if you would instead describe in words what
"mark is None" means.

Somewhat useful examples:

merge.py:

Line 236:             else: # both changed
Line 245:             else: # case 2 A,B/B/B or case 4,21 A/B/B
Line 275:             else: # case 3,20 A/B/A

discovery.py:

Line 133:         else: # update missing heads

dirstate.py:

Line 655:                 else: # does it match a directory?

filemerge.py:

Line 58:             else: # configured but non-existing tools are more silent

hg.py:

Line 588:     else: # assume it's a global ui object





More information about the Mercurial-devel mailing list