On Tue, Feb 16, 2010 at 11:21:14AM +0100, Johan Herland wrote:
> 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.
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html