[PATCH v2 0/3] rev-parse and "--"

From: Jeff King <peff@peff.net>
Date: Dec 6, 2013, 22:05 UTC
Message-ID: <20131206220520.GA30652@sigill.intra.peff.net>
In-Reply-To: <20131206211509.GB20536@sigill.intra.peff.net>

On Fri, Dec 06, 2013 at 04:15:09PM -0500, Jeff King wrote:

Show 18 quoted lines

> If you have both a file and a branch named "foo", running:
> 
>   git log foo
> 
> will complain. We should do the same in rev-parse, and
> demand that it be disambiguated with:
> 
>   git rev-parse foo --
> 
> or
> 
>   git rev-parse -- foo
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Hmm, looking at this again, I guess we need to give the same treatment
> to the try_difference code path above, as "foo..bar" can be ambiguous
> with a filename (and "git log" seems to diagnose that).

Here's a revised series which handles that:

  [1/3]: rev-parse: correctly diagnose revision errors before "--"
  [2/3]: rev-parse: be more careful with munging arguments
  [3/3]: rev-parse: diagnose ambiguous revision/filename arguments

While fixing this commit, I noticed another bug whose fix is a prerequisite. That's now patch 2 in the series. I think the first two patches are correct and should be applied.

Patch 3 is the revised version of this patch which notices ambiguity. However, I'm having second thoughts on it. I think it's the right thing to do if you want to help people build something like "git log" themselves. But it does mean that we are breaking somebody who does:

  echo foo >HEAD
  commit=$(git rev-parse HEAD)

I'm tempted to say that people who did that are stupid and wrong (and ugly, too). They should probably be using "--verify" in this case. But it has been that way for a long time, and there are two instances in our test scripts that are broken by the patch.

Is it better for "rev-parse" to be more careful, and to behave more like the rest of git? Or is better to be historically compatible?

One thing to note is that "git rev-parse HEAD" is slightly broken there already. Because "git rev-parse $some_branch" may do very different things than what the caller expects if $some_branch does not exist (but there is a file with the same name). So maybe we are doing a favor by calling out the problem; if they want a rev, they should be using "--verify" (or "--").

-Peff