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

Ævar Arnfjörð Bjarmason  [off-list ref] 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 ;-)

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?

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.

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

Ævar Arnfjörð Bjarmason  [off-list ref] writes:

quoted

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 ;-)

quoted

 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.

quoted

 	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.