Re: [PATCH] find_unique_abbrev(): honor caller-supplied "len" better

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Mar 10, 2011, 23:07 UTC
Message-ID: <AANLkTimH9=OWD4+dPsdYaL2VPdDkTsUG_N3GBf168XqD@mail.gmail.com>
In-Reply-To: <7vhbba38mm.fsf_-_@alter.siamese.dyndns.org>

On Thu, Mar 10, 2011 at 2:37 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 9 quoted lines

>
> The code however forgot that the function may be called with a "len" that
> is long enough.  If an object is uniquely identifiable with only 4 leading
> characters today, and if the caller gives 7 as len and the guard is set to
> 3, it returned 10 hexdigits, which was 3 characters longer than necessary.
> We should instead return 7 leading characters in such a case, as that is
> in line with the original intention of using 3 characters more than
> absolutely necessary to give the disambiguation we find today a better
> chance to survive.

The thing is, that just makes the notion of "abbrevguard" pointless. Why have it?

When you pass in 6 as a len, and that isn't sufficient, it expands it to (say) 10. And then you pass in 7 as a length, and now it's sufficient, so it keeps it at 7.

That's just stupid. You gave it a bigger length suggestion, and you got a smaller end result. That's crazy.

However, I think the _real_ problem is not whether that behavior is really stupid or not. I think the real problem is that abbrevguard really isn't a well-defined, and you get this kind of crazy semantics.

So I think the REAL problem is different:

 (a) DEFAULT_ABBREV is just too damn small. 7 made sense as a random
number back when we did this, but we're talking over 5 years ago. The
seven comes from commit 47dd0d595d04e. Back then, a million objects
was a really almost inconceivably big number.  Even the BK tree (that
I was going by as a target) was just 65k revisions for Linux, so with
most changes only touching a few files, "million" was "long time in
the future". Now we're close to 2 million.

It turns out 640kB isn't enough for everybody. For the kernel, we have several objects that need 10 digits just for uniqeness right NOW.. 12 digits is a _somewhat_ reasonable safe value for the forseeable future. But 11 would be too short. And I don't think the kernel is the biggest repo.

 (b) You can't change DEFAULT_ABBREV except with the command line option.

 (c) Even there it's unnecessarily hard.  Want to see your commit
numbers abbreviated appropriately too? Oh, you have to use
"--abbrev=12 --abbrev-commit". We didn't think the interface through.

 (d) some places don't even take the command line option. Grep for
DEFAULT_ABBREV, and notice how often it's just used as-is.

So I would suggest ditching 'unique_abbrev_extra_length' entirely. I doubt anybody uses it, and the whole concept is simply badly designed with crazy semantics as per your patch.

                              Linus