Re: [PATCH 19/20] abbrev: support relative abbrev values
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 12, 2018, 19:16 UTC
- Message-ID
- <xmqqvaan95os.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <20180608224136.20220-20-avarab@gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 23 quoted lines
> 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 ;-)
Show 8 quoted lines
> 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?
> 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.