Re: [PATCH 5/5] maintenance: allow custom refspecs during prefetch

From: Derrick Stolee <stolee@gmail.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, tom.saeger@oracle.com, gitster@pobox.com,
	sunshine@sunshineco.com,
	Derrick Stolee <derrickstolee@github.com>,
	Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH 5/5] maintenance: allow custom refspecs during prefetch
Date: Fri, 9 Apr 2021 07:48:31 -0400	[thread overview]
Message-ID: <243a0aa1-7c0a-d2ab-49ff-8ea8cacc1b81@gmail.com> (raw)
In-Reply-To: <87im4yjspp.fsf@evledraar.gmail.com>

On 4/7/2021 6:26 AM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Apr 07 2021, Ævar Arnfjörð Bjarmason wrote:
> 
>> On Mon, Apr 05 2021, Derrick Stolee via GitGitGadget wrote:
>>> +	GIT_TRACE2_EVENT="$(pwd)/prefetch-refspec.txt" git maintenance run --task=prefetch 2>/dev/null &&
>>
>> I see this is following some established convention in the file, but is
>> there really not a way to make this pass without directing stderr to
>> /dev/null? It makes ad-hoc debugging when reviewing harder.
> 
> As I later found out this is copy/pasted to get around the fact that
> --quiet is dependent on isatty(), so without this the result would be
> different under --verbose and non-verbose testing.

Yes, adding --quiet directly is a better pattern.

> So that dates back to 3ddaad0e060 (maintenance: add --quiet option,
> 2020-09-17), but I see other quiet=isatty(2) in related code. I wish we
> could isolate that particular behavior so removing the 2>/dev/null when
> debugging the tests doesn't cause you to run into this, maybe an
> explicit --quiet or --no-quiet option for all but one test that's
> checking that isatty() behavior?
> 
>> I tried just removing it, but then (in an earlier test case) the
>> "test_subcommand" fails because it can't find the line we're looking
>> for, so us piping stderr to /dev/null impacts our trace2 output?
> 
> I hadn't seen seen test_subcommand before, sorry to be blunt, but "ew!".

Saying "I'm about to be rude" before being rude doesn't excuse it.
I had to step away and get over this comment before I could examine
the actually constructive feedback below.

A better way to communicate the same information could be "This test
helper seems more complicated than necessary, and has some gaps that
could be filled." I completely agree with this statement.

> So we're ad-hoc grepping trace2 JSON output just to find out whether we
> invoked some subcommand. But unlike test_expect_code etc. this one
> doesn't run git for you, but instead we have temp *.txt files and the
> command disconnected from the run.
> 
> And because you're using "grep" and "! grep" to test, you're hiding the
> difference between "did not find this line" v.s. "did not find anything
> at all".

You're right that there is value in comparing the ordered list of
subcommands run by a given Git command. That will catch buggy tests
that are checking that a subcommand doesn't run.

> Because of that the second test using test_subcommand is either buggy or
> painfully non-obvious. We check that "run --auto" doesn't contain a
> "auto --quiet", but in reality it doesn't contain any subcommands at
> all. We didn't run any because it exited with "nothing to pack".

That exit is from the third command, which does not pass the --auto
command. The "nothing to pack" output is from the 'git gc --no-quiet'
subcommand that is being checked in this third test.

> I think converting the whole thing to something like the WIP/RFC patch
> below is much better and more readable.

This is an interesting approach. I don't see you using the ERR that you
are inputting anywhere, so that seems like an unnecessary bloat to the
consumers. But maybe I haven't discovered all of the places where this
would be useful, but it seems better to pipe stderr to a file for later
comparison when needed.
> +test_expect_process_tree () {
> +	depth= &&
> +	>actual &&
> +	cat >expect &&
> +	cat <&3 >expect.err
> +	while test $# != 0
> +	do
> +		case "$1" in
> +		--depth)
> +			depth="$2"
> +			shift
> +			;;
> +		*)
> +			break
> +			;;
> +		esac
> +		shift
> +	done &&
Do you have an example where this is being checked? Or can depth
be left as 1 for now?

> +	log="$(pwd)/proc-tree.txt" &&
> +	>"$log" &&
> +	GIT_TRACE2_PERF="$log" "$@" 2>actual.err &&
> +	grep "child_start" proc-tree.txt >proc-tree-start.txt || : &&
> +	if test -n "$depth"
> +	then
> +		grep " d$depth " proc-tree-start.txt >tmp.txt || : &&
> +		mv tmp.txt proc-tree-start.txt
> +	fi &&
> +	sed -e 's/^.*argv:\[//' -e 's/\]$//' <proc-tree-start.txt >actual &&
> +	test_cmp expect actual &&
> +	test_cmp expect.err actual.err
> +} 7>&2 2>&4

I think similar ideas could apply to test_region. Giving it a try
now.

-Stolee