| Message ID | 20250202120926.322417-1-ayu.chandekar@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 7c1d34fe5d1229362f2c3ecf2d493167a1f555a2 |
| Headers | show |
| Series | t6423: fix suppression of Git���s exit code in tests | expand |
On Sun, 2 Feb 2025 at 17:40, ayu-ch <ayu.chandekar@gmail.com> wrote: > > From: Ayush Chandekar <ayu.chandekar@gmail.com> > > Some test in t6423 supress Git's exit code, which can cause test s/supress/suppress > failures go unnoticed. Specifically using git <subcommand> | > <other-command> masks potential failures of the Git command. > > This commit ensures that Git's exit status is correctly propogated by: > - Avoiding pipes that suppress exit codes. s/propogated/propagated The commit message should be in imperative mood (cf. Documentation/SubmittingPatches) > > Signed-off-by: Ayush Chandekar <ayu.chandekar@gmail.com> > --- > t/t6423-merge-rename-directories.sh | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh > index 88d1cf2cde..94080c65d1 100755 > --- a/t/t6423-merge-rename-directories.sh > +++ b/t/t6423-merge-rename-directories.sh > @@ -5071,7 +5071,8 @@ test_expect_success '12i: Directory rename causes rename-to-self' ' > test_path_is_file source/bar && > test_path_is_file source/baz && > > - git ls-files | uniq >tracked && > + git ls-files >actual && > + uniq <actual >tracked && > test_line_count = 3 tracked && > > git status --porcelain -uno >actual && > @@ -5129,7 +5130,8 @@ test_expect_success '12j: Directory rename to root causes rename-to-self' ' > test_path_is_file bar && > test_path_is_file baz && > > - git ls-files | uniq >tracked && > + git ls-files >actual && > + uniq <actual >tracked && > test_line_count = 3 tracked && > > git status --porcelain -uno >actual && > @@ -5187,7 +5189,8 @@ test_expect_success '12k: Directory rename with sibling causes rename-to-self' ' > test_path_is_file dirA/bar && > test_path_is_file dirA/baz && > > - git ls-files | uniq >tracked && > + git ls-files >actual && > + uniq <actual >tracked && > test_line_count = 3 tracked && > > git status --porcelain -uno >actual && > -- > 2.48.GIT > > It should’ve been v2 of the patch you sent earlier [1] (cf. Documentation/MyFirstConribution), but otherwise, it looks good. [1]: https://lore.kernel.org/git/20250201004556.930220-1-ayu.chandekar@gmail.com/ Meet
On Sun, Feb 2, 2025 at 7:09 AM ayu-ch <ayu.chandekar@gmail.com> wrote: > Some test in t6423 supress Git's exit code, which can cause test > failures go unnoticed. Specifically using git <subcommand> | > <other-command> masks potential failures of the Git command. > > This commit ensures that Git's exit status is correctly propogated by: > - Avoiding pipes that suppress exit codes. > > Signed-off-by: Ayush Chandekar <ayu.chandekar@gmail.com> > --- > diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh > @@ -5071,7 +5071,8 @@ test_expect_success '12i: Directory rename causes rename-to-self' ' > - git ls-files | uniq >tracked && > + git ls-files >actual && > + uniq <actual >tracked && I was curious if the project has a preference between `uniq filename` and `uniq <filename`, but apparently we haven't: % git grep 'uniq <' -- t | wc -l 2 git grep 'uniq [a-z0-9]' -- t | wc -l 2 Though there does seem to be a global preference in the project to specify the filename directly to the command rather than redirecting from stdin. For instance: % git grep 'sort <' -- t | wc -l 54 % git grep 'sort [a-z0-9]' -- t | wc -l 140 In any case, what you have here is probably fine, so no need to reroll just for this.
Eric Sunshine <sunshine@sunshineco.com> writes: > I was curious if the project has a preference between `uniq filename` > and `uniq <filename`, but apparently we haven't: > > % git grep 'uniq <' -- t | wc -l > 2 > git grep 'uniq [a-z0-9]' -- t | wc -l > 2 > > Though there does seem to be a global preference in the project to > specify the filename directly to the command rather than redirecting > from stdin. For instance: > > % git grep 'sort <' -- t | wc -l > 54 > % git grep 'sort [a-z0-9]' -- t | wc -l > 140 Have you inspected the hits from these grep runs? $ git grep -c 'sort [a-z0-9]' -- t/t7004-tag.sh t/t7004-tag.sh:17 Among 17 of them, 15 are on test titles. $ git grep -c '^test_expect_[sf].*sort [a-z0-9]' -- t/t7004-tag.sh t/t7004-tag.sh:15 So the above numbers are totally unreliable as a guide, I am afraid. It is probably better to use sort/uniq without input redirection because your $ sort/uniq input >output can be easily extended to $ sort/uniq input-a input-b input-c >output but $ sort/uniq <input >output cannot be extended the same way, and you'd end up doing nonsense pipe like this: $ cat input-a input-b input-c | sort >output which is a no-no. In reality, however, we are not all that logical. $ git grep -e '^[ ]*sort [a-z0-9][-a-z0-9]* ' -- t | wc -l 46 $ git grep -e '^[ ]*sort <' -- t | wc -l 51 with "s/sort/uniq/", the numbers are 0 vs 1. There are a handful of sort invocations that take their input from redirected <<HEREDOC included in the latter number, but the overall picture does not change with them excluded.
Do you see any other changes needed in this patch? Let me know if there's anything you want me to adjust, especially in my commit message. Since my previous attempt wasn't very suitable. Regards, Ayush
Ayush Chandekar <ayu.chandekar@gmail.com> writes: > Do you see any other changes needed in this patch? Let me know if there's > anything you want me to adjust, especially in my commit message. Since my > previous attempt wasn't very suitable. If I were to change something, there are two minor things, but they are so minor that I'd be OK without these changes. If this is supposed to be a part of microproject exchange (sorry, I lost track), then I am also OK to do the second (and hopefully final) iteration to give us a chance to practice. If I were you and I chose to iterate one more time, I'd rephrase this This commit ensures that Git's exit status is correctly propogated by: - Avoiding pipes that suppress exit codes. to more like Instead of placing a git command on the upstream side of a pipe, redirect its output to a file and process the file contents in two separate steps to avoid losing the exit status. Also I'd not redirect into "uniq", i.e. instead of uniq <actual >tracked && I'd write uniq actual >tracked && but as discussed with Eric, this "better style" is not followed by existing code.
diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh index 88d1cf2cde..94080c65d1 100755 --- a/t/t6423-merge-rename-directories.sh +++ b/t/t6423-merge-rename-directories.sh @@ -5071,7 +5071,8 @@ test_expect_success '12i: Directory rename causes rename-to-self' ' test_path_is_file source/bar && test_path_is_file source/baz && - git ls-files | uniq >tracked && + git ls-files >actual && + uniq <actual >tracked && test_line_count = 3 tracked && git status --porcelain -uno >actual && @@ -5129,7 +5130,8 @@ test_expect_success '12j: Directory rename to root causes rename-to-self' ' test_path_is_file bar && test_path_is_file baz && - git ls-files | uniq >tracked && + git ls-files >actual && + uniq <actual >tracked && test_line_count = 3 tracked && git status --porcelain -uno >actual && @@ -5187,7 +5189,8 @@ test_expect_success '12k: Directory rename with sibling causes rename-to-self' ' test_path_is_file dirA/bar && test_path_is_file dirA/baz && - git ls-files | uniq >tracked && + git ls-files >actual && + uniq <actual >tracked && test_line_count = 3 tracked && git status --porcelain -uno >actual &&