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

["Karl Hasselström" <kha@treskal.com>]

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.

> +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.

> +    # 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