Re: [PATCH 2/5] test-lib: use exact match for test_subcommand

From: Eric Sunshine <sunshine@sunshineco.com>
To: Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
Cc: Git List <git@vger.kernel.org>,
	Tom Saeger <tom.saeger@oracle.com>,
	Junio C Hamano <gitster@pobox.com>,
	Derrick Stolee <derrickstolee@github.com>,
	Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH 2/5] test-lib: use exact match for test_subcommand
Date: Mon, 5 Apr 2021 13:31:42 -0400	[thread overview]
Message-ID: <CAPig+cRaysm=-UcqY4zY5cV0KzAwd+RwzxXN-ZMV0nz=L4_+DA@mail.gmail.com> (raw)
In-Reply-To: <2b74889c2a323f03be477ffdf9ff388405779c3b.1617627856.git.gitgitgadget@gmail.com>

On Mon, Apr 5, 2021 at 9:04 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> The use of 'grep' inside test_subcommand uses general patterns, leading
> to sometimes needing escape characters to avoid incorrect matches.
> Further, some platforms interpret different glob characters differently.

These are regular expression metacharacters, not glob characters. A
more general way to say this might be:

    Furthermore, it can be difficult to know which characters need
    escaping since the actual regular expression language implemented
    by various `grep`s differs between platforms; for instance, some
    may employ pure BRE, whereas others a mix of BRE & ERE.

    Sidestep this difficulty by using `grep -F`...

> Use 'grep -F' to use an exact match. This requires removing escape
> characters from existing callers. Luckily, this is only one test that
> expects refspecs as part of the subcommand.
>
> Reported-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>

The Reported-by: feels a bit unusual in this context. Perhaps
Helped-by: would be more appropriate.

> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> @@ -142,8 +142,8 @@ test_expect_success 'prefetch multiple remotes' '
> -       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/remote1/* <run-prefetch.txt &&
> +       test_subcommand git fetch remote2 $fetchargs +refs/heads/*:refs/prefetch/remote2/* <run-prefetch.txt &&

To be really robust and avoid accidental glob expansion (as unlikely
as it is), you should quote any arguments which contain glob
metacharacters such as "*" rather than supplying them bare like this.