Re: Please pull the patch series "use the $( ... ) construct for command substitution"

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 14, 2014, 21:21 UTC
Message-ID: <CAPig+cT-Nn_hnqE0CK=c0-9+_QG9hpsp9j4cERsvvjwkrLmSMw@mail.gmail.com>
In-Reply-To: <vpqoaz0i8od.fsf@anie.imag.fr>

On Wed, May 14, 2014 at 1:14 PM, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

Show 44 quoted lines

> Elia Pinto <gitter.spiros@gmail.com> writes:
>
>> The following changes since commit 6308767f0bb58116cb405e1f4f77f5dfc1589920:
>>   Merge branch 'fc/prompt-zsh-read-from-file' (2014-05-13 11:53:14 -0700)
>> are available in the git repository at:
>>   https://github.com/devzero2000/git-core.git  ep/shell-command-substitution-v4
>
> There's a mis-replacement of multiple `..` `..` on the same line in
> t9300-fast-import.sh. I've sent you a pull request with a fixup.
>
> I'm not sure about this one:
>
> commit e69c77e580d56d587381066f56027c8a596c237a
> Author: Elia Pinto <gitter.spiros@gmail.com>
> Date:   Wed May 14 03:28:11 2014 -0700
>
>     t9137-git-svn-dcommit-clobber-series.sh: use the $( ... ) construct for command substitution
> [...]
> @@ -38,20 +38,20 @@ test_expect_success 'some unrelated changes to git' "
>         "
>
>  test_expect_success 'change file but in unrelated area' "
> -       test x\"\`sed -n -e 4p < file\`\" = x4 &&
> -       test x\"\`sed -n -e 7p < file\`\" = x7 &&
> +       test x"$(sed -n -e 4p < file)" = x4 &&
> +       test x"$(sed -n -e 7p < file)" = x7 &&
>               ^
>             here
>
> We're inside " from the test_expect_success line. We used to have a
> literal " (because of the backslash), we now have a closing " because
> you removed the \. So, the sed command used to be protected by
> double-quotes, and it is now outside them. Compare:
>
> $ sh -c "echo \"\`date\`\""
> Wed May 14 18:47:54 MEST 2014
> $ sh -c "echo "$(date)""
> Wed
>
> In your case, it doesn't break because the expected output of sed
> contains no space, but that seems dangerous to me.
>
> I do not understand the use of the \ in front of the ` in the original
> code.

The second argument of test_expect_success is double-quoted, so a bare `...` would be evaluated before test_expect_success is even invoked. Quoting it as \'...\' correctly suppresses the automatic evaluation, giving test_expect_success the opportunity to evaluate it on-demand.

In this case, it doesn't matter since "file" is populated outside the invocation of test_expect_success, but it would matter if "file" was populated or modified within the test itself. In that sense, the original code with delayed \`...\` evaluation is more robust and future-proof.

Show 5 quoted lines

> The correct code should be
>
>         test x\"$(sed -n -e 4p < file)\" = x4 &&
>
> I guess.

Same issue. The $(...) is being evaluated even before test_expect_success is invoked. Better would be to suspend evaluation via \$(...) to allow test_expect_success to evaluate it on-demand.

Show 129 quoted lines

>         perl -i.bak -p -e 's/^4\$/4444/' file &&
>         perl -i.bak -p -e 's/^7\$/7777/' file &&
> -       test x\"\`sed -n -e 4p < file\`\" = x4444 &&
> -       test x\"\`sed -n -e 7p < file\`\" = x7777 &&
> +       test x"$(sed -n -e 4p < file)" = x4444 &&
> +       test x"$(sed -n -e 7p < file)" = x7777 &&
>
> Likewise.
>
> -               test x\"\`sed -n -e 4p < file\`\" = x4444 &&
> -               test x\"\`sed -n -e 7p < file\`\" = x7777 &&
> -               test x\"\`sed -n -e 58p < file\`\" = x5588 &&
> -               test x\"\`sed -n -e 61p < file\`\" = x6611
> +               test x"$(sed -n -e 4p < file)" = x4444 &&
> +               test x"$(sed -n -e 7p < file)" = x7777 &&
> +               test x"$(sed -n -e 58p < file)" = x5588 &&
> +               test x"$(sed -n -e 61p < file)" = x6611
>
> Likewise.
>
>
> More or less the same issue with
>
> commit 020568b9c36c023810a3482b7b73bcadd6406a85
> Author: Elia Pinto <gitter.spiros@gmail.com>
> Date:   Mon Apr 28 05:49:50 2014 -0700
>
>     t9114-git-svn-dcommit-merge.sh: use the $( ... ) construct for command substitution
> [...]
> diff --git a/t/t9114-git-svn-dcommit-merge.sh b/t/t9114-git-svn-dcommit-merge.sh
> index fb41876..cf2e25f 100755
> --- a/t/t9114-git-svn-dcommit-merge.sh
> +++ b/t/t9114-git-svn-dcommit-merge.sh
> @@ -68,8 +68,8 @@ test_expect_success 'setup git mirror and merge' '
>  test_debug 'gitk --all & sleep 1'
>
>  test_expect_success 'verify pre-merge ancestry' "
> -       test x\`git rev-parse --verify refs/heads/svn^2\` = \
> -            x\`git rev-parse --verify refs/heads/merge\` &&
> +       test x\$(git rev-parse --verify refs/heads/svn^2\) = \
> +            x\$(git rev-parse --verify refs/heads/merge\) &&
>         git cat-file commit refs/heads/svn^ | grep '^friend$'
>         "
>
> I'm not sure what's the intent of the \ in front of ` in the original
> code, but changing it to $(...) changes the meaning:
>
> $ sh -c "echo \`date\`"
> Wed May 14 18:58:19 MEST 2014
> $ sh -c "echo \$(date\)"
> sh: 1: Syntax error: end of file unexpected (expecting ")")
>
> I didn't investigate closely, but I'm getting test failures without your
> patch, and the script stops in the middle with it so it does break
> something.
>
> @@ -80,10 +80,10 @@ test_expect_success 'git svn dcommit merges' "
>  test_debug 'gitk --all & sleep 1'
>
>  test_expect_success 'verify post-merge ancestry' "
> -       test x\`git rev-parse --verify refs/heads/svn\` = \
> -            x\`git rev-parse --verify refs/remotes/origin/trunk \` &&
> -       test x\`git rev-parse --verify refs/heads/svn^2\` = \
> -            x\`git rev-parse --verify refs/heads/merge\` &&
> +       test x\$(git rev-parse --verify refs/heads/svn\) = \
> +            x\$(git rev-parse --verify refs/remotes/origin/trunk \) &&
> +       test x\$(git rev-parse --verify refs/heads/svn^2\) = \
> +            x\$(git rev-parse --verify refs/heads/merge\) &&
>
> Likewise.
>
>
> commit 7e29ac501ce24aa5af3a50f839cd3ad176481a96
> Author: Elia Pinto <gitter.spiros@gmail.com>
> Date:   Wed Mar 26 04:48:40 2014 -0700
>
>     t9100-git-svn-basic.sh: use the $( ... ) construct for command substitution
>
> -test_expect_success 'able to dcommit to a subdirectory' "
> +test_expect_success 'able to dcommit to a subdirectory' '
>
> There is an actual change other than sed + review and trivial fix here.
> That makes the review harder. Such change should IMHO not be part of the
> same series.
>
> -       git commit -m '/bar/d should be in the log' &&
> +       git commit -m "bar/d should be in the log" &&
>
> You lost a / here.
>
>         git svn dcommit -i bar &&
> -       test -z \"\`git diff refs/heads/my-bar refs/remotes/bar\`\" &&
> +       test -z $(git diff refs/heads/my-bar refs/remotes/bar) &&
>
> Did you not loos the \"...\" whitespace protection here?
>
> -       test -z \"\`git diff refs/heads/my-bar refs/remotes/bar\`\" &&
> +       test -z "$(git diff refs/heads/my-bar refs/remotes/bar)" &&
>
> That seems to be the correct way of doing what you tried right above.
>
> commit b438455b7b97d90a1b8da4ec4e9de0063c20f63c
> Author: Elia Pinto <gitter.spiros@gmail.com>
> Date:   Wed Mar 26 04:48:40 2014 -0700
>
>     t9107-git-svn-migrate.sh: use the $( ... ) construct for command substitution
>
> [...]
>
> @@ -75,7 +75,7 @@ test_expect_success 'multi-fetch works on partial urls + paths' "
>         for i in trunk a b tags/0.1 tags/0.2 tags/0.3; do
>                 git rev-parse --verify refs/remotes/origin/\$i^0 >> refs.out || exit 1;
>             done &&
> -       test -z \"\`sort < refs.out | uniq -d\`\" &&
> +       test -z \"\$(sort < refs.out | uniq -d\)\" &&
>
> Same problem as above, this \$( is broken.
>
> My advice: apply my small fix for multiple `...` `...`, and eject the
> other patches from the series for now, they are distracting reviewers.
>
> That should lead to a trivially-correct series with ~80 patches. Once
> this one is accepted, the 4 remaining patches can be fixed and reviewed
> more carefully (Cc Eric Wong <normalperson@yhbt.net> since the patches
> are about git-svn).
>
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/