Web lists-archives.com

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




Ævar Arnfjörð Bjarmason  <avarab@xxxxxxxxx> 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?

>  	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.