Re: [StGIT PATCH] Implement a new patch identification scheme and id command

From: Karl Hasselström <kha@treskal.com>
Date: Jun 14, 2008, 09:47 UTC
Message-ID: <20080614094714.GC14282@diana.vm.bytemark.co.uk>
In-Reply-To: <20080614072833.7899.91460.stgit@localhost.localdomain>

On 2008-06-14 08:28:33 +0100, Catalin Marinas wrote:

> The new scheme allows '[<branch>:]<patch>' and '[<branch>:]{base}'
> (the latter showing the base of a stack). The former format allows
> symbols like ^, ^{...} etc.

I like your choices.

Show 6 quoted lines

> +def git_sha1(repository, branch, name):
> +    """Return the SHA1 value if 'name' is a patch name or Git commit.
> +    The patch names allowed are in the form '<branch>:<patch>' and can be
> +    followed by standard symbols used by git-rev-parse. If <patch> is '{base}',
> +    it represents the bottom of the stack.
> +    """

Why not return the Commit directly, and let the caller extract its sha1 if that's what it wants?

You don't remove the old parse_rev() and git_id(), and particularly the latter has a lot of callers. Meaning that the rest of StGit still speaks the old syntax.

Show 5 quoted lines

> +    # Try a Git commit first
> +    try:
> +        return repository.rev_parse(name, discard_stderr = True).sha1
> +    except libgit.RepositoryException:
> +        pass

What if you have a branch or tag with the same name as a patch? This will prefer the branch, which might not be the best choice.

> +current one. The bottom of a patch is accessible with the
> +'[<branch>:]<patch>]^' format."""

You have an extra ] here.

> -directory = DirectoryHasRepository()
> +directory = common.DirectoryHasRepositoryLib()
>  options = [make_option('-b', '--branch',
>                         help = 'use BRANCH instead of the default one')]

Couldn't we kill this option? (And in the process, the branch argument to git_sha1.)

> -test_expect_success 'Try new form of id with slashy branch' \
> +test_expect_success 'Try new id with slashy branch' \

Strictly speaking, this isn't so new anymore.

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