[PATCH] bookmarks: simplify code

[Adrian Buehlmann]

On 2012-10-17 19:21, Kevin Bullock wrote:
> # HG changeset patch
> # User Kevin Bullock <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>.
> 
> 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 len(marks) == 0:
>              ui.status(_("no bookmarks set\n"))
> -        if inactive:
> +        elif inactive:
>              if not repo._bookmarkcurrent:
>                  ui.status(_("no active bookmark\n"))
>              else:
>                  bookmarks.setcurrent(repo, None)
> -            return
>          else:
>              for bmark, n in sorted(marks.iteritems()):
>                  current = repo._bookmarkcurrent
> @@ -879,7 +874,6 @@ def bookmark(ui, repo, mark=None, rev=No
>                      ui.write(" %s %-25s %d:%s\n" % (
>                          prefix, bmark, repo.changelog.rev(n), hexfn(n)),
>                          label=label)
> -        return
>  
>  @command('branch',
>      [('f', 'force', None,