On Thu, Mar 11 2021, Junio C Hamano wrote:

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

quoted

Remove the last users of the $_x05 variable from the tests. It turns
out that all of these tests can be rewritten unambiguously to simply
use [0-9a-f]* instead.

I am unsure about the "unambiguously" part, e.g.

quoted

-	sed -e "s/ $_x05[0-9a-f]*	/ X	/" <current >check &&
+	sed -e "s/ [0-9a-f]*	/ X	/" <current >check &&

does't the preimage say "we expect at least 5 hexdigits to be shown
here"?  The postimage lets even an empty string to pass. 

quoted

In the case of the tree matching we're relying on there being a <TAB>
after the SHA (but a space between the modes and type), then in some
of the other tests here that an abbreviated SHA is at the start of the
line, etc.

Sure, but these being tests, I am not sure we should be assuming the
correct input to these transformations.

quoted

 test_expect_success 'ls-tree --abbrev=5' '
 	git ls-tree --abbrev=5 $tree >current &&
-	sed -e "s/ $_x05[0-9a-f]*	/ X	/" <current >check &&
+	sed -e "s/ [0-9a-f]*	/ X	/" <current >check &&
 	cat >expected <<\EOF &&
 100644 blob X	1.txt
 100644 blob X	2.txt

This one is particularly iffy.  The --abbrev=5 test is designed to
ensure that the resulting abbreviated object names are at least 5
hexdigits long, even when the repository is so small that only 4
hexdigits are sufficient to avoid ambiguity, while allowing the
output to be longer than specified 5 (when 5 turns out to be
insufficient for disambiguation).

So, I dunno.

Yes, I think this patch should be dropped. Do you mind just dropping the
4/4 and having it be a 3-patch series? I can also re-submit a v2 like
that if it's easier...

I saw the feedback on 3/4, yeah I also think that "cut" is a bit nasty,
but probably not worth spending more time on it...

My assumption in writing this patch was that it was fine because we test
the details of abbrev behavior somewhere else, surely...

But is it turns out we don't, I was misrecalling that some version of my
testing for abbreviations in [1] had landed.

But alas it didn't. From rebasing it now (it's far from submission
quality yet) I see that the lack of testing on abbreviations in the
interim has silently caused some regressions on master.

E.g. we seemingly unintentionally started accepting "-c core.abbrev="
(empty string) in a9ecaa06a7 (core.abbrev=no disables abbreviations,
2020-09-01) is a side-effect of starting to accept boolean values. Also
"git branch --abbrev=-12345" became a non-error in the interim (haven't
dug, but some of the refactoring to parse_options() I assume..).

So yeah, I think this patch is a bad idea now, our test coverage for
abbreviations is really hanging on by a thread.

1. https://lore.kernel.org/git/20180608224136.20220-1-avarab@gmail.com/ (local)