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

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

On Wed, Apr 07 2021, Ævar Arnfjörð Bjarmason wrote:

> On Mon, Apr 05 2021, Derrick Stolee via GitGitGadget wrote:
>
>> [...]
>> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
>> index fc2315edec11..3366ea188782 100755
>> --- a/t/t7900-maintenance.sh
>> +++ b/t/t7900-maintenance.sh
>> @@ -142,20 +142,51 @@ test_expect_success 'prefetch multiple remotes' '
>>  	test_commit -C clone2 two &&
>>  	GIT_TRACE2_EVENT="$(pwd)/run-prefetch.txt" git maintenance run --task=prefetch 2>/dev/null &&
>>  	fetchargs="--prune --no-tags --no-write-fetch-head --recurse-submodules=no --refmap= --quiet" &&
>> -	test_subcommand git fetch remote1 $fetchargs +refs/heads/*:refs/prefetch/remote1/* <run-prefetch.txt &&
>> -	test_subcommand git fetch remote2 $fetchargs +refs/heads/*:refs/prefetch/remote2/* <run-prefetch.txt &&
>> +	test_subcommand git fetch remote1 $fetchargs +refs/heads/*:refs/prefetch/remotes/remote1/* <run-prefetch.txt &&
>> +	test_subcommand git fetch remote2 $fetchargs +refs/heads/*:refs/prefetch/remotes/remote2/* <run-prefetch.txt &&
>>  	test_path_is_missing .git/refs/remotes &&
>> -	git log prefetch/remote1/one &&
>> -	git log prefetch/remote2/two &&
>> +	git log prefetch/remotes/remote1/one &&
>> +	git log prefetch/remotes/remote2/two &&
>>  	git fetch --all &&
>> -	test_cmp_rev refs/remotes/remote1/one refs/prefetch/remote1/one &&
>> -	test_cmp_rev refs/remotes/remote2/two refs/prefetch/remote2/two &&
>> +	test_cmp_rev refs/remotes/remote1/one refs/prefetch/remotes/remote1/one &&
>> +	test_cmp_rev refs/remotes/remote2/two refs/prefetch/remotes/remote2/two &&
>>  
>>  	test_cmp_config refs/prefetch/ log.excludedecoration &&
>>  	git log --oneline --decorate --all >log &&
>>  	! grep "prefetch" log
>>  '
>>  
>> +test_expect_success 'prefetch custom refspecs' '
>> +	git -C clone1 branch -f special/fetched HEAD &&
>> +	git -C clone1 branch -f special/secret/not-fetched HEAD &&
>> +
>> +	# create multiple refspecs for remote1
>> +	git config --add remote.remote1.fetch +refs/heads/special/fetched:refs/heads/fetched &&
>> +	git config --add remote.remote1.fetch ^refs/heads/special/secret/not-fetched &&
>> +
>> +	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.

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

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

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

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

The pattern is basically stolen from test_commit and
check_sub_test_lib_test_err, respectively.

As an aside: The new test_expect_process_tree function would be much
less painful if we had a helper that took the JSON output and emitted
some sensible subset of the information therein. For now I just ad-hoc
grepped the "perf" output. AFAICT the only way to get "depth" from
trace2's JSON so to count slashes in SIDs.

In the case of your tests you're mostly/(only?) interested in a slice of
the "child_start" events. If we had a helper to spew out a pretty-print
version of that (or a subset of it) we could test_cmp against that
directly.

diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 3366ea18878..d03fb361562 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -30,28 +30,41 @@ test_expect_success 'help text' '
 '
 
 test_expect_success 'run [--auto|--quiet]' '
-	GIT_TRACE2_EVENT="$(pwd)/run-no-auto.txt" \
-		git maintenance run 2>/dev/null &&
-	GIT_TRACE2_EVENT="$(pwd)/run-auto.txt" \
-		git maintenance run --auto 2>/dev/null &&
-	GIT_TRACE2_EVENT="$(pwd)/run-no-quiet.txt" \
-		git maintenance run --no-quiet 2>/dev/null &&
-	test_subcommand git gc --quiet <run-no-auto.txt &&
-	test_subcommand ! git gc --auto --quiet <run-auto.txt &&
-	test_subcommand git gc --no-quiet <run-no-quiet.txt
+	test_expect_process_tree --depth 0 git maintenance run <<-\OUT 3<<-\ERR &&
+	git gc --quiet
+	OUT
+	ERR
+
+	test_expect_process_tree --depth 0 git maintenance run --auto <<-\OUT 3<<-\ERR &&
+	OUT
+	ERR
+
+	test_expect_process_tree --depth 0 git maintenance run --quiet <<-\OUT 3<<-\ERR &&
+	git gc --quiet
+	OUT
+	ERR
+
+	test_expect_process_tree --depth 0 git maintenance run --no-quiet <<-\OUT 3<<-\ERR
+	git gc --no-quiet
+	OUT
+	ERR
 '
 
 test_expect_success 'maintenance.auto config option' '
-	GIT_TRACE2_EVENT="$(pwd)/default" git commit --quiet --allow-empty -m 1 &&
-	test_subcommand git maintenance run --auto --quiet <default &&
-	GIT_TRACE2_EVENT="$(pwd)/true" \
-		git -c maintenance.auto=true \
-		commit --quiet --allow-empty -m 2 &&
-	test_subcommand git maintenance run --auto --quiet  <true &&
-	GIT_TRACE2_EVENT="$(pwd)/false" \
-		git -c maintenance.auto=false \
-		commit --quiet --allow-empty -m 3 &&
-	test_subcommand ! git maintenance run --auto --quiet  <false
+	test_expect_process_tree git commit --quiet --allow-empty -m 1 <<-\OUT 3<<-\ERR &&
+	git maintenance run --auto --quiet
+	OUT
+	ERR
+
+	test_expect_process_tree git commit --quiet --allow-empty -m 2 <<-\OUT 3<<-\ERR &&
+	git maintenance run --auto --quiet
+	OUT
+	ERR
+
+	test_expect_process_tree git commit --quiet --allow-empty -m 3 <<-\OUT 3<<-\ERR
+	git maintenance run --auto --quiet
+	OUT
+	ERR
 '
 
 test_expect_success 'maintenance.<task>.enabled' '
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index a5915dec22d..cd1187b473c 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1625,6 +1625,38 @@ test_path_is_hidden () {
 	return 1
 }
 
+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 &&
+	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
+
 # Check that the given command was invoked as part of the
 # trace2-format trace on stdin.
 #