Re: [PATCH v2 16/22] t/t3*: merge a "grep | awk" pipeline

[Taylor Blau <me@ttaylorr.com>]

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