| Message ID | 20240315194620.10713-17-dev+git@drbeat.li (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 67dd07e8af11534d8bd723747f9c0a91c530b6bc |
| Headers | show |
| Series | avoid redundant pipelines | expand |
On Fri, Mar 15, 2024 at 08:46:13PM +0100, Beat Bolli wrote: > Signed-off-by: Beat Bolli <dev+git@drbeat.li> > --- > t/t3920-crlf-messages.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/t/t3920-crlf-messages.sh b/t/t3920-crlf-messages.sh > index 5eed640a6825..50ae222f0842 100755 > --- a/t/t3920-crlf-messages.sh > +++ b/t/t3920-crlf-messages.sh > @@ -97,7 +97,7 @@ test_expect_success 'branch: --verbose works with messages using CRLF' ' > git branch -v >tmp && > # Remove first two columns, and the line for the currently checked out branch > current=$(git branch --show-current) && > - grep -v $current <tmp | awk "{\$1=\$2=\"\"}1" >actual && > + awk "/$current/ { next } { \$1 = \$2 = \"\" } 1" <tmp >actual && I think that using `next` here is fine to ignore lines that match `$current`, but the canonical approach would probably be using the `!` operator instead to negate the match, like so: awk "!/$current/ { \$1 = \$2 = \"\" } 1" <tmp >actual && Not worth a reroll, of course, just something that I noticed while reading. Thanks, Taylor
On 16.03.24 02:49, Taylor Blau wrote: > On Fri, Mar 15, 2024 at 08:46:13PM +0100, Beat Bolli wrote: >> Signed-off-by: Beat Bolli <dev+git@drbeat.li> >> --- >> t/t3920-crlf-messages.sh | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/t/t3920-crlf-messages.sh b/t/t3920-crlf-messages.sh >> index 5eed640a6825..50ae222f0842 100755 >> --- a/t/t3920-crlf-messages.sh >> +++ b/t/t3920-crlf-messages.sh >> @@ -97,7 +97,7 @@ test_expect_success 'branch: --verbose works with messages using CRLF' ' >> git branch -v >tmp && >> # Remove first two columns, and the line for the currently checked out branch >> current=$(git branch --show-current) && >> - grep -v $current <tmp | awk "{\$1=\$2=\"\"}1" >actual && >> + awk "/$current/ { next } { \$1 = \$2 = \"\" } 1" <tmp >actual && > > I think that using `next` here is fine to ignore lines that match > `$current`, but the canonical approach would probably be using the > `!` operator instead to negate the match, like so: > > awk "!/$current/ { \$1 = \$2 = \"\" } 1" <tmp >actual && > > Not worth a reroll, of course, just something that I noticed while > reading. Except it's not the same :-) This was actually my first try, but then I realized that awk continues to evaluate patterns and actions until the end of the script. The "1" at the end is the "always true" pattern that causes the default action "print $0" to run. So the "next" is needed to discard the current line. Having said this, awk "!/$current/ { \$1 = \$2 = \"\"; print \$0 }" <tmp >actual && would work, and it would also remove the obscure flow detailed above. Regards, Beat
On Sat, Mar 16, 2024 at 11:09:47AM +0100, Beat Bolli wrote: > On 16.03.24 02:49, Taylor Blau wrote: > > On Fri, Mar 15, 2024 at 08:46:13PM +0100, Beat Bolli wrote: > > > Signed-off-by: Beat Bolli <dev+git@drbeat.li> > > > --- > > > t/t3920-crlf-messages.sh | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/t/t3920-crlf-messages.sh b/t/t3920-crlf-messages.sh > > > index 5eed640a6825..50ae222f0842 100755 > > > --- a/t/t3920-crlf-messages.sh > > > +++ b/t/t3920-crlf-messages.sh > > > @@ -97,7 +97,7 @@ test_expect_success 'branch: --verbose works with messages using CRLF' ' > > > git branch -v >tmp && > > > # Remove first two columns, and the line for the currently checked out branch > > > current=$(git branch --show-current) && > > > - grep -v $current <tmp | awk "{\$1=\$2=\"\"}1" >actual && > > > + awk "/$current/ { next } { \$1 = \$2 = \"\" } 1" <tmp >actual && > > > > I think that using `next` here is fine to ignore lines that match > > `$current`, but the canonical approach would probably be using the > > `!` operator instead to negate the match, like so: > > > > awk "!/$current/ { \$1 = \$2 = \"\" } 1" <tmp >actual && > > > > Not worth a reroll, of course, just something that I noticed while > > reading. > > Except it's not the same :-) This was actually my first try, but then I > realized that awk continues to evaluate patterns and actions until the end > of the script. The "1" at the end is the "always true" pattern that causes > the default action "print $0" to run. > > So the "next" is needed to discard the current line. > > Having said this, > > awk "!/$current/ { \$1 = \$2 = \"\"; print \$0 }" <tmp >actual && > > would work, and it would also remove the obscure flow detailed above. > Ah. Thanks for the explanation. These details would not hurt to have in a commit message, but I think that this change is fine as-is. Those curious enough can likely find this thread on the list for this particular instance. But these sort of less-than-trivial details are exactly the sorts of things we like to capture in a well-written patch message. Thanks, Taylor
diff --git a/t/t3920-crlf-messages.sh b/t/t3920-crlf-messages.sh index 5eed640a6825..50ae222f0842 100755 --- a/t/t3920-crlf-messages.sh +++ b/t/t3920-crlf-messages.sh @@ -97,7 +97,7 @@ test_expect_success 'branch: --verbose works with messages using CRLF' ' git branch -v >tmp && # Remove first two columns, and the line for the currently checked out branch current=$(git branch --show-current) && - grep -v $current <tmp | awk "{\$1=\$2=\"\"}1" >actual && + awk "/$current/ { next } { \$1 = \$2 = \"\" } 1" <tmp >actual && test_cmp expect actual '
Signed-off-by: Beat Bolli <dev+git@drbeat.li> --- t/t3920-crlf-messages.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)