| Message ID | 20210311001447.28254-5-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] git-bisect: remove unused SHA-1 $x40 shell variable | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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.
On Thu, Mar 11 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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@gmail.com/
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >>> 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... As long as the earlier three are good to go, I can just cut the tip of the branch, or just drop it all now and send a three-patch series after the release. Either is fine. > My assumption in writing this patch was that it was fine because we test > the details of abbrev behavior somewhere else, surely... I expected that the test whose title is "ls-tree --abbrev=5" is targetted towards testing the details of abbrev behaviour. Isn't that the case? Is the same assumption brought silent breakages to the other two patches to the tests, by chance? I only gave cursory look on them and don't think anybody else looked at them carefully.
diff --git a/t/t3101-ls-tree-dirname.sh b/t/t3101-ls-tree-dirname.sh index 12bf31022a..110ebf7d52 100755 --- a/t/t3101-ls-tree-dirname.sh +++ b/t/t3101-ls-tree-dirname.sh @@ -187,7 +187,7 @@ EOF 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 diff --git a/t/t3508-cherry-pick-many-commits.sh b/t/t3508-cherry-pick-many-commits.sh index e8375d1c97..1bf867d139 100755 --- a/t/t3508-cherry-pick-many-commits.sh +++ b/t/t3508-cherry-pick-many-commits.sh @@ -84,7 +84,7 @@ test_expect_success 'output to keep user entertained during multi-pick' ' git reset --hard first && test_tick && git cherry-pick first..fourth >actual && - sed -e "s/$_x05[0-9a-f][0-9a-f]/OBJID/" <actual >actual.fuzzy && + sed -e "s/ [0-9a-f]*\\]/ OBJID]/" <actual >actual.fuzzy && test_line_count -ge 3 actual.fuzzy && test_cmp expected actual.fuzzy ' @@ -122,7 +122,7 @@ test_expect_success 'output during multi-pick indicates merge strategy' ' git reset --hard first && test_tick && git cherry-pick --strategy resolve first..fourth >actual && - sed -e "s/$_x05[0-9a-f][0-9a-f]/OBJID/" <actual >actual.fuzzy && + sed -e "s/ [0-9a-f]*\\]/ OBJID]/" <actual >actual.fuzzy && test_cmp expected actual.fuzzy ' diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh index 35a2f62392..ec65081e17 100755 --- a/t/t6006-rev-list-format.sh +++ b/t/t6006-rev-list-format.sh @@ -453,16 +453,18 @@ test_expect_success 'add SP before non-empty (2)' ' ' test_expect_success '--abbrev' ' - echo SHORT SHORT SHORT >expect2 && echo LONG LONG LONG >expect3 && git log -1 --format="%h %h %h" HEAD >actual1 && git log -1 --abbrev=5 --format="%h %h %h" HEAD >actual2 && git log -1 --abbrev=5 --format="%H %H %H" HEAD >actual3 && - sed -e "s/$OID_REGEX/LONG/g" -e "s/$_x05/SHORT/g" <actual2 >fuzzy2 && - sed -e "s/$OID_REGEX/LONG/g" -e "s/$_x05/SHORT/g" <actual3 >fuzzy3 && - test_cmp expect2 fuzzy2 && + sed -e "s/$OID_REGEX/LONG/g" <actual3 >fuzzy3 && + test_file_size actual2 >expect && + # 3*5 SHAs + 3 separating spaces + echo 18 >actual && + test_cmp expect actual && test_cmp expect3 fuzzy3 && - ! test_cmp actual1 actual2 + ! test_cmp actual1 actual2 && + ! test_cmp actual2 actual3 ' test_expect_success '%H is not affected by --abbrev-commit' ' diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh index 1cbc9715a8..fec5a7b2dd 100755 --- a/t/t7600-merge.sh +++ b/t/t7600-merge.sh @@ -189,7 +189,7 @@ test_expect_success 'merge c0 with c1' ' verify_head "$c1" && git reflog -1 >reflog.actual && - sed "s/$_x05[0-9a-f]*/OBJID/g" reflog.actual >reflog.fuzzy && + sed "s/^[0-9a-f]*/OBJID/" <reflog.actual >reflog.fuzzy && test_cmp reflog.expected reflog.fuzzy ' @@ -220,7 +220,7 @@ test_expect_success 'merge from unborn branch' ' verify_head "$c1" && git reflog -1 >reflog.actual && - sed "s/$_x05[0-9a-f][0-9a-f]/OBJID/g" reflog.actual >reflog.fuzzy && + sed "s/^[0-9a-f]*/OBJID/g" reflog.actual >reflog.fuzzy && test_cmp reflog.expected reflog.fuzzy ' diff --git a/t/test-lib.sh b/t/test-lib.sh index 4d5ba558d3..aeb4b2da1c 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -511,7 +511,7 @@ SQ=\' # when case-folding filenames u200c=$(printf '\342\200\214') -export _x05 LF u200c EMPTY_TREE EMPTY_BLOB ZERO_OID OID_REGEX +export LF u200c EMPTY_TREE EMPTY_BLOB ZERO_OID OID_REGEX # Each test should start with something like this, after copyright notices: # @@ -1380,10 +1380,6 @@ then fi fi -# Convenience -# A regexp to match 5 hexdigits -_x05='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]' - test_oid_init ZERO_OID=$(test_oid zero)
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. 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. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/t3101-ls-tree-dirname.sh | 2 +- t/t3508-cherry-pick-many-commits.sh | 4 ++-- t/t6006-rev-list-format.sh | 12 +++++++----- t/t7600-merge.sh | 4 ++-- t/test-lib.sh | 6 +----- 5 files changed, 13 insertions(+), 15 deletions(-)