From: Karl Hasselström Date: Sat, 14 Jun 2008 09:47:14 GMT Subject: Re: [StGIT PATCH] Implement a new patch identification scheme and id command 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 '[:]' and '[:]{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 ':' and can be > + followed by standard symbols used by git-rev-parse. If 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 > +'[:]]^' 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