Re: [PATCH 1/2] sha1_name: try to use same abbrev length when core.abbrevguard is specified

[Namhyung Kim <namhyung@gmail.com>]

2011-03-09 (수), 11:12 -0800, Junio C Hamano: 
> Namhyung Kim <namhyung@gmail.com> writes:
> 
> > If find_unique_abbrev() finds a ambiguous SHA1 name, it tries
> > to find again with increased length. In this case, result hex
> > strings could have different lengths even though the
> > core.abbrevguard config option is specified. But if the option
> > is specified and increased length (delta) is less than its
> > value, the result could be adjusted to the same length.
> 
> I am not sure if I can understand what problem you are trying to solve
> from the above description.
> 

That's probably because of my poor English. :(


> The function is given "len" from the caller to specify the minimum length
> of the output the caller expects (i.e. even if 4 hexdigits is enough to
> identify the given commit in a small project, the caller can say it wants
> to see at least 7 hexdigits).  The loop without your patch finds the
> shortest prefix whose length is at least that given length that uniquely
> identifies the given object (or the shortest prefix that doesn't identify
> any existing object if the given sha1 does not exist in the repository).
> And then ensures the returned value is longer by the guard as an extra
> safety measure, so that later when the project grows, the disambiguation
> we find today has a better chance to survive.
> 
> With this patch, the loop decreases the length of the guard when "len"
> given by the caller is insufficient to ensure uniqueness, which does not
> sound right.
> 
> Suppose the given object has ambiguous other objects and you need 8
> hexdigits at least to make it unique in today's history.  The caller gives
> you len of 7, and the guard is set to 3.
> 
> With the original code, the loop starts with 7, finds that it is not
> long enough to disambiguate, increments and retries, finds that 8 is the
> shortest prefix, and then adds the guard and returns 11 hexdigits.
> 
> With your patch, the loop starts with 7 with extra set to 3, finds that 7
> is not long enough and decrements extra to 2, finds that 8 is the shortest
> prefix, and then returns only 10 hexdigits.
> 
> Which feels like totally going against the reason why we added the guard.
> 
> What am I missing?
> 

What I was thinking is like below (from a user's point of view):

I knew git use 7 hexdigit to represent a commit object and it was not
enough for my project. So I gave it 5 for the guard and expected to see
12 in everywhere - if 12 always guarantees uniqueness for the project
now and probably, the near future. But sometimes git prints 13 even
though 12 is long enough to represent the commit uniquely. Because 7 is
not enough but 8 is, and simply add 5 (the guard) to it.

So I thought it would be natural to print 12 always in this case and
thus, submitted the patch.

Thanks.

-- 
Regards,
Namhyung Kim