Re: [PATCH 1/2] tests: fix non-portable "${var:-"str"}" construct

2018-08-29 Thread Ann T Ropea
Ævar Arnfjörð Bjarmason  writes:

> Fix this by removing the redundant quotes. There's no need for them,
> and the resulting code works under all the aforementioned shells. This
> fixes a regression in c2f1d3989 ("t4013: test new output from diff
> --abbrev --raw", 2017-12-03) first released with Git v2.16.0.

Acked-by: Ann T Ropea 

Thanks.


Re: [PATCH 1/2] tests: fix non-portable "${var:-"str"}" construct

2018-08-28 Thread Junio C Hamano
Ævar Arnfjörð Bjarmason   writes:

> On both AIX 7200-00-01-1543 and FreeBSD 11.2-RELEASE-p2 the
> "${var:-"str"}" syntax means something different than what it does
> under the bash or dash shells.
>
> Both will consider the start of the new unescaped quotes to be a new
> argument to test_expect_success, resulting in the following error:
>
> error: bug in the test script: 'git diff-tree initial # magic
> is (not' does not look like a prereq
>
> Fix this by removing the redundant quotes. There's no need for them,
> and the resulting code works under all the aforementioned shells.

Yup, there is no need for that inner dq pair in this particular case.

I was more worried about scripted Porcelains, like 

   : "${GIT_OBJECT_DIRECTORY="$(git rev-parse --git-path objects)"}"

which I think can safely lose the outer dq pair.  In t/ directory,
there is this one in test-lib-functions.sh which I do not offhand
know how these problematic shells would handle.

echo "#!${2-"$SHELL_PATH"}" &&

Other than the presence of these two that are not covered by this
patch, the patch itself looks good.  Thanks.

> fixes a regression in c2f1d3989 ("t4013: test new output from diff
> --abbrev --raw", 2017-12-03) first released with Git v2.16.0.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason 
> ---
>  t/t4013-diff-various.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
> index f8d853595b..73f7038253 100755
> --- a/t/t4013-diff-various.sh
> +++ b/t/t4013-diff-various.sh
> @@ -140,7 +140,7 @@ do
>   expect="$TEST_DIRECTORY/t4013/diff.$test"
>   actual="$pfx-diff.$test"
>  
> - test_expect_success "git $cmd # magic is ${magic:-"(not used)"}" '
> + test_expect_success "git $cmd # magic is ${magic:-(not used)}" '
>   {
>   echo "$ git $cmd"
>   case "$magic" in