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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Apr 09 2021, Derrick Stolee wrote:

> 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.

I didn't mean to be rude, my understanding of "blunt" is more like
"forthright" or "without mincing words" or something to that effect, but
I'm not a native speaker of English, so I'll take your understanding
over mine and apologize, sorry.

What I meant to say is: When we look at some prior art in the test suite
that does similar things this pattern is needlessly convoluted, and
separates the command invocation from the test assert itself. Something
similar-ish to check_sub_test_lib_test would allow you to run the
command, but do any asserting of the proc tree in the helper.

>> 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.

Yes, it's probably not a good default here. For the test-lib.sh tests
there's check_sub_test_lib_test and check_sub_test_lib_test_err, most of
the tests only test stdout.

>> +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?

It can probably be hardcoded, but I was hoping someone more familiar
with trace2 would chime in, but I'm fairly sure there's not a way to do
it without parsing the existing output with either some clever
grep/awk-ing of the PERF output, or stateful parsing of the JSON.

I thought that for git maintenance tests perhaps something wanted to
assert that we didn't have maintenance invoking maintenance, or that
something expected to prune refs really invoked the relevant prune
command via "gc".

>> +	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.

Probably, I didn't even notice that one...