* Please pull the patch series "use the $( ... ) construct for command substitution"
@ 2014-05-14 15:23 Elia Pinto
2014-05-14 17:14 ` Matthieu Moy
0 siblings, 1 reply; 4+ messages in thread
From: Elia Pinto @ 2014-05-14 15:23 UTC (permalink / raw)
To: git@vger.kernel.org; +Cc: Matthieu Moy
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
for you to fetch changes up to 8c8883150c391fae33c122228af937594142600e:
test-lib-functions.sh: use the $( ... ) construct for command
substitution (2014-05-14 05:34:52 -0700)
----------------------------------------------------------------
I have re-created a branch with these patches based on a previous
observation made here
http://www.spinics.net/lists/git/msg230236.html
Thanks very much to the people (Matthieu i think)
that will make the reviews, I understand it's boring
----------------------------------------------------------------
Elia Pinto (83):
t5003-archive-zip.sh: use the $( ... ) construct for command substitution
t5517-push-mirror.sh: use the $( ... ) construct for command substitution
t6002-rev-list-bisect.sh: use the $( ... ) construct for command
substitution
t7700-repack.sh: use the $( ... ) construct for command substitution
t5100-mailinfo.sh: use the $( ... ) construct for command substitution
t5520-pull.sh: use the $( ... ) construct for command substitution
t6015-rev-list-show-all-parents.sh: use the $( ... ) construct
for command substitution
t8003-blame-corner-cases.sh: use the $( ... ) construct for
command substitution
t5300-pack-object.sh: use the $( ... ) construct for command substitution
t5522-pull-symlink.sh: use the $( ... ) construct for command substitution
t6032-merge-large-rename.sh: use the $( ... ) construct for
command substitution
t5301-sliding-window.sh: use the $( ... ) construct for command
substitution
t5530-upload-pack-error.sh: use the $( ... ) construct for
command substitution
t6034-merge-rename-nocruft.sh: use the $( ... ) construct for
command substitution
t9100-git-svn-basic.sh: use the $( ... ) construct for command
substitution
t5302-pack-index.sh: use the $( ... ) construct for command substitution
t5537-fetch-shallow.sh: use the $( ... ) construct for command
substitution
t6132-pathspec-exclude.sh: use the $( ... ) construct for
command substitution
t9101-git-svn-props.sh: use the $( ... ) construct for command
substitution
t5303-pack-corruption-resilience.sh: use the $( ... ) construct
for command substitution
t5538-push-shallow.sh: use the $( ... ) construct for command substitution
t7001-mv.sh: use the $( ... ) construct for command substitution
t9104-git-svn-follow-parent.sh: use the $( ... ) construct for
command substitution
t5304-prune.sh: use the $( ... ) construct for command substitution
t5550-http-fetch-dumb.sh: use the $( ... ) construct for command
substitution
t7003-filter-branch.sh: use the $( ... ) construct for command
substitution
t9105-git-svn-commit-diff.sh: use the $( ... ) construct for
command substitution
t5305-include-tag.sh: use the $( ... ) construct for command substitution
t5551-http-fetch-smart.sh: use the $( ... ) construct for
command substitution
t7004-tag.sh: use the $( ... ) construct for command substitution
t9107-git-svn-migrate.sh: use the $( ... ) construct for command
substitution
t5500-fetch-pack.sh: use the $( ... ) construct for command substitution
t5570-git-daemon.sh: use the $( ... ) construct for command substitution
t7006-pager.sh: use the $( ... ) construct for command substitution
t9108-git-svn-glob.sh: use the $( ... ) construct for command substitution
t5505-remote.sh: use the $( ... ) construct for command substitution
t5601-clone.sh: use the $( ... ) construct for command substitution
t7103-reset-bare.sh: use the $( ... ) construct for command substitution
t9109-git-svn-multi-glob.sh: use the $( ... ) construct for
command substitution
t5506-remote-groups.sh: use the $( ... ) construct for command
substitution
t5700-clone-reference.sh: use the $( ... ) construct for command
substitution
t7406-submodule-update.sh: use the $( ... ) construct for
command substitution
t9110-git-svn-use-svm-props.sh: use the $( ... ) construct for
command substitution
t5510-fetch.sh: use the $( ... ) construct for command substitution
t5710-info-alternate.sh: use the $( ... ) construct for command
substitution
t7408-submodule-reference.sh: use the $( ... ) construct for
command substitution
t9114-git-svn-dcommit-merge.sh: use the $( ... ) construct for
command substitution
t5515-fetch-merge-logic.sh: use the $( ... ) construct for
command substitution
t5900-repo-selection.sh: use the $( ... ) construct for command
substitution
t7504-commit-msg-hook.sh: use the $( ... ) construct for command
substitution
t5516-fetch-push.sh: use the $( ... ) construct for command substitution
t6001-rev-list-graft.sh: use the $( ... ) construct for command
substitution
t7602-merge-octopus-many.sh: use the $( ... ) construct for
command substitution
unimplemented.sh: use the $( ... ) construct for command substitution
t1100-commit-tree-options.sh: use the $( ... ) construct for
command substitution
t1401-symbolic-ref.sh: use the $( ... ) construct for command substitution
t1410-reflog.sh: use the $( ... ) construct for command substitution
t1511-rev-parse-caret.sh: use the $( ... ) construct for command
substitution
t1512-rev-parse-disambiguation.sh: use the $( ... ) construct
for command substitution
t2102-update-index-symlinks.sh: use the $( ... ) construct for
command substitution
t3030-merge-recursive.sh: use the $( ... ) construct for command
substitution
t3100-ls-tree-restrict.sh: use the $( ... ) construct for
command substitution
t3101-ls-tree-dirname.sh: use the $( ... ) construct for command
substitution
t3210-pack-refs.sh: use the $( ... ) construct for command substitution
t3403-rebase-skip.sh: use the $( ... ) construct for command substitution
t3511-cherry-pick-x.sh: use the $( ... ) construct for command
substitution
t3600-rm.sh: use the $( ... ) construct for command substitution
t3700-add.sh: use the $( ... ) construct for command substitution
t7505-prepare-commit-msg-hook.sh: use the $( ... ) construct for
command substitution
t9118-git-svn-funky-branch-names.sh: use the $( ... ) construct
for command substitution
t9119-git-svn-info.sh: use the $( ... ) construct for command substitution
t9129-git-svn-i18n-commitencoding.sh: use the $( ... ) construct
for command substitution
t9130-git-svn-authors-file.sh: use the $( ... ) construct for
command substitution
t9132-git-svn-broken-symlink.sh: use the $( ... ) construct for
command substitution
t9137-git-svn-dcommit-clobber-series.sh: use the $( ... )
construct for command substitution
t9138-git-svn-authors-prog.sh: use the $( ... ) construct for
command substitution
t9145-git-svn-master-branch.sh: use the $( ... ) construct for
command substitution
t9150-svk-mergetickets.sh: use the $( ... ) construct for
command substitution
t9300-fast-import.sh: use the $( ... ) construct for command substitution
t9350-fast-export.sh: use the $( ... ) construct for command substitution
t9501-gitweb-standalone-http-status.sh: use the $( ... )
construct for command substitution
t9901-git-web--browse.sh: use the $( ... ) construct for command
substitution
test-lib-functions.sh: use the $( ... ) construct for command substitution
t/t1100-commit-tree-options.sh | 4 ++--
t/t1401-symbolic-ref.sh | 2 +-
t/t1410-reflog.sh | 24 ++++++++++++------------
t/t1511-rev-parse-caret.sh | 4 ++--
t/t1512-rev-parse-disambiguation.sh | 8 ++++----
t/t2102-update-index-symlinks.sh | 2 +-
t/t3030-merge-recursive.sh | 2 +-
t/t3100-ls-tree-restrict.sh | 2 +-
t/t3101-ls-tree-dirname.sh | 2 +-
t/t3210-pack-refs.sh | 2 +-
t/t3403-rebase-skip.sh | 2 +-
t/t3511-cherry-pick-x.sh | 14 +++++++-------
t/t3600-rm.sh | 4 ++--
t/t3700-add.sh | 16 ++++++++--------
t/t5003-archive-zip.sh | 2 +-
t/t5100-mailinfo.sh | 12 ++++++------
t/t5300-pack-object.sh | 18 +++++++++---------
t/t5301-sliding-window.sh | 14 +++++++-------
t/t5302-pack-index.sh | 34
+++++++++++++++++-----------------
t/t5303-pack-corruption-resilience.sh | 8 ++++----
t/t5304-prune.sh | 2 +-
t/t5305-include-tag.sh | 8 ++++----
t/t5500-fetch-pack.sh | 16 ++++++++--------
t/t5505-remote.sh | 2 +-
t/t5506-remote-groups.sh | 2 +-
t/t5510-fetch.sh | 10 +++++-----
t/t5515-fetch-merge-logic.sh | 4 ++--
t/t5516-fetch-push.sh | 4 ++--
t/t5517-push-mirror.sh | 2 +-
t/t5520-pull.sh | 10 +++++-----
t/t5522-pull-symlink.sh | 2 +-
t/t5530-upload-pack-error.sh | 2 +-
t/t5537-fetch-shallow.sh | 4 ++--
t/t5538-push-shallow.sh | 4 ++--
t/t5550-http-fetch-dumb.sh | 8 ++++----
t/t5551-http-fetch-smart.sh | 2 +-
t/t5570-git-daemon.sh | 8 ++++----
t/t5601-clone.sh | 2 +-
t/t5700-clone-reference.sh | 2 +-
t/t5710-info-alternate.sh | 2 +-
t/t5900-repo-selection.sh | 2 +-
t/t6001-rev-list-graft.sh | 12 ++++++------
t/t6002-rev-list-bisect.sh | 6 +++---
t/t6015-rev-list-show-all-parents.sh | 6 +++---
t/t6032-merge-large-rename.sh | 2 +-
t/t6034-merge-rename-nocruft.sh | 2 +-
t/t6132-pathspec-exclude.sh | 2 +-
t/t7001-mv.sh | 4 ++--
t/t7003-filter-branch.sh | 6 +++---
t/t7004-tag.sh | 16 ++++++++--------
t/t7006-pager.sh | 2 +-
t/t7103-reset-bare.sh | 2 +-
t/t7406-submodule-update.sh | 4 ++--
t/t7408-submodule-reference.sh | 2 +-
t/t7504-commit-msg-hook.sh | 2 +-
t/t7505-prepare-commit-msg-hook.sh | 32
++++++++++++++++----------------
t/t7602-merge-octopus-many.sh | 8 ++++----
t/t7700-repack.sh | 4 ++--
t/t8003-blame-corner-cases.sh | 4 ++--
t/t9100-git-svn-basic.sh | 24 ++++++++++++------------
t/t9101-git-svn-props.sh | 30 +++++++++++++++---------------
t/t9104-git-svn-follow-parent.sh | 48
++++++++++++++++++++++++------------------------
t/t9105-git-svn-commit-diff.sh | 4 ++--
t/t9107-git-svn-migrate.sh | 16 ++++++++--------
t/t9108-git-svn-glob.sh | 20 ++++++++++----------
t/t9109-git-svn-multi-glob.sh | 32
++++++++++++++++----------------
t/t9110-git-svn-use-svm-props.sh | 2 +-
t/t9114-git-svn-dcommit-merge.sh | 12 ++++++------
t/t9118-git-svn-funky-branch-names.sh | 2 +-
t/t9119-git-svn-info.sh | 2 +-
t/t9129-git-svn-i18n-commitencoding.sh | 4 ++--
t/t9130-git-svn-authors-file.sh | 12 ++++++------
t/t9132-git-svn-broken-symlink.sh | 4 ++--
t/t9137-git-svn-dcommit-clobber-series.sh | 24 ++++++++++++------------
t/t9138-git-svn-authors-prog.sh | 2 +-
t/t9145-git-svn-master-branch.sh | 4 ++--
t/t9150-svk-mergetickets.sh | 2 +-
t/t9300-fast-import.sh | 68
++++++++++++++++++++++++++++++++++----------------------------------
t/t9350-fast-export.sh | 6 +++---
t/t9501-gitweb-standalone-http-status.sh | 6 +++---
t/t9901-git-web--browse.sh | 2 +-
t/test-lib-functions.sh | 8 ++++----
unimplemented.sh | 2 +-
83 files changed, 363 insertions(+), 363 deletions(-)
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: Please pull the patch series "use the $( ... ) construct for command substitution" 2014-05-14 15:23 Please pull the patch series "use the $( ... ) construct for command substitution" Elia Pinto @ 2014-05-14 17:14 ` Matthieu Moy 2014-05-14 21:21 ` Eric Sunshine 0 siblings, 1 reply; 4+ messages in thread From: Matthieu Moy @ 2014-05-14 17:14 UTC (permalink / raw) To: Elia Pinto; +Cc: git@vger.kernel.org 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 correct code should be test x\"$(sed -n -e 4p < file)\" = x4 && I guess. 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/ ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: Please pull the patch series "use the $( ... ) construct for command substitution" 2014-05-14 17:14 ` Matthieu Moy @ 2014-05-14 21:21 ` Eric Sunshine 2014-05-15 7:45 ` Matthieu Moy 0 siblings, 1 reply; 4+ messages in thread From: Eric Sunshine @ 2014-05-14 21:21 UTC (permalink / raw) To: Matthieu Moy; +Cc: Elia Pinto, git@vger.kernel.org On Wed, May 14, 2014 at 1:14 PM, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote: > 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. > 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\$/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/ ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Please pull the patch series "use the $( ... ) construct for command substitution" 2014-05-14 21:21 ` Eric Sunshine @ 2014-05-15 7:45 ` Matthieu Moy 0 siblings, 0 replies; 4+ messages in thread From: Matthieu Moy @ 2014-05-15 7:45 UTC (permalink / raw) To: Eric Sunshine; +Cc: Elia Pinto, git@vger.kernel.org Eric Sunshine <sunshine@sunshineco.com> 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/ ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-05-15 7:45 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-05-14 15:23 Please pull the patch series "use the $( ... ) construct for command substitution" Elia Pinto 2014-05-14 17:14 ` Matthieu Moy 2014-05-14 21:21 ` Eric Sunshine 2014-05-15 7:45 ` Matthieu Moy
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.