Re: [PATCH v5 22/26] t7700: consolidate code into test_no_missing_in_packs()

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 2, 2019, 22:53 UTC
Message-ID: <xmqqo8wqwd9m.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20191202205037.GA43606@generichostname>

Denton Liu <liu.denton@gmail.com> writes:

Show 12 quoted lines

> Hi Junio,
> ...
>> "grep" is better only if the original were
>> 
>>     $ sed -n -e '/required match/p'
>> 
>> but everybody would write it with grep to begin with ;-)
>
> This was what I was intending. It was originally written like the above
> and it made sense to convert it to use grep. I guess "filter lines" in
> my commit message is a little bit vague. Could we change this to "filter
> matching lines" perhaps?

Ah, I see. I somehow thought that some of the "sed" invocation in the original version were doing "find lines and filter its contents" (i.e. "-n -e 's/find/munge/p'"), but all three of them are just "find lines" (i.e. "-n -e '/find/p'"). So I think the change made by the patch is OK.

I think I was reacting to the output of "grep" being piped to "cut". IOW, the original

	... | sed -n -e '/find/p' |
	while read sha1 garbage
	do
		... use sha1 ...

were rewritten to

	... >raw &&
	grep -e 'find' raw | cut -d" " -f1 >orig
	... use orig as a list of sha1s ...

But the "grep piped to cut" can be a single process

	... >raw &&
	sed -n -e 's/\(find\).*/\1/p' raw >orig
	... use orig as a list of sha1s ...

So in the tiniest picture, turning "sed -n -e /find/p" into "grep" is not wrong per-se, but if you step back a bit and see a larger picture, using "sed" a bit more effectively turns out to be still a better rewrite.

... and I wrote the above before I read the remainder of your response, where you considered which one is easier to read between "grep piped to cut" and "sed" ;-)

> (By the way, if I were to reroll this series, should I keep sending out
> the entire patchset? It feels very noisy to send out 20-something emails
> every reroll when I'm just making a small one or two line change.)

Especially if it is near the end of the series, just a single step is OK. But is there anything that is glaringly wrong that needs a reroll? Or would it be "this is good enough, so let's have them cook in 'next' and graduate to 'master'---further clean-up can be done after all the dust settles"? I have an impression that we reached the latter by now.

Thanks.