Re: [PATCH 19/20] abbrev: support relative abbrev values

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jun 13, 2018, 22:22 UTC
Message-ID: <871sdawcmh.fsf@evledraar.gmail.com>
In-Reply-To: <xmqqvaan95os.fsf@gitster-ct.c.googlers.com>

On Tue, Jun 12 2018, Junio C Hamano wrote:

Show 40 quoted lines

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Change the core.abbrev config variable and the corresponding --abbrev
>> command-line option to support relative values such as +1 or -1.
>>
>> Before Linus's e6c587c733 ("abbrev: auto size the default
>> abbreviation", 2016-09-30) git would default to abbreviating object
>> names to 7-hexdigits, and only picking longer SHA-1s as needed if that
>> was ambiguous.
>>
>> That change instead set the default length as a function of the
>> estimated current count of objects:
>>
>>     Based on the expectation that we would see collision in a
>>     repository with 2^(2N) objects when using object names shortened
>>     to first N bits, use sufficient number of hexdigits to cover the
>>     number of objects in the repository.  Each hexdigit (4-bits) we
>>     add to the shortened name allows us to have four times (2-bits) as
>>     many objects in the repository.
>>
>> By supporting relative values for core.abbrev we can allow users to
>> consistently opt-in for either a higher or lower probability of
>> collision, without needing to hardcode a given numeric value like
>> "10", which would be overkill on some repositories, and far to small
>> on others.
>
> Nicely explained and calculated ;-)
>
>>  test_expect_success 'describe core.abbrev=[-+]1 and --abbrev=[-+]1' '
>> -	test_must_fail git -c core.abbrev=+1 describe | sed_g_tr_d_n >describe &&
>> -	test_must_fail git -c core.abbrev=-1 describe | sed_g_tr_d_n >describe &&
>> +	git -c core.abbrev=-1 describe | sed_g_tr_d_n >describe &&
>> +	test_byte_count = 6 describe &&
>> +
>> +	git -c core.abbrev=+1 describe | sed_g_tr_d_n >describe &&
>> +	test_byte_count = 8 describe &&
>
> Even though I see the point of supporting absurdly small absolute
> values like 4, I do not quite see the point of supporting negative
> relative values here.  What's the expected use case?

I'll add a better explanation for this to the commit message.

Initially I did this for consistency, since it was easy to implement, and there's no reason to have that arbitrary limitation, but thinking about it again I think I'll use this for some of my projects.

E.g. here's a breakdown of my dotfiles repo:

    $ git -c core.abbrev=4 log  --pretty=format:%h|perl -nE 'chomp;say length'|sort|uniq -c|sort -nr
        784 4
         59 5
          7 6

I don't have a single commit that needs 7 characters, yet that's our default. This is a sane trade-off for the kernel, but for something that's just a toy or something you're playing around with something shorter can make sense.

SHA-1s aren't just written down, but also e.g. remembered in wetware short-term memory.

Show 24 quoted lines

>>  	git log --abbrev=+1 --pretty=format:%h -1 | tr_d_n >log &&
>> -	test_byte_count = 4 log &&
>> +	test_byte_count = 8 log &&
>
> This, together with many many others in the rest of the patch, is
> cute but confusing in that the diff shows change from 4 to 8 due to
> the redefinition of what abbrev=+1 means.  I have a feeling that it
> may not be worth doing it "right", but if we were doing it "right",
> we would probably have done it in multiple steps:
>
>     - the earlier patches in this series that demonstrates
>       --abbrev=+1 is --abbrev=1 and core.abbrev=+1 is an error.
>
>     - ensure --abbrev=+1 is rejected as syntax error just like
>       core.abbrev=+1 was, without introducing relative values
>
>     - introduce relative value.
>
> That way, the last step (which corresponds to this patch) would show
> change from "log --abbrev=+1" failing due to syntax error to showing
> abbreviated value that is slightly longer than the default.
>
> But a I said, it may not be worth doing so.  "Is it worth supporting
> negative relative length?" still stands, though.

I'll see what I can do about this value churn.