| Message ID | 20240805235917.190699-3-abdobngad@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | t7004: Modernize the style | expand |
On Mon, Aug 5, 2024 at 8:00 PM AbdAlRahman Gad <abdobngad@gmail.com> wrote: > Split "test-tool ... | sed" pipeline into two commands to avoid losing > exit status from test-tool. > > Signed-off-by: AbdAlRahman Gad <abdobngad@gmail.com> > --- > diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh > @@ -97,7 +97,8 @@ test_expect_success 'creating a tag with --create-reflog should create reflog' ' > - test-tool ref-store main for-each-reflog-ent refs/tags/tag_with_reflog1 | sed -e "s/^.* //" >actual && > + test-tool ref-store main for-each-reflog-ent refs/tags/tag_with_reflog1 >actual.body && > + sed -e "s/^.* //" actual.body >actual && It's not just `test_tool` we care about; we also (importantly) don't want to see `git` itself upstream of a pipe, and there are many such instances remaining in this script. Here are some common examples: test $(git tag -l | wc -l) -eq 0 && git cat-file tag "$1" | sed -e "/BEGIN PGP/q" git tag -l | grep "^tag-one-line" >actual && forged=$(git cat-file tag ... | sed -e ... | git mktag) && git tag -l --no-sort "foo*" | sort >actual && By the way, these days, rather than: test $(git tag -l | wc -l) -eq 0 && we would say: test_stdout_line_count = 0 git tag -l && which nicely avoids placing `git` upstream of a pipe.
On 8/6/24 06:13, Eric Sunshine wrote: > On Mon, Aug 5, 2024 at 8:00 PM AbdAlRahman Gad <abdobngad@gmail.com> wrote: >> Split "test-tool ... | sed" pipeline into two commands to avoid losing >> exit status from test-tool. >> >> Signed-off-by: AbdAlRahman Gad <abdobngad@gmail.com> >> --- >> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh >> @@ -97,7 +97,8 @@ test_expect_success 'creating a tag with --create-reflog should create reflog' ' >> - test-tool ref-store main for-each-reflog-ent refs/tags/tag_with_reflog1 | sed -e "s/^.* //" >actual && >> + test-tool ref-store main for-each-reflog-ent refs/tags/tag_with_reflog1 >actual.body && >> + sed -e "s/^.* //" actual.body >actual && > > It's not just `test_tool` we care about; we also (importantly) don't > want to see `git` itself upstream of a pipe, and there are many such > instances remaining in this script. Here are some common examples: > > test $(git tag -l | wc -l) -eq 0 && > git cat-file tag "$1" | sed -e "/BEGIN PGP/q" > git tag -l | grep "^tag-one-line" >actual && > forged=$(git cat-file tag ... | sed -e ... | git mktag) && > git tag -l --no-sort "foo*" | sort >actual && > > By the way, these days, rather than: > > test $(git tag -l | wc -l) -eq 0 && > > we would say: > > test_stdout_line_count = 0 git tag -l && > > which nicely avoids placing `git` upstream of a pipe. Thanks for the review. Could this be done in a separate patch series? As modifying a patch in the beginning of a patch series requires lots of time to adapt the rest of the series to the change.
On Tue, Aug 6, 2024 at 4:38 AM AbdAlRahman Gad <abdobngad@gmail.com> wrote: > On 8/6/24 06:13, Eric Sunshine wrote: > > On Mon, Aug 5, 2024 at 8:00 PM AbdAlRahman Gad <abdobngad@gmail.com> wrote: > >> - test-tool ref-store main for-each-reflog-ent refs/tags/tag_with_reflog1 | sed -e "s/^.* //" >actual && > >> + test-tool ref-store main for-each-reflog-ent refs/tags/tag_with_reflog1 >actual.body && > >> + sed -e "s/^.* //" actual.body >actual && > > > > It's not just `test_tool` we care about; we also (importantly) don't > > want to see `git` itself upstream of a pipe, and there are many such > > instances remaining in this script. Here are some common examples: > > > > test $(git tag -l | wc -l) -eq 0 && > > git cat-file tag "$1" | sed -e "/BEGIN PGP/q" > > git tag -l | grep "^tag-one-line" >actual && > > forged=$(git cat-file tag ... | sed -e ... | git mktag) && > > git tag -l --no-sort "foo*" | sort >actual && > > Thanks for the review. Could this be done in a separate patch series? As > modifying a patch in the beginning of a patch series requires lots of > time to adapt the rest of the series to the change. As the person doing the work and submitting the patches, you choose how much effort to put in. If you feel that fixing all the "`git` upstream of a pipe" problems is better left for a future "modernization" series, then that's almost certainly fine. Since reviewers are trying to help you polish your patches, they usually prefer to review shorter series than longer ones anyhow. If you decide not to make these changes, though, be sure to explain your decision in the cover letter so that reviewers understand why such a cleanup is missing from the current series. Having said that, if you're not going to fix all the "`git` upstream of a pipe" cases, then you might consider dropping this patch from the current series since it is merely touching the "tip of the iceberg" by fixing only these two minor cases while leaving all the more significant cases unfixed. But other reviewers may feel differently. If you're worried about the ripple effect of making significant changes to this patch which sits near the beginning of the series, then one way to sidestep the issue is to relocate this patch to the end of the series. This is possible because, unlike many other patch series in which there is inherent order in the patches where each change must follow the preceding change, that is not the case with this patch series. The specific order of patches in this series is pretty much immaterial. So, moving this patch to the end of the series would cause only a tiny ripple since it touches only two small chunks of code, and once the patch is last in the series, then it becomes easy to apply all the other "`git` upstream of a pipe" fixes. But it's up to you if you want to do that extra work. As noted above, reviewers usually prefer a shorter simpler series over a longer more complex one, so relegating such fixes to a followup series is almost certainly acceptable. (But be sure to explain your decision in the cover letter.)
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index 3100a4c219..1e31f39646 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -97,7 +97,8 @@ test_expect_success 'creating a tag with --create-reflog should create reflog' ' test_when_finished "git tag -d tag_with_reflog1" && git tag --create-reflog tag_with_reflog1 && git reflog exists refs/tags/tag_with_reflog1 && - test-tool ref-store main for-each-reflog-ent refs/tags/tag_with_reflog1 | sed -e "s/^.* //" >actual && + test-tool ref-store main for-each-reflog-ent refs/tags/tag_with_reflog1 >actual.body && + sed -e "s/^.* //" actual.body >actual && test_cmp expected actual ' @@ -108,7 +109,8 @@ test_expect_success 'annotated tag with --create-reflog has correct message' ' test_when_finished "git tag -d tag_with_reflog2" && git tag -m "annotated tag" --create-reflog tag_with_reflog2 && git reflog exists refs/tags/tag_with_reflog2 && - test-tool ref-store main for-each-reflog-ent refs/tags/tag_with_reflog2 | sed -e "s/^.* //" >actual && + test-tool ref-store main for-each-reflog-ent refs/tags/tag_with_reflog2 >actual.body && + sed -e "s/^.* //" actual.body >actual && test_cmp expected actual '
Split "test-tool ... | sed" pipeline into two commands to avoid losing exit status from test-tool. Signed-off-by: AbdAlRahman Gad <abdobngad@gmail.com> --- t/t7004-tag.sh | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)