| Message ID | patch-v2-2.8-345a667d5bb-20221202T000227Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tests: fix ignored & hidden exit codes | expand |
Am 02.12.2022 um 01:06 schrieb Ævar Arnfjörð Bjarmason: > Don't hide the exit code from the "git checkout" we run to checkout > our attributes file. > > This fixes cases where we'd have e.g. missed memory leaks under > SANITIZE=leak, this code doesn't leak (the relevant "git checkout" > leak has been fixed), but in a past version of git we'd continue past > this failure under SANITIZE=leak when these invocations had errored > out, even under "--immediate". > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > t/t0027-auto-crlf.sh | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh > index a94ac1eae37..574344a99db 100755 > --- a/t/t0027-auto-crlf.sh > +++ b/t/t0027-auto-crlf.sh > @@ -294,11 +294,17 @@ checkout_files () { > pfx=eol_${ceol}_crlf_${crlf}_attr_${attr}_ && > for f in LF CRLF LF_mix_CR CRLF_mix_LF LF_nul > do > - rm crlf_false_attr__$f.txt && > - if test -z "$ceol"; then > - git checkout -- crlf_false_attr__$f.txt > + if test -z "$ceol" > + then > + test_expect_success "setup $f checkout" ' > + rm crlf_false_attr__$f.txt && > + git checkout -- crlf_false_attr__$f.txt > + ' > else > - git -c core.eol=$ceol checkout -- crlf_false_attr__$f.txt > + test_expect_success "setup $f checkout with core.eol=$ceol" ' > + rm crlf_false_attr__$f.txt && > + git -c core.eol=$ceol checkout -- crlf_false_attr__$f.txt > + ' That adds five test_expect_success calls. Wouldn't one suffice, for the whole for loop, and a "|| return 1"? One line above the context there's a "git config" call that should also be covered, right? Side note: The checkout commands only differ in their -c parameter. They could be unified like this, which might simplify handling their return code: git ${ceol:+-c core.eol=$ceol} checkout -- crlf_false_attr__$f.txt > fi > done >
On Thu, Dec 1, 2022 at 8:02 PM René Scharfe <l.s.r@web.de> wrote: > Am 02.12.2022 um 01:06 schrieb Ævar Arnfjörð Bjarmason: > > Don't hide the exit code from the "git checkout" we run to checkout > > our attributes file. > > @@ -294,11 +294,17 @@ checkout_files () { > > pfx=eol_${ceol}_crlf_${crlf}_attr_${attr}_ && > > for f in LF CRLF LF_mix_CR CRLF_mix_LF LF_nul > > do > > - rm crlf_false_attr__$f.txt && > > - if test -z "$ceol"; then > > - git checkout -- crlf_false_attr__$f.txt > > + if test -z "$ceol" > > + then > > + test_expect_success "setup $f checkout" ' > > + rm crlf_false_attr__$f.txt && > > + git checkout -- crlf_false_attr__$f.txt > > + ' > > else > > - git -c core.eol=$ceol checkout -- crlf_false_attr__$f.txt > > + test_expect_success "setup $f checkout with core.eol=$ceol" ' > > + rm crlf_false_attr__$f.txt && > > + git -c core.eol=$ceol checkout -- crlf_false_attr__$f.txt > > + ' > > That adds five test_expect_success calls. Wouldn't one suffice, for the > whole for loop, and a "|| return 1"? Seems like a reasonable idea. > One line above the context there's a "git config" call that should also > be covered, right? Also, the call to create_gitattributes() just above that line is in the function's &&-chain, which implies that it too should be covered.
On Fri, Dec 02, 2022 at 02:02:55AM +0100, René Scharfe wrote: > Am 02.12.2022 um 01:06 schrieb Ævar Arnfjörð Bjarmason: > > Don't hide the exit code from the "git checkout" we run to checkout > > our attributes file. > > > > This fixes cases where we'd have e.g. missed memory leaks under > > SANITIZE=leak, this code doesn't leak (the relevant "git checkout" > > leak has been fixed), but in a past version of git we'd continue past > > this failure under SANITIZE=leak when these invocations had errored > > out, even under "--immediate". > > > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > > --- > > t/t0027-auto-crlf.sh | 14 ++++++++++---- > > 1 file changed, 10 insertions(+), 4 deletions(-) > > > > diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh > > index a94ac1eae37..574344a99db 100755 > > --- a/t/t0027-auto-crlf.sh > > +++ b/t/t0027-auto-crlf.sh > > @@ -294,11 +294,17 @@ checkout_files () { > > pfx=eol_${ceol}_crlf_${crlf}_attr_${attr}_ && > > for f in LF CRLF LF_mix_CR CRLF_mix_LF LF_nul > > do > > - rm crlf_false_attr__$f.txt && > > - if test -z "$ceol"; then > > - git checkout -- crlf_false_attr__$f.txt > > + if test -z "$ceol" > > + then > > + test_expect_success "setup $f checkout" ' > > + rm crlf_false_attr__$f.txt && > > + git checkout -- crlf_false_attr__$f.txt > > + ' > > else > > - git -c core.eol=$ceol checkout -- crlf_false_attr__$f.txt > > + test_expect_success "setup $f checkout with core.eol=$ceol" ' > > + rm crlf_false_attr__$f.txt && > > + git -c core.eol=$ceol checkout -- crlf_false_attr__$f.txt > > + ' > > That adds five test_expect_success calls. Wouldn't one suffice, for the > whole for loop, and a "|| return 1"? > > One line above the context there's a "git config" call that should also > be covered, right? > > Side note: The checkout commands only differ in their -c parameter. > They could be unified like this, which might simplify handling their > return code: > > git ${ceol:+-c core.eol=$ceol} checkout -- crlf_false_attr__$f.txt > > > fi > > done > > That is a nice one. (I learned about the ${:+} yesterday). Is it supported by all the shells/os combinations ? If not, we can still extract the if: if test -z "$ceol"; then CEOL="-c core.eol=$ceol" else CEOL="" fi git $CEOL checkout -- crlf_false_attr__$f.txt && The original reasoning (for not looking at the return code) was that if the checkout fails, the test suite will fail further down anyway. But checking for failures early is a good thing. Thanks for cleaning up my mess.
On Fri, Dec 2, 2022 at 12:59 AM Torsten Bögershausen <tboegi@web.de> wrote: > On Fri, Dec 02, 2022 at 02:02:55AM +0100, René Scharfe wrote: > > Side note: The checkout commands only differ in their -c parameter. > > They could be unified like this, which might simplify handling their > > return code: > > > > git ${ceol:+-c core.eol=$ceol} checkout -- crlf_false_attr__$f.txt > > That is a nice one. (I learned about the ${:+} yesterday). > Is it supported by all the shells/os combinations ? ${:+} is well supported. It's POSIX. It's used heavily in the Git test suite already. So, it's safe to use.
diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh index a94ac1eae37..574344a99db 100755 --- a/t/t0027-auto-crlf.sh +++ b/t/t0027-auto-crlf.sh @@ -294,11 +294,17 @@ checkout_files () { pfx=eol_${ceol}_crlf_${crlf}_attr_${attr}_ && for f in LF CRLF LF_mix_CR CRLF_mix_LF LF_nul do - rm crlf_false_attr__$f.txt && - if test -z "$ceol"; then - git checkout -- crlf_false_attr__$f.txt + if test -z "$ceol" + then + test_expect_success "setup $f checkout" ' + rm crlf_false_attr__$f.txt && + git checkout -- crlf_false_attr__$f.txt + ' else - git -c core.eol=$ceol checkout -- crlf_false_attr__$f.txt + test_expect_success "setup $f checkout with core.eol=$ceol" ' + rm crlf_false_attr__$f.txt && + git -c core.eol=$ceol checkout -- crlf_false_attr__$f.txt + ' fi done
Don't hide the exit code from the "git checkout" we run to checkout our attributes file. This fixes cases where we'd have e.g. missed memory leaks under SANITIZE=leak, this code doesn't leak (the relevant "git checkout" leak has been fixed), but in a past version of git we'd continue past this failure under SANITIZE=leak when these invocations had errored out, even under "--immediate". Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/t0027-auto-crlf.sh | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)