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

From: Denton Liu <[email-masked]>
Date: Tue, 3 Dec 2019 23:24:37 -0800

Hi Junio,

On Tue, Dec 03, 2019 at 07:41:19AM -0800, Junio C Hamano wrote:
> Denton Liu <[email-masked]> writes:
> 
> >> 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.
> >
> > Perhaps the log message could use some improvement to document the
> > discussion we had? I don't know if that's worth a reroll, though. Aside
> > from that, I agree that it's ready for 'next'.
> 
> Sure, let's see what you have in mind.

Here's a complete replacement for the commit message:

	t7700: consolidate code into test_no_missing_in_packs()

	The code to test that objects were not missing from the packfile was
	duplicated many times. Extract the duplicated code into
	test_no_missing_in_packs() and use that instead.

	Refactor the resulting extraction so that if any git commands fail,
	their return codes are not silently lost.

	We were using sed to filter lines. Although not incorrect, this is
	exactly what grep is built for. Replace this invocation of sed with grep
	so that we use the correct tool for the job.

	Instead of verifying each file of `alt_objects/pack/*.idx` individually
	in a for-loop, batch them together into one verification step.

	The original testing construct was O(n^2): it used a grep in a loop to
	test whether any objects were missing in the packfile. Rewrite this to
	sort the files then use `comm -23` so that finding missing lines from
	the original file is done more efficiently.

	The result of this is that we end up with a `grep | cut | sort`
	pipeline. Previously, we were extracting the `sha1` as part of the
	`while read sha1 rest` loop. Since we removed the while-loop, we need to
	use `cut` to extract the `sha1` field. Note that we could have chosen to
	combine the `grep | cut` into a single `sed` invocation but we
	consciously leave it separate as it makes the intent more clear.

	While we're at it, add a space to `commit_and_pack ()` for style.

	Signed-off-by: Denton Liu <[email-masked]>

The only change between this and the old commit message is the addition
of the "The result of this..." paragraph.

Thanks,

Denton

> 
> Thanks for working on this.