Re: Please pull the patch series "use the $( ... ) construct for command substitution"
Eric Sunshine writes: > On Wed, May 14, 2014 at 1:14 PM, Matthieu Moy > >> 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. Ah, of course, you're right. >> 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. OK, then the correct code should be test x\"\$(sed -n -e 4p < file)\" = x4 && And the other attempt was close: > > 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\) && \-escaping the $ was right, but the \) should be a single ). In any case, the fact that we need to discuss this supports my claim "this should be reviewed as a separate series". -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Please pull the patch series "use the $( ... ) construct for command substitution"
On Wed, May 14, 2014 at 1:14 PM, Matthieu Moy wrote: > Elia Pinto 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 > 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. > 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. > perl -i.bak -p -e 's/^4\$//' file && > perl -i.bak -p -e 's/^7\$//' file && > - test x\"\`sed -n -e 4p < file\`\" = x && > - test x\"\`sed -n -e 7p < file\`\" = x && > + test x"$(sed -n -e 4p < file)" = x && > + test x"$(sed -n -e 7p < file)" = x && > > Likewise. > > - test x\"\`sed -n -e 4p < file\`\" = x && > - test x\"\`sed -n -e 7p < file\`\" = x && > - test x\"\`sed -n -e 58p < file\`\" = x5588 && > - test x\"\`sed -n -e 61p < file\`\" = x6611 > + test x"$(sed -n -e 4p < file)" = x && > + test x"$(sed -n -e 7p < file)" = x && > + 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 > 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 --veri
Re: Please pull the patch series "use the $( ... ) construct for command substitution"
Elia Pinto 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 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 correct code should be test x\"$(sed -n -e 4p < file)\" = x4 && I guess. perl -i.bak -p -e 's/^4\$//' file && perl -i.bak -p -e 's/^7\$//' file && - test x\"\`sed -n -e 4p < file\`\" = x && - test x\"\`sed -n -e 7p < file\`\" = x && + test x"$(sed -n -e 4p < file)" = x && + test x"$(sed -n -e 7p < file)" = x && Likewise. - test x\"\`sed -n -e 4p < file\`\" = x && - test x\"\`sed -n -e 7p < file\`\" = x && - test x\"\`sed -n -e 58p < file\`\" = x5588 && - test x\"\`sed -n -e 61p < file\`\" = x6611 + test x"$(sed -n -e 4p < file)" = x && + test x"$(sed -n -e 7p < file)" = x && + 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 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 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 seri
