| Message ID | 179c67caec0f8123d09455cb78532419166e1b9e.1582447606.git.martin.agren@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] t4117: check for files using `test_path_is_file` | expand |
Martin Ågren <martin.agren@gmail.com> writes: > We `cat` files, but don't inspect or grab the contents in any way. > Unlike in an earlier commit, there is no reason to suspect that these > files could be missing, so `cat`-ing them is just wasted effort. > ... > rm -rf repo-cloned && > test_must_fail git clone repo repo-cloned 2>git-stderr.log && > - cat git-stderr.log && > grep "error: .missing-delay\.a. was not filtered properly" git-stderr.log Loss of cat does not change if git-stderr.log file went missing for whatever reason (e.g. typo while updating the test), just like 2/3, so the debugging cruft can safely be removed. > - $RUN for-each-reflog-ent HEAD >actual && cat actual && > + $RUN for-each-reflog-ent HEAD >actual && > head -n1 actual | grep first && Very similar, as lack of 'actual' for whatever reason will manifest in a failure for "grep" from finding 'first' anyway. I sort of sympathetic to your desire to have two classes and separate/distribute them into 2/3 and 3/3, but to me the line between two classes looks like it was drawn along a wrong axis. The proposed log message for this step hints that the criteria for tests to be thrown into this category is that the output of 'cat' is not used in any useful way, and the input of 'cat' is known to exist (i.e. so "cat fails if the file is missing" is not part of the test), but that applies to the one instance singled out in 2/3 (i.e. the file in question, kwdelfile.c, is created with shell redirection, just like "actual" file above is). > diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh > index 02478bc4ec..d09eff503c 100755 > --- a/t/t1450-fsck.sh > +++ b/t/t1450-fsck.sh > @@ -141,7 +141,6 @@ test_expect_success 'email without @ is okay' ' > git update-ref refs/heads/bogus "$new" && > test_when_finished "git update-ref -d refs/heads/bogus" && > git fsck 2>out && > - cat out && > ! grep "commit $new" out > ' This one on the other hand *DOES* rely on 'out' being created; we do not want to take the failing 'grep' as a sign of success if it is because 'out' is missing. > diff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh > index 2242cd098e..a30b7ca6bc 100755 > --- a/t/t2107-update-index-basic.sh > +++ b/t/t2107-update-index-basic.sh > @@ -9,7 +9,6 @@ Tests for command-line parsing and basic operation. > > test_expect_success 'update-index --nonsense fails' ' > test_must_fail git update-index --nonsense 2>msg && > - cat msg && > test -s msg > ' This one does not. "test -s msg" on non-existent msg will fail, so this is closer to category 2/3. So, I am OK to have two patches that catch two classes, but the division between 2/3 and 3/3 in this series does not look the right one. I am also OK to have a single patch with updated log message, saying "removal of 'cat <file>' may miss a failure mode that <file> did not get created, which would have been caught as a test failure in the original, but the <file>s used by cats removed in this patch are either impossible to be missing (because a preceding step in the test created it, or the &&-cascade would have failed if it failed to create the file), or followed by another step in the test that would fail if the file is missing (e.g. running grep on the file), so it is safe to drop these cats", or something like that.
On Mon, 24 Feb 2020 at 20:33, Junio C Hamano <gitster@pobox.com> wrote: > > Martin Ågren <martin.agren@gmail.com> writes: > > > git fsck 2>out && > > - cat out && > > ! grep "commit $new" out > > ' > > This one on the other hand *DOES* rely on 'out' being created; we do > not want to take the failing 'grep' as a sign of success if it is > because 'out' is missing. > > > test_must_fail git update-index --nonsense 2>msg && > > - cat msg && > > test -s msg > > ' > > This one does not. "test -s msg" on non-existent msg will fail, so > this is closer to category 2/3. > > So, I am OK to have two patches that catch two classes, but the > division between 2/3 and 3/3 in this series does not look the right > one. Heh. You don't want to know how long I waffled on whether to split the one hunk out to 2/3 and make the rest 3/3 vs. having a slightly larger 2/2. For the first patch 1/3, if we lose the cat entirely, we risk bugs in *git* being hidden. For the hunk in patch 2/3, I first thought it was in the same category, before I realized that kwdelfile.c disappearing would be a bug in *p4* as opposed to *git p4*. Since we're not in the business of testing/verifying other people's software, we can afford to drop that call entirely. At one point, I had this in the commit message, but in the end I figured one reason for the removal was enough and just kept the "we'll soon grep" argument. I realize now that the line between 2/3 and 3/3 is blurry. FWIW, for patch 3/3 my reasoning was that for the similar concern about the file not existing, we'd depend on the shell messing up the redirection quite badly and not creating a file at all, yet continuing with the && cascade. Which seemed like a pretty crazy bug. And again, that shouldn't be our worry. (I see now that there's a case in 3/3 where a buggy test_cmp could delete "actual" and we'd fail to notice after this commit. That probably also sorts under pathological bugs...) > I am also OK to have a single patch with updated log message, saying > "removal of 'cat <file>' may miss a failure mode that <file> did not > get created, which would have been caught as a test failure in the > original, but the <file>s used by cats removed in this patch are > either impossible to be missing (because a preceding step in the > test created it, or the &&-cascade would have failed if it failed to > create the file), or followed by another step in the test that would > fail if the file is missing (e.g. running grep on the file), so it is > safe to drop these cats", or something like that. Let me re-roll with 1/2 (=1/3) and 2/2 (=1/3+2/3). Thanks for a review. Martin
Martin Ågren <martin.agren@gmail.com> writes: > For the first patch 1/3, if we lose the cat entirely, we risk bugs in > *git* being hidden. Yes, as Peff already said, the change to explicitly test if the file exists makes perfect sense. > For the hunk in patch 2/3, I first thought it was in the same > category, before I realized that kwdelfile.c disappearing would be > a bug in *p4* as opposed to *git p4*. Since we're not in the > business of testing/verifying other people's software, we can > afford to drop that call entirely. It is ">kwdelfile.c" shell redirection that creates that file. I agree with you that preparing for a bug that "p4 add -t ktext kwdelfile.c" or "p4 submit" removes the file is a little too much.
diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh index 6c6d77b51a..dc664da551 100755 --- a/t/t0021-conversion.sh +++ b/t/t0021-conversion.sh @@ -795,7 +795,6 @@ test_expect_success PERL 'missing file in delayed checkout' ' rm -rf repo-cloned && test_must_fail git clone repo repo-cloned 2>git-stderr.log && - cat git-stderr.log && grep "error: .missing-delay\.a. was not filtered properly" git-stderr.log ' diff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh index d199d872fb..36b7ef5046 100755 --- a/t/t1406-submodule-ref-store.sh +++ b/t/t1406-submodule-ref-store.sh @@ -75,7 +75,7 @@ test_expect_success 'for_each_reflog()' ' ' test_expect_success 'for_each_reflog_ent()' ' - $RUN for-each-reflog-ent HEAD >actual && cat actual && + $RUN for-each-reflog-ent HEAD >actual && head -n1 actual | grep first && tail -n2 actual | head -n1 | grep master.to.new ' diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh index 02478bc4ec..d09eff503c 100755 --- a/t/t1450-fsck.sh +++ b/t/t1450-fsck.sh @@ -141,7 +141,6 @@ test_expect_success 'email without @ is okay' ' git update-ref refs/heads/bogus "$new" && test_when_finished "git update-ref -d refs/heads/bogus" && git fsck 2>out && - cat out && ! grep "commit $new" out ' diff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh index 2242cd098e..a30b7ca6bc 100755 --- a/t/t2107-update-index-basic.sh +++ b/t/t2107-update-index-basic.sh @@ -9,7 +9,6 @@ Tests for command-line parsing and basic operation. test_expect_success 'update-index --nonsense fails' ' test_must_fail git update-index --nonsense 2>msg && - cat msg && test -s msg ' diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh index 318b5bce7e..4a08000713 100755 --- a/t/t3007-ls-files-recurse-submodules.sh +++ b/t/t3007-ls-files-recurse-submodules.sh @@ -130,7 +130,6 @@ test_expect_success '--recurse-submodules and pathspecs setup' ' git ls-files --recurse-submodules >actual && test_cmp expect actual && - cat actual && git ls-files --recurse-submodules "*" >actual && test_cmp expect actual ' diff --git a/t/t5150-request-pull.sh b/t/t5150-request-pull.sh index 1ad4ecc29a..c1811ea0f4 100755 --- a/t/t5150-request-pull.sh +++ b/t/t5150-request-pull.sh @@ -150,7 +150,6 @@ test_expect_success 'pull request after push' ' git request-pull initial origin master:for-upstream >../request ) && sed -nf read-request.sed <request >digest && - cat digest && { read task && read repository && @@ -179,7 +178,6 @@ test_expect_success 'request asks HEAD to be pulled' ' git request-pull initial "$downstream_url" >../request ) && sed -nf read-request.sed <request >digest && - cat digest && { read task && read repository && diff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh index 2a8c449661..5d8f401d8e 100755 --- a/t/t5409-colorize-remote-messages.sh +++ b/t/t5409-colorize-remote-messages.sh @@ -56,14 +56,13 @@ test_expect_success 'short line' ' test_expect_success 'case-insensitive' ' git --git-dir child/.git -c color.remote=always push -f origin HEAD:refs/heads/case-insensitive 2>output && - cat output && test_decode_color <output >decoded && grep "<BOLD;RED>error<RESET>: error" decoded && grep "<BOLD;RED>ERROR<RESET>: also highlighted" decoded ' test_expect_success 'leading space' ' - git --git-dir child/.git -c color.remote=always push -f origin HEAD:refs/heads/leading-space 2>output && cat output && + git --git-dir child/.git -c color.remote=always push -f origin HEAD:refs/heads/leading-space 2>output && test_decode_color <output >decoded && grep " <BOLD;RED>error<RESET>: leading space" decoded ' diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 5f8f1c287f..4d3a55caaf 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -378,7 +378,6 @@ test_expect_success 'fetch from GIT URL with a non-applying branch.<name>.merge # the strange name is: a\!'b test_expect_success 'quoting of a strangely named repo' ' test_must_fail git fetch "a\\!'\''b" > result 2>&1 && - cat result && grep "fatal: '\''a\\\\!'\''b'\''" result ' diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh index 1c5fb1d1f8..9130b887d2 100755 --- a/t/t8003-blame-corner-cases.sh +++ b/t/t8003-blame-corner-cases.sh @@ -173,7 +173,6 @@ test_expect_success 'blame during cherry-pick with file rename conflict' ' git show HEAD@{1}:rodent > rodent && git add rodent && git blame -f -C -C1 rodent | sed -e "$pick_fc" >current && - cat current && cat >expected <<-\EOF && mouse-Initial mouse-Second diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh index ae9950a9c2..3e41c58a13 100755 --- a/t/t9300-fast-import.sh +++ b/t/t9300-fast-import.sh @@ -1047,7 +1047,6 @@ test_expect_success 'M: rename root to subdirectory' ' EOF git fast-import <input && git diff-tree -M -r M4^ M4 >actual && - cat actual && compare_diff_raw expect actual ' diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh index 5856563068..c98c1dfc23 100755 --- a/t/t9800-git-p4-basic.sh +++ b/t/t9800-git-p4-basic.sh @@ -202,7 +202,6 @@ test_expect_success 'exit when p4 fails to produce marshaled output' ' export PATH && test_expect_code 1 git p4 clone --dest="$git" //depot >errs 2>&1 ) && - cat errs && test_i18ngrep ! Traceback errs '
We `cat` files, but don't inspect or grab the contents in any way. Unlike in an earlier commit, there is no reason to suspect that these files could be missing, so `cat`-ing them is just wasted effort. Signed-off-by: Martin Ågren <martin.agren@gmail.com> --- t/t0021-conversion.sh | 1 - t/t1406-submodule-ref-store.sh | 2 +- t/t1450-fsck.sh | 1 - t/t2107-update-index-basic.sh | 1 - t/t3007-ls-files-recurse-submodules.sh | 1 - t/t5150-request-pull.sh | 2 -- t/t5409-colorize-remote-messages.sh | 3 +-- t/t5510-fetch.sh | 1 - t/t8003-blame-corner-cases.sh | 1 - t/t9300-fast-import.sh | 1 - t/t9800-git-p4-basic.sh | 1 - 11 files changed, 2 insertions(+), 13 deletions(-)