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
