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

Commit Message

Derrick Stolee April 5, 2021, 1:04 p.m. UTC

From: Derrick Stolee <[e-mail]>

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.

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 <[e-mail]>
Signed-off-by: Derrick Stolee <[e-mail]>
---
 t/t7900-maintenance.sh  | 4 ++--
 t/test-lib-functions.sh | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

Comments

Eric Sunshine April 5, 2021, 5:31 p.m. UTC | #1

On Mon, Apr 5, 2021 at 9:04 AM Derrick Stolee via GitGitGadget
<[e-mail]> 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 <[e-mail]>
> Signed-off-by: Derrick Stolee <[e-mail]>

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.

Junio C Hamano April 5, 2021, 5:43 p.m. UTC | #2

Eric Sunshine <[e-mail]> writes:

> On Mon, Apr 5, 2021 at 9:04 AM Derrick Stolee via GitGitGadget
> <[e-mail]> 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 <[e-mail]>
>> Signed-off-by: Derrick Stolee <[e-mail]>
>
> 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.

Yup, just enclose the whole refspec inside dq-pair, like

	test_subcommand git fetch remote2 $fetchargs \
		"+refs/heads/*:refs/prefetch/remote2/*" <run-prefetch.txt &&

would be the easiest to read.

Thanks.

Patch

diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 2412d8c5c006..fc2315edec11 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -142,8 +142,8 @@  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/remote1/* <run-prefetch.txt &&
+	test_subcommand git fetch remote2 $fetchargs +refs/heads/*:refs/prefetch/remote2/* <run-prefetch.txt &&
 	test_path_is_missing .git/refs/remotes &&
 	git log prefetch/remote1/one &&
 	git log prefetch/remote2/two &&
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 6348e8d7339c..a5915dec22df 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1652,9 +1652,9 @@  test_subcommand () {
 
 	if test -n "$negate"
 	then
-		! grep "\[$expr\]"
+		! grep -F "[$expr]"
 	else
-		grep "\[$expr\]"
+		grep -F "[$expr]"
 	fi
 }