Re: [PATCH] submodule summary: Don't barf when invoked in an empty repo

From: Jeff King <peff@peff.net>
Date: Feb 17, 2010, 06:10 UTC
Message-ID: <20100217061005.GA520@coredump.intra.peff.net>
In-Reply-To: <201002161121.14613.johan@herland.net>

On Tue, Feb 16, 2010 at 11:21:14AM +0100, Johan Herland wrote:

Show 11 quoted lines

> I'm working from the simple test case in the below patch, I get the
> following output with your proposed fix:
> 
>   [...]
>   trace: built-in: git 'rev-parse' '-q' '--verify' '^0'
>   [...]
>   trace: built-in: git 'diff-index' '--raw' 'HEAD' '--'
>   fatal: bad revision 'HEAD'
>   [...]
> 
> I.e. your fix doesn't work because $1 is empty (not "HEAD") at this point.

Ah, right. I was testing "git status" which calls "git submodule summary HEAD", but your test uses the assumed HEAD. And both need fixing.

Show 5 quoted lines

> My alternative patch (below) does pass my test case (and all the other
> tests as well)
> 
> I'd still like an ACK from the original author (Ping Yin) as well, as I'm
> not sure if I overlooked something by removing the "$1^0".

Your patch looks correct to me. I don't really see how dropping the "^0" will have any effect. rev-parse --verify already prefers revisions to files, and diff-index should peel if needed.

Which, btw, seems like a mini-bug here. The point of this code is to say "if we have an argument, is it a revision? If so, shift it off. Otherwise, put it (and any other arguments) after the -- as a file limiter".

Usually if I have a branch "master" and a file "master", git will complain about the ambiguity unless I use an explicit "--" separator. But here it always takes it as a revision, no questions asked. Or if I am in "--files" mode, that argument is simply removed and ignored (see just below where we unset $head).

Probably it should be re-ordered as

  if test -n "$files"; then
     ...
  else
     if rev=$(git rev-parse -q --verify --default HEAD "$1")
       ...
  fi

It just doesn't come up much because usually having branch names and filenames the same is sufficiently annoying that nobody does it.

-Peff