[2/6] test-lib-functions: test_cmp: eval $GIT_TEST_CMP

Commit Message

Đoàn Trần Công Danh March 19, 2020, 2 p.m. UTC

Shell recognises first non-assignment token as command name.
Thus, ` cd t/perf; ./p0000-perf-lib-sanity.sh -d -i -v` reports:

> test_cmp:1: command not found: diff -u

Using `eval` to unquote $GIT_TEST_CMP as same as precedence in `git_editor`.

Signed-off-by: Đoàn Trần Công Danh <[e-mail]>
---
 t/test-lib-functions.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jeff King March 19, 2020, 4:02 p.m. UTC | #1

On Thu, Mar 19, 2020 at 09:00:03PM +0700, Đoàn Trần Công Danh wrote:

> Shell recognises first non-assignment token as command name.
> Thus, ` cd t/perf; ./p0000-perf-lib-sanity.sh -d -i -v` reports:
> 
> > test_cmp:1: command not found: diff -u
> 
> Using `eval` to unquote $GIT_TEST_CMP as same as precedence in `git_editor`.

Adding an "eval" here will subtly change the behavior for other shells.
Right now they'd just do whitespace splitting (which presumably busybox
is not), but with this we'd expand metachars, etc.

I suspect that's fine (and maybe even preferable, because if you really
do have a space in the path you can actually escape it). So I don't mind
this change.

I do worry that this whitespace splitting behavior could bite us in
other scripts. Curiously, my version of busybox (1.30.1) doesn't seem to
have any problem with it, though.

-Peff

Eric Sunshine March 19, 2020, 4:14 p.m. UTC | #2

On Thu, Mar 19, 2020 at 12:02 PM Jeff King <[e-mail]> wrote:
> On Thu, Mar 19, 2020 at 09:00:03PM +0700, Đoàn Trần Công Danh wrote:
> > > test_cmp:1: command not found: diff -u
> >
> > Using `eval` to unquote $GIT_TEST_CMP as same as precedence in `git_editor`.
>
> I do worry that this whitespace splitting behavior could bite us in
> other scripts. Curiously, my version of busybox (1.30.1) doesn't seem to
> have any problem with it, though.

I had the same reaction upon reading the patch. It's not just other
scripts in which the problem might manifest, but a change to the value
of some variable which is used as a command invocation. Providing a
"fix" for this one particular case may help get past test a failure on
Alpine under busybox, but it is not a good general solution. (Some
sort of helper function which smooths away differences like this --
say git_indirect_cmd() or something -- which can be used wherever a
$VARIABLE is invoked as a command might be a better approach, but I
haven't really thought it through.)

Đoàn Trần Công Danh March 20, 2020, 1:29 a.m. UTC | #3

On 2020-03-19 12:02:11-0400, Jeff King <[e-mail]> wrote:
> On Thu, Mar 19, 2020 at 09:00:03PM +0700, Đoàn Trần Công Danh wrote:
> 
> > Shell recognises first non-assignment token as command name.
> > Thus, ` cd t/perf; ./p0000-perf-lib-sanity.sh -d -i -v` reports:
> > 
> > > test_cmp:1: command not found: diff -u
> > 
> > Using `eval` to unquote $GIT_TEST_CMP as same as precedence in `git_editor`.
> 
> Adding an "eval" here will subtly change the behavior for other shells.
> Right now they'd just do whitespace splitting (which presumably busybox
> is not), but with this we'd expand metachars, etc.
> 
> I suspect that's fine (and maybe even preferable, because if you really
> do have a space in the path you can actually escape it). So I don't mind
> this change.
> 
> I do worry that this whitespace splitting behavior could bite us in
> other scripts. Curiously, my version of busybox (1.30.1) doesn't seem to
> have any problem with it, though.

Ah, this patch wasn't meant for busybox.
It run into failure with both bash and dash in VoidLinux, and bash in
ArchLinux, I couldn't start p0000 in Ubuntu docker image due to
missing /usr/bin/time (even if I have /usr/bin/time installed).

Patch

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 352c213d52..ab0e47ae17 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -905,7 +905,7 @@  test_expect_code () {
 # - not all diff versions understand "-u"
 
 test_cmp() {
-	$GIT_TEST_CMP "$@"
+	eval "$GIT_TEST_CMP" '"$@"'
 }
 
 # Check that the given config key has the expected value.