From: Karl Hasselström <kha@treskal.com>
Date: Sun, 22 Jun 2008 15:48:54 GMT
Subject: Re: [StGIT PATCH 3/4] Convert git_id() to the new id format
Message-ID: <20080622154854.GC4468@diana.vm.bytemark.co.uk>
In-Reply-To: <20080619214222.27794.74083.stgit@localhost.localdomain>

On 2008-06-19 22:42:22 +0100, Catalin Marinas wrote:

> The patch rewrites git_id() to use the new id format and coverts the
> commands using this function. The git_id() will be removed once all
> the commands are converted to the new infrastructure where
> git_commit() will be used instead.

Looks good. And the code volume reduction is significant.

>      if not rev:
> +        # backwards compatibility
>          return None

Could you expand this comment a bit? It's not enough of a clue for me.
:-/

> -def git_commit(name, repository, branch = None):
> +def git_commit(name, repository, branch_name = None):

Very nice parameter rename here, now that we have Branch objects (and
use a crappy language with no type system).

> -rev = '([patch][//[bottom | top]]) | <tree-ish> | base'
> -
> -If neither bottom nor top are given but a '//' is present, the command
> -shows the specified patch (defaulting to the current one)."""
> +rev = '([branch:]patch) | <tree-ish> | base'

You can remove the parentheses now; they were only needed because they
used to enclose a complicated expression. Besides, shouldn't it be
[branch:]{base} instead of base? So something like

  rev = [<branch>:]<patch> | [<branch>:]{base} | <tree-ish>

>  help = 'show the files modified by a patch (or the current patch)'
> -usage = """%prog [options] [<patch>]
> +usage = """%prog [options] [[<branch>:]<patch>]

Unrelated to this patch: I realized last week that it's silly for stg
files to not accept a patch range.

>      if len(args) == 0:
> -        patch = ''
> +        patch = 'HEAD'

Ah, so this is the backwards compatibility thing -- we used to pass
the empty string when we meant HEAD.

> -        (refpatchname, refbranchname, refpatchid) = parse_rev(patchname)
> -        if refpatchname and not refpatchid and \
> -               (not refpatchid or refpatchid == 'top'):
> -            # FIXME: should also support picking //top.old
> +        refbranchname, refpatchname = parse_rev(patchname)
> +        if refpatchname:

The corresponding TODO comment now would be that pick should be able
to pick patches from the past, from the stack log.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle