On Thu, Mar 11 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: > >> 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. > >> - 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. > >> 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. > >> 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@xxxxxxxxx/
