Re: [PATCH] sha1_name: don't resolve refs when core.warnambiguousrefs is false

From: Jeff King <peff@peff.net>
Date: Jan 7, 2014, 22:08 UTC
Message-ID: <20140107220856.GA10074@sigill.intra.peff.net>
In-Reply-To: <xmqqd2k3cz42.fsf@gitster.dls.corp.google.com>

On Tue, Jan 07, 2014 at 12:31:57PM -0800, Junio C Hamano wrote:

Show 10 quoted lines

> >   c. Just leave it at Brodie's patch with nothing else on top.
> >
> > My thinking in favor of (b) was basically "does anybody actually care
> > about ambiguous refs in this situation anyway?". If they do, then I
> > think (c) is my preferred choice.
> 
> OK.  I agree with that line of thinking.  Let's take it one step at
> a time, i.e. do c. and also use warn_on_object_refname_ambiguity in
> "rev-list --stdin" first and leave the simplification (i.e. b.) for
> later.

Here's a series to do that. The first three are just cleanups I noticed while looking at the problem.

While I was writing the commit messages, though, I had a thought. Maybe we could simply do the check faster for the common case that most refs do not look like object names? Right now we blindly call dwim_ref for each get_sha1 call, which is the expensive part. If we instead just loaded all of the refnames from the dwim_ref location (basically heads, tags and the top-level of "refs/"), we could build an index of all of the entries matching the 40-hex pattern. In 99% of cases, this would be zero entries, and the check would collapse to a simple integer comparison (and even if we did have one, it would be a simple binary search in memory).

Our index is more racy than actually checking the filesystem, but I don't think it matters here.

Anyway, here is the series I came up with, in the meantime. I can take a quick peek at just making it faster, too.

  [1/4]: cat-file: refactor error handling of batch_objects
  [2/4]: cat-file: fix a minor memory leak in batch_objects
  [3/4]: cat-file: restore ambiguity warning flag in batch_objects
  [4/4]: revision: turn off object/refname ambiguity check for --stdin

-Peff