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
next prev parent reply other threads:[~2021-04-09 11:49 UTC|newest]
Thread overview: 72+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-05 13:04 [PATCH 0/5] Maintenance: adapt custom refspecs Derrick Stolee via GitGitGadget
2021-04-05 13:04 ` [PATCH 1/5] maintenance: simplify prefetch logic Derrick Stolee via GitGitGadget
2021-04-05 17:01 ` Tom Saeger
2021-04-05 13:04 ` [PATCH 2/5] test-lib: use exact match for test_subcommand Derrick Stolee via GitGitGadget
2021-04-05 17:31 ` Eric Sunshine
2021-04-05 17:43 ` Junio C Hamano
2021-04-05 13:04 ` [PATCH 3/5] refspec: output a refspec item Derrick Stolee via GitGitGadget
2021-04-05 16:57 ` Tom Saeger
2021-04-05 17:40 ` Eric Sunshine
2021-04-05 17:44 ` Junio C Hamano
2021-04-06 11:21 ` Derrick Stolee
2021-04-06 15:23 ` Eric Sunshine
2021-04-06 16:51 ` Derrick Stolee
2021-04-07 8:46 ` Ævar Arnfjörð Bjarmason
2021-04-07 20:53 ` Derrick Stolee
2021-04-07 22:05 ` Ævar Arnfjörð Bjarmason
2021-04-07 22:49 ` Junio C Hamano
2021-04-07 23:01 ` Ævar Arnfjörð Bjarmason
2021-04-08 7:33 ` Junio C Hamano
2021-04-05 13:04 ` [PATCH 4/5] test-tool: test refspec input/output Derrick Stolee via GitGitGadget
2021-04-05 17:52 ` Eric Sunshine
2021-04-06 11:13 ` Derrick Stolee
2021-04-07 8:54 ` Ævar Arnfjörð Bjarmason
2021-04-05 13:04 ` [PATCH 5/5] maintenance: allow custom refspecs during prefetch Derrick Stolee via GitGitGadget
2021-04-05 17:16 ` Tom Saeger
2021-04-06 11:15 ` Derrick Stolee
2021-04-07 8:53 ` Ævar Arnfjörð Bjarmason
2021-04-07 10:26 ` Ævar Arnfjörð Bjarmason
2021-04-09 11:48 ` Derrick Stolee [this message]
2021-04-09 19:28 ` Ævar Arnfjörð Bjarmason
2021-04-10 0:56 ` Derrick Stolee
2021-04-10 11:37 ` Ævar Arnfjörð Bjarmason
2021-04-07 13:47 ` Ævar Arnfjörð Bjarmason
2021-04-06 18:47 ` [PATCH v2 0/5] Maintenance: adapt custom refspecs Derrick Stolee via GitGitGadget
2021-04-06 18:47 ` [PATCH v2 1/5] maintenance: simplify prefetch logic Derrick Stolee via GitGitGadget
2021-04-07 23:23 ` Emily Shaffer
2021-04-09 19:00 ` Derrick Stolee
2021-04-06 18:47 ` [PATCH v2 2/5] test-lib: use exact match for test_subcommand Derrick Stolee via GitGitGadget
2021-04-06 18:47 ` [PATCH v2 3/5] refspec: output a refspec item Derrick Stolee via GitGitGadget
2021-04-06 18:47 ` [PATCH v2 4/5] test-tool: test refspec input/output Derrick Stolee via GitGitGadget
2021-04-07 23:08 ` Josh Steadmon
2021-04-07 23:26 ` Emily Shaffer
2021-04-06 18:47 ` [PATCH v2 5/5] maintenance: allow custom refspecs during prefetch Derrick Stolee via GitGitGadget
2021-04-06 19:36 ` Tom Saeger
2021-04-06 19:45 ` Derrick Stolee
2021-04-07 23:09 ` Josh Steadmon
2021-04-07 23:37 ` Emily Shaffer
2021-04-08 0:23 ` Jonathan Tan
2021-04-10 2:03 ` [PATCH v3 0/3] Maintenance: adapt custom refspecs Derrick Stolee via GitGitGadget
2021-04-10 2:03 ` [PATCH v3 1/3] maintenance: simplify prefetch logic Derrick Stolee via GitGitGadget
2021-04-12 20:13 ` Tom Saeger
2021-04-12 20:27 ` Derrick Stolee
2021-04-10 2:03 ` [PATCH v3 2/3] fetch: add --prefetch option Derrick Stolee via GitGitGadget
2021-04-11 21:09 ` Ramsay Jones
2021-04-12 20:23 ` Derrick Stolee
2021-04-10 2:03 ` [PATCH v3 3/3] maintenance: use 'git fetch --prefetch' Derrick Stolee via GitGitGadget
2021-04-11 1:35 ` [PATCH v3 0/3] Maintenance: adapt custom refspecs Junio C Hamano
2021-04-12 16:48 ` Tom Saeger
2021-04-12 17:24 ` Tom Saeger
2021-04-12 17:41 ` Tom Saeger
2021-04-12 20:25 ` Derrick Stolee
2021-04-16 12:49 ` [PATCH v4 0/4] " Derrick Stolee via GitGitGadget
2021-04-16 12:49 ` [PATCH v4 1/4] maintenance: simplify prefetch logic Derrick Stolee via GitGitGadget
2021-04-16 18:02 ` Tom Saeger
2021-04-16 12:49 ` [PATCH v4 2/4] fetch: add --prefetch option Derrick Stolee via GitGitGadget
2021-04-16 17:52 ` Tom Saeger
2021-04-16 18:26 ` Tom Saeger
2021-04-16 12:49 ` [PATCH v4 3/4] maintenance: use 'git fetch --prefetch' Derrick Stolee via GitGitGadget
2021-04-16 12:49 ` [PATCH v4 4/4] maintenance: respect remote.*.skipFetchAll Derrick Stolee via GitGitGadget
2021-04-16 13:54 ` Ævar Arnfjörð Bjarmason
2021-04-16 14:33 ` Tom Saeger
2021-04-16 18:31 ` Tom Saeger
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=243a0aa1-7c0a-d2ab-49ff-8ea8cacc1b81@gmail.com \
--to=stolee@gmail.com \
--cc=avarab@gmail.com \
--cc=derrickstolee@github.com \
--cc=dstolee@microsoft.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=sunshine@sunshineco.com \
--cc=tom.saeger@oracle.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).