[PATCH] t4014: shell portability fix

[PATCH] t4014: shell portability fix

[Junio C Hamano]

One-shot assignment to an environment variable, i.e.

	VAR=VAL cmd

does not work as expected for "cmd" that is a shell function on
certain shells.  test_commit _is_ a helper function and cannot be
driven that way portably.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t4014-format-patch.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 8049cad..c3aa543 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1072,7 +1072,7 @@ test_expect_success '--from omits redundant in-body header' '
 '
 
 test_expect_success 'in-body headers trigger content encoding' '
-	GIT_AUTHOR_NAME="éxötìc" test_commit exotic &&
+	(export GIT_AUTHOR_NAME="éxötìc"; test_commit exotic) &&
 	test_when_finished "git reset --hard HEAD^" &&
 	git format-patch -1 --stdout --from >patch &&
 	cat >expect <<-\EOF &&

Re: [PATCH] t4014: shell portability fix

[Jeff King]

On Tue, May 31, 2016 at 03:53:15PM -0700, Junio C Hamano wrote:

> One-shot assignment to an environment variable, i.e.
> 
> 	VAR=VAL cmd
> 
> does not work as expected for "cmd" that is a shell function on
> certain shells.  test_commit _is_ a helper function and cannot be
> driven that way portably.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  t/t4014-format-patch.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index 8049cad..c3aa543 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -1072,7 +1072,7 @@ test_expect_success '--from omits redundant in-body header' '
>  '
>  
>  test_expect_success 'in-body headers trigger content encoding' '
> -	GIT_AUTHOR_NAME="éxötìc" test_commit exotic &&
> +	(export GIT_AUTHOR_NAME="éxötìc"; test_commit exotic) &&

Thanks. This one is my fault. There's another use of the same name
elsewhere, but it's to call "git commit" directly, so it's OK.

-Peff

PS I really hate this particular shell behavior, as it means that your
   code can break because somebody _else_ decides to replace the command
   you are calling with a function.  But I guess we are stuck with it.

Re: [PATCH] t4014: shell portability fix

[Ramsay Jones]

On 31/05/16 23:53, Junio C Hamano wrote:
> One-shot assignment to an environment variable, i.e.
> 
> 	VAR=VAL cmd
> 
> does not work as expected for "cmd" that is a shell function on
> certain shells.  test_commit _is_ a helper function and cannot be
> driven that way portably.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  t/t4014-format-patch.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index 8049cad..c3aa543 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -1072,7 +1072,7 @@ test_expect_success '--from omits redundant in-body header' '
>  '
>  
>  test_expect_success 'in-body headers trigger content encoding' '
> -	GIT_AUTHOR_NAME="éxötìc" test_commit exotic &&
> +	(export GIT_AUTHOR_NAME="éxötìc"; test_commit exotic) &&

Isn't 'export VAR=VAL' non-portable? So, maybe:

	(GIT_AUTHOR_NAME="éxötìc"; export GIT_AUTHOR_NAME; test_commit exotic) &&

>  	test_when_finished "git reset --hard HEAD^" &&
>  	git format-patch -1 --stdout --from >patch &&
>  	cat >expect <<-\EOF &&

ATB,
Ramsay Jones

Re: [PATCH] t4014: shell portability fix

[Eric Sunshine]

On Tue, May 31, 2016 at 6:56 PM, Jeff King <peff@peff.net> wrote:
> On Tue, May 31, 2016 at 03:53:15PM -0700, Junio C Hamano wrote:
>> One-shot assignment to an environment variable, i.e.
>>
>>       VAR=VAL cmd
>>
>> does not work as expected for "cmd" that is a shell function on
>> certain shells.  test_commit _is_ a helper function and cannot be
>> driven that way portably.
>>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
>> @@ -1072,7 +1072,7 @@ test_expect_success '--from omits redundant in-body header' '
>>  test_expect_success 'in-body headers trigger content encoding' '
>> -     GIT_AUTHOR_NAME="éxötìc" test_commit exotic &&
>> +     (export GIT_AUTHOR_NAME="éxötìc"; test_commit exotic) &&

Isn't "export FOO=val" unportable?

> Thanks. This one is my fault. There's another use of the same name
> elsewhere, but it's to call "git commit" directly, so it's OK.

I was under the impression that the project was moving toward 'env' to
deal[1] with this sort of issue.

[1]: 512477b (tests: use "env" to run commands with temporary env-var
settings, 2014-03-18)

Re: [PATCH] t4014: shell portability fix

[Jeff King]

On Tue, May 31, 2016 at 08:09:43PM -0400, Eric Sunshine wrote:

> >> -     GIT_AUTHOR_NAME="éxötìc" test_commit exotic &&
> >> +     (export GIT_AUTHOR_NAME="éxötìc"; test_commit exotic) &&
> 
> Isn't "export FOO=val" unportable?

Good catch. I was so busy looking for other cases I didn't even see
the problem here.

> > Thanks. This one is my fault. There's another use of the same name
> > elsewhere, but it's to call "git commit" directly, so it's OK.
> 
> I was under the impression that the project was moving toward 'env' to
> deal[1] with this sort of issue.
> 
> [1]: 512477b (tests: use "env" to run commands with temporary env-var
> settings, 2014-03-18)

We can use it with test_must_fail, because it takes a command name:

  test_must_fail env FOO=BAR whatever-you-would-have-run

But I don't think it works in the general case, as test_commit does not
run its arguments. So you'd want something like:

  env FOO=BAR test_commit exotic

but of course that doesn't work because "env" can't find the shell
function. I think with bash you could do:

  export test_commit
  env FOO=BAR bash -c test_commit exotic

but we can't rely on that.

-Peff

Re: [PATCH] t4014: shell portability fix

[Jeff King]

On Tue, May 31, 2016 at 10:31:59PM -0400, Jeff King wrote:

> On Tue, May 31, 2016 at 08:09:43PM -0400, Eric Sunshine wrote:
> 
> > I was under the impression that the project was moving toward 'env' to
> > deal[1] with this sort of issue.
> > 
> > [1]: 512477b (tests: use "env" to run commands with temporary env-var
> > settings, 2014-03-18)
> 
> We can use it with test_must_fail, because it takes a command name:
> 
>   test_must_fail env FOO=BAR whatever-you-would-have-run
> 
> But I don't think it works in the general case, as test_commit does not
> run its arguments. So you'd want something like:
> 
>   env FOO=BAR test_commit exotic
> 
> but of course that doesn't work because "env" can't find the shell
> function. I think with bash you could do:
> 
>   export test_commit
>   env FOO=BAR bash -c test_commit exotic
> 
> but we can't rely on that.

I wondered if we could implement our own "env" in the shell, but it's a
little non-trivial, because:

  - our basic tool for setting variable-named variables is "eval", which
    means we need an extra layer of quoting

  - we have to restore the variables after. That means telling the
    difference between unset and empty (possible with "-" versus ":-", I
    think), but also the difference between unexported and exported
    (maybe possible by parsing "export -p", but I'd be shocked if that
    doesn't run into portability problems).

Here's what I came up with, which does seem to work. It's pretty gnarly,
though.

-- >8 --
# possible without a sub-program?
# portability issues with no-newline and sed?
shellquote () {
	printf "'"
	printf '%s' "$1" | sed "s/'/'\\\\''/g"
	printf "'"
}

# is there a simpler way, even when the contents are "unset"?
is_unset () {
	eval "test -z \"\${$1}\" && test unset = \"\${$1-unset}\""
}

# probably not portable; also, possible without sub-program?
is_exported () {
	export -p | grep "^declare -x $1="
}

# just a syntactic convenience
add_to () {
	eval "$1=\"\$$1
		\$2\""
}

fake_env () {
	fake_env_restore_=
	while test $# -gt 0
	do
		case "$1" in
		*=*)
			# this whole thing is not safe when the var name has
			# spaces or other meta-characters, but since the names
			# all come from our test scripts, that should be OK
			fake_env_var_=${1%%=*}
			eval "fake_env_orig_=\$$fake_env_var_"
			fake_env_val_=${1#*=}
			shift

			# unset value and clear export flag...
			add_to fake_env_restore_ "unset $fake_env_var_"
			# ...and then restore what was there, if anything
			if ! is_unset "$fake_env_var_"
			then
				add_to fake_env_restore_ \
					"$fake_env_var_=$(shellquote "$fake_env_orig_")"
				if is_exported "$fake_env_var_"
				then
					add_to fake_env_restore_ \
						"export $fake_env_var_"
				fi
			fi

			eval "$fake_env_var_=$(shellquote "$fake_env_val_")"
			eval "export $fake_env_var_"
			;;
		*)
			"$@"
			fake_env_ret_=$?
			eval "$fake_env_restore_"
			return $fake_env_ret_
			;;
		esac
	done
}

# simple exercise
show() {
	echo >&2 "$1: myvar=$myvar"
}

myvar="horrible \"\\' mess"
show before
fake_env myvar="temporary $myvar" show during
show after

-Peff

Re: [PATCH] t4014: shell portability fix

[Jeff King]

On Tue, May 31, 2016 at 11:31:40PM -0400, Jeff King wrote:

> I wondered if we could implement our own "env" in the shell, but it's a
> little non-trivial, because:
> [...]
> Here's what I came up with, which does seem to work. It's pretty gnarly,
> though.

Here's a revised version that drops the shellquoting by using extra
layers of "eval" indirection. You may have heard of 3-star C
programmers. I think I just became a 2-dollar shell programmer.

So this is gross, but I think it actually _is_ portable, with the
exception of the "is it exported" check.

-- >8 --
# is there a simpler way, even when the contents are "unset"?
is_unset () {
	eval "test -z \"\${$1}\" && test unset = \"\${$1-unset}\""
}

# probably not portable; also, possible without sub-program?
is_exported () {
	export -p | grep "^declare -x $1="
}

# set var named by $1 to contents of $2
set_var () {
	eval "$1=\$2"
}

# set var named by $1 to contents of var named by $2
copy_var () {
	eval "set_var \$1 \"\$$2\""
}

# just a syntactic convenience
add_to () {
	eval "$1=\"\$$1
		\$2\""
}

fake_env () {
	fake_env_restore_=
	while test $# -gt 0
	do
		case "$1" in
		*=*)
			# this whole thing is not safe when the var name has
			# spaces or other meta-characters, but since the names
			# all come from our test scripts, that should be OK
			fake_env_var_=${1%%=*}
			fake_env_orig_=fake_env_orig_$fake_env_var_
			copy_var $fake_env_orig_ $fake_env_var_
			fake_env_val_=${1#*=}
			shift

			# unset value and clear export flag...
			add_to fake_env_restore_ "unset $fake_env_var_"
			# ...and then restore what was there, if anything
			if ! is_unset "$fake_env_var_"
			then
				add_to fake_env_restore_ \
					"copy_var $fake_env_var_ $fake_env_orig_"
				if is_exported "$fake_env_var_"
				then
					add_to fake_env_restore_ \
						"export $fake_env_var_"
				fi
			fi
			# and then clean up our temp variable
			add_to fake_env_restore_ "unset $fake_env_orig_"

			set_var $fake_env_var_ "$fake_env_val_"
			eval "export $fake_env_var_"
			;;
		*)
			"$@"
			fake_env_ret_=$?
			eval "$fake_env_restore_"
			return $fake_env_ret_
			;;
		esac
	done
}

# simple exercise
foo() {
	echo >&2 "$1: bar=$bar"
}

bar="horrible \"\\' mess"
foo before
fake_env bar="temporary $bar" foo during
foo after

Re: [PATCH] t4014: shell portability fix

[Jeff King]

On Tue, May 31, 2016 at 11:44:13PM -0400, Jeff King wrote:

> # probably not portable; also, possible without sub-program?
> is_exported () {
> 	export -p | grep "^declare -x $1="
> }

Obviously this should have been "grep -q" (and my test didn't notice
because the variable isn't actually exported!).

But yeah, this is not very portable. Doing:

  export AAAA=content
  for i in bash dash mksh ksh93
  do
    printf '%5s ==> ' $i
    $i -c 'export -p' | head -1
  done

yields:

   bash ==> declare -x AAAA="content"
   dash ==> export AAAA='content'
   mksh ==> export AAAA=content
  ksh93 ==> export AAAA=content

which is actually not too hard to pattern-match, but who knows what else
exists. I guess another strategy would be to actually spawn a
sub-process and see if it has the variable set.

I think my code also doesn't handle exported-but-unset variables, though
I'm not sure we really need to care about that in practice.

-Peff

Re: [PATCH] t4014: shell portability fix

[Jeff King]

On Tue, May 31, 2016 at 11:44:13PM -0400, Jeff King wrote:

> So this is gross, but I think it actually _is_ portable, with the
> exception of the "is it exported" check.

Hmm. So after thinking on this, I realized we don't have to do the
clean-up ourselves at all, if we simply operate in a subshell. That
means that any shell functions we call couldn't mutate the state, but
that's probably an acceptable compromise, if the goal is to behave like
env except for being able to call shell functions.

Here is the "final" version of the more complicated scheme I came up
with. That I think should be fairly portable, but the subshell thing is
probably way less gross.

-- >8 --
test_var_is_set () {
	eval "test -n \"\${$1+set}\""
}

test_var_is_exported () {
	sh -c "test -n \"\${$1+set}\""
}

# set var named by $1 to contents of $2
test_set_var () {
	eval "$1=\$2"
}

# set var named by $1 to contents of var named by $2
test_copy_var () {
	eval "test_set_var \$1 \"\$$2\""
}

# just a syntactic convenience
add_to () {
	eval "$1=\"\$$1
		\$2\""
}

test_env () {
	test_env_restore_=
	while test $# -gt 0
	do
		case "$1" in
		*=*)
			# this whole thing is not safe when the var name has
			# spaces or other meta-characters, but since the names
			# all come from our test scripts, that should be OK
			test_env_var_=${1%%=*}
			test_env_orig_=test_env_orig_$test_env_var_
			test_copy_var $test_env_orig_ $test_env_var_
			test_env_val_=${1#*=}
			shift

			# unset value and clear export flag...
			add_to test_env_restore_ "unset $test_env_var_"
			# ...and then restore what was there, if anything
			if test_var_is_set "$test_env_var_"
			then
				add_to test_env_restore_ \
					"test_copy_var $test_env_var_ $test_env_orig_"
				if test_var_is_exported "$test_env_var_"
				then
					add_to test_env_restore_ \
						"export $test_env_var_"
				fi
			fi
			# and then clean up our temp variable
			add_to test_env_restore_ "unset $test_env_orig_"

			test_set_var $test_env_var_ "$test_env_val_"
			eval "export $test_env_var_"
			;;
		*)
			"$@"
			test_env_ret_=$?
			eval "$test_env_restore_"
			return $test_env_ret_
			;;
		esac
	done
}

# simple exercise
show () {
	echo >&2 "$1: here=$myvar ${myvar+(set)} sub=$(sh -c 'echo "$myvar ${myvar+(set)}"')"
}

doit () {
	echo "==> $1"
	show before
	test_env myvar="temporary $myvar" show during
	show after
}

unset myvar
doit unset
myvar="horrible \"\\' mess"
doit with-value
export myvar
doit exported-with-value
myvar=
export myvar
doit exported-but-blank

Re: [PATCH] t4014: shell portability fix

[Jeff King]

On Wed, Jun 01, 2016 at 01:33:25AM -0400, Jeff King wrote:

> Here is the "final" version of the more complicated scheme I came up
> with. That I think should be fairly portable, but the subshell thing is
> probably way less gross.

OK, last email tonight, I promise.

Here's the subshell version. I'm a little embarrassed not to have
thought of it sooner (though the other one was a fun exercise).

	test_env () {
		(
			while test $# -gt 0
			do
				case "$1" in
				*=*)
					eval "${1%%=*}=\${1#*=}"
					eval "export ${1%%=*}"
					shift
					;;
				*)
					"$@"
					exit
					;;
				esac
			done
		)
	}

-Peff

Re: [PATCH] t4014: shell portability fix

[Johannes Sixt]

Am 01.06.2016 um 05:31 schrieb Jeff King:
> 	printf '%s' "$1" | sed "s/'/'\\\\''/g"
...
> 	export -p | grep "^declare -x $1="
...
> 					"$fake_env_var_=$(shellquote "$fake_env_orig_")"
...
> 			eval "$fake_env_var_=$(shellquote "$fake_env_val_")"

An intolerable number of new processes... Please stop this mental exercise.

-- Hannes

Re: [PATCH] t4014: shell portability fix

[Jeff King]

On Wed, Jun 01, 2016 at 07:48:12AM +0200, Johannes Sixt wrote:

> Am 01.06.2016 um 05:31 schrieb Jeff King:
> > 	printf '%s' "$1" | sed "s/'/'\\\\''/g"
> ...
> > 	export -p | grep "^declare -x $1="
> ...
> > 					"$fake_env_var_=$(shellquote "$fake_env_orig_")"
> ...
> > 			eval "$fake_env_var_=$(shellquote "$fake_env_val_")"
> 
> An intolerable number of new processes... Please stop this mental exercise.

Please read to the end of the thread?

-Peff

Re: [PATCH] t4014: shell portability fix

[Eric Sunshine]

On Tue, May 31, 2016 at 10:31 PM, Jeff King <peff@peff.net> wrote:
> On Tue, May 31, 2016 at 08:09:43PM -0400, Eric Sunshine wrote:
>> I was under the impression that the project was moving toward 'env' to
>> deal[1] with this sort of issue.
>>
>> [1]: 512477b (tests: use "env" to run commands with temporary env-var
>> settings, 2014-03-18)
>
> We can use it with test_must_fail, because it takes a command name:
>
>   test_must_fail env FOO=BAR whatever-you-would-have-run
>
> But I don't think it works in the general case, as test_commit does not
> run its arguments. [...]

You're right, of course. I should have seen this.

Re: [PATCH] t4014: shell portability fix

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> OK, last email tonight, I promise.
>
> Here's the subshell version. I'm a little embarrassed not to have
> thought of it sooner (though the other one was a fun exercise).
>
> 	test_env () {
> 		(
> 			while test $# -gt 0
> 			do
> 				case "$1" in
> 				*=*)
> 					eval "${1%%=*}=\${1#*=}"

Is this an elaborate way to say 'eval "$1"', or is there anything
more subtle going on?

> 					eval "export ${1%%=*}"
> 					shift
> 					;;
> 				*)
> 					"$@"
> 					exit

... or 'exec "$@"'

> 					;;
> 				esac
> 			done
> 		)
> 	}

You can dedent the whole thing and remove the outermost {} pair.

Other than that, looks good to me.

>
> -Peff

Re: [PATCH] t4014: shell portability fix

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> 				*)
>> 					"$@"
>> 					exit
>
> ... or 'exec "$@"'

Not really.  I am an idiot.

The whole point of this exercise is that we would want to have a
shell function as $1 at this point in the flow; 'exec' would not
be appropriate here.

Re: [PATCH] t4014: shell portability fix

[Jeff King]

On Tue, May 31, 2016 at 11:49:10PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > OK, last email tonight, I promise.
> >
> > Here's the subshell version. I'm a little embarrassed not to have
> > thought of it sooner (though the other one was a fun exercise).
> >
> > 	test_env () {
> > 		(
> > 			while test $# -gt 0
> > 			do
> > 				case "$1" in
> > 				*=*)
> > 					eval "${1%%=*}=\${1#*=}"
> 
> Is this an elaborate way to say 'eval "$1"', or is there anything
> more subtle going on?

Notice that the value half isn't expanded until we get inside the eval.
So:

  $ good() { eval "${1%%=*}=\${1#*=}"; }
  $ bad() { eval "$1"; }
  $ good foo="funny variable"; echo $foo
  funny variable
  $ bad foo="funny variable"
  bash: variable: command not found

> > 				*)
> > 					"$@"
> > 					exit
> 
> ... or 'exec "$@"'

You can't exec a function, AFAIK (and that was the point of this
exercise).

> > 		)
> > 	}
> 
> You can dedent the whole thing and remove the outermost {} pair.

True. I didn't know about that until recently. Is it portable
everywhere?

Here's the patch I wrote up earlier (but was too timid to send out after
my barrage of emails :) ).

It doesn't have the dedent, but I don't mind if you want to tweak it.

-- >8 --
Subject: test-lib: add in-shell "env" replacement

The one-shot environment variable syntax:

  FOO=BAR some-program

is unportable when some-program is actually a shell
function, like test_must_fail (on some shells FOO remains
set after the function returns, and on others it does not).

We sometimes get around this by using env, like:

  test_must_fail env FOO=BAR some-program

But that only works because test_must_fail's arguments are
themselves a command which can be run. You can't run:

  env FOO=BAR test_must_fail some-program

because env does not know about our shell functions. So
there is no equivalent for test_commit, for example, and one
must resort to:

  (
    FOO=BAR
    export FOO
    test_commit
  )

which is a bit verbose.  Let's add a version of "env" that
works _inside_ the shell, by creating a subshell, exporting
variables from its argument list, and running the command.

Its use is demonstrated on a currently-unportable case in
t4014.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t4014-format-patch.sh |  2 +-
 t/test-lib-functions.sh | 22 ++++++++++++++++++++++
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 8049cad..805dc90 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1072,7 +1072,7 @@ test_expect_success '--from omits redundant in-body header' '
 '
 
 test_expect_success 'in-body headers trigger content encoding' '
-	GIT_AUTHOR_NAME="éxötìc" test_commit exotic &&
+	test_env GIT_AUTHOR_NAME="éxötìc" test_commit exotic &&
 	test_when_finished "git reset --hard HEAD^" &&
 	git format-patch -1 --stdout --from >patch &&
 	cat >expect <<-\EOF &&
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 3978fc0..48884d5 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -939,3 +939,25 @@ mingw_read_file_strip_cr_ () {
 		eval "$1=\$$1\$line"
 	done
 }
+
+# Like "env FOO=BAR some-program", but run inside a subshell, which means
+# it also works for shell functions (though those functions cannot impact
+# the environment outside of the test_env invocation).
+test_env () {
+	(
+		while test $# -gt 0
+		do
+			case "$1" in
+			*=*)
+				eval "${1%%=*}=\${1#*=}"
+				eval "export ${1%%=*}"
+				shift
+				;;
+			*)
+				"$@"
+				exit
+				;;
+			esac
+		done
+	)
+}
-- 
2.9.0.rc0.174.g479a78d

Re: [PATCH] t4014: shell portability fix

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> > 					eval "${1%%=*}=\${1#*=}"
>> 
>> Is this an elaborate way to say 'eval "$1"', or is there anything
>> more subtle going on?
>
> Notice that the value half isn't expanded until we get inside the eval.

Ahh, right.

> Here's the patch I wrote up earlier (but was too timid to send out after
> my barrage of emails :) ).

Looks very sensible.  I'll drop all these "Attempt to test with
ksh93 found these breakages" patches and queue this one.

> -- >8 --
> Subject: test-lib: add in-shell "env" replacement
>
> The one-shot environment variable syntax:
>
>   FOO=BAR some-program
>
> is unportable when some-program is actually a shell
> function, like test_must_fail (on some shells FOO remains
> set after the function returns, and on others it does not).
>
> We sometimes get around this by using env, like:
>
>   test_must_fail env FOO=BAR some-program
>
> But that only works because test_must_fail's arguments are
> themselves a command which can be run. You can't run:

We can do "test_must_fail test_commit ...", but "test_must_fail env
FOO=BAR test_commit ..." would not work, right?

If so s/because/when/, perhaps?

Re: [PATCH] t4014: shell portability fix

[Jeff King]

On Wed, Jun 01, 2016 at 08:07:16AM -0700, Junio C Hamano wrote:

> > Here's the patch I wrote up earlier (but was too timid to send out after
> > my barrage of emails :) ).
> 
> Looks very sensible.  I'll drop all these "Attempt to test with
> ksh93 found these breakages" patches and queue this one.

Curious based on our previous discussion, I applied your patches and did
a "make SHELL_PATH=/bin/ksh93 test". There were only a handful of
failures remaining. Some were definitely the "../.git" thing (which
Andreas earlier reported as fixed, though the fix still has not made it
into the version in Debian), but some were just puzzling. I shrugged and
gave up.

> > We sometimes get around this by using env, like:
> >
> >   test_must_fail env FOO=BAR some-program
> >
> > But that only works because test_must_fail's arguments are
> > themselves a command which can be run. You can't run:
> 
> We can do "test_must_fail test_commit ...", but "test_must_fail env
> FOO=BAR test_commit ..." would not work, right?
> 
> If so s/because/when/, perhaps?

Right. What I was trying to say is that it works in this case because
test_must_fail further executes its arguments, which gives us an
opportunity to put the "env" on the right-hand side of the function
call. I'm not sure s/because/when/ makes that any better, though.

Maybe:

    We sometimes get around this by using env, like:

      test_must_fail env FOO=BAR some-program

    But that works for test_must_fail because it further runs its
    arguments via the shell, so we can stick the "env" on the right-hand
    side of the function. It would not work to do:

      env FOO=BAR test_must_fail some-program

    because env does not know about our shell functions...

is more clear?

-Peff

Re: [PATCH] t4014: shell portability fix

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Maybe:
>
>     We sometimes get around this by using env, like:
>
>       test_must_fail env FOO=BAR some-program
>
>     But that works for test_must_fail because it further runs its
>     arguments via the shell, so we can stick the "env" on the right-hand
>     side of the function. It would not work to do:
>
>       env FOO=BAR test_must_fail some-program
>
>     because env does not know about our shell functions...
>
> is more clear?

I don't know.  What I wanted to say was that "test_must_fail env"
pattern works _only_ when some-program is not a shell function, even
though "test_must_fail some-program" itself without env is OK when
some-program is a shell function.

Re: [PATCH] t4014: shell portability fix

[Jeff King]

On Wed, Jun 01, 2016 at 09:57:06AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Maybe:
> >
> >     We sometimes get around this by using env, like:
> >
> >       test_must_fail env FOO=BAR some-program
> >
> >     But that works for test_must_fail because it further runs its
> >     arguments via the shell, so we can stick the "env" on the right-hand
> >     side of the function. It would not work to do:
> >
> >       env FOO=BAR test_must_fail some-program
> >
> >     because env does not know about our shell functions...
> >
> > is more clear?
> 
> I don't know.  What I wanted to say was that "test_must_fail env"
> pattern works _only_ when some-program is not a shell function, even
> though "test_must_fail some-program" itself without env is OK when
> some-program is a shell function.

Right, but I think that is taking it to a meta-level. We are already
talking about one shell function, test_must_fail versus test_commit.
Introducing another one in the test_must_fail to the right of "env"
obviously does not work, but that is independent of whether
test_must_fail is in use.

-Peff