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

From: Catalin Marinas <catalin.marinas@gmail.com>
Date: Jun 16, 2008, 10:00 UTC
Message-ID: <b0943d9e0806160300q55dde16fg90de0fa12e3d5dc0@mail.gmail.com>
In-Reply-To: <20080614094714.GC14282@diana.vm.bytemark.co.uk>

2008/6/14 Karl Hasselström <kha@treskal.com>:

Show 7 quoted lines

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

I think the initial idea belongs to Yann.

Show 9 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?

OK, I was thinking about that. I'll convert it to git_commit(...) and return a commit since I think it's only "stg id" that needs the SHA1 value.

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

I thought about removing them when we convert the commands to the new infrastructure. In the meantime, I can rewrite git_id to use git_commit directly. The parse_rev is only used by the 'pick' command (and git_id). I'll have a look at these functions.

Show 8 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.

I can swap them and have the patch as preferred. People can be more precise if they want the branch or tag by passing heads/... or tags/...

Show 7 quoted lines

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

No problem with the option but I would like to keep the branch argument to git_sha1. There might be cases where it is used like picking multiple patches and I only specify one -B option to 'pick'. I don't want to build the qualified patch name for every patch. I can change the prototype of git_sha1 though (to git_commit):

def git_commit(name, repository, branch = None):

and branch needs to be passed explicitly and used instead of the default one (when the patch name doesn't contain any).

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

The id format is new.

-- 
Catalin