{"thread":{"id":"36654","subject":"Please pull the patch series \"use the $( ... ) construct for command substitution\"","startedAt":"2014-05-14T15:23:05Z","lastAt":"2014-05-15T07:45:22Z","messageCount":4,"participants":["Elia Pinto","Matthieu Moy","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"241504","messageId":"CA+EOSBk4YvQHTG=gRd1TF9gX0OgjLpjRidh7NAa9wmjr6bSkBQ@mail.gmail.com","threadId":"36654","inReplyTo":null,"subject":"Please pull the patch series \"use the $( ... ) construct for command substitution\"","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2014-05-14T15:23:05Z","receivedAt":"2014-05-14T15:23:05Z","isPatch":false,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"The following changes since commit 6308767f0bb58116cb405e1f4f77f5dfc1589920:\n\n\n  Merge branch 'fc/prompt-zsh-read-from-file' (2014-05-13 11:53:14 -0700)\n\n\nare available in the git repository at:\n\n\n  https://github.com/devzero2000/git-core.git  ep/shell-command-substitution-v4\n\n\nfor you to fetch changes up to 8c8883150c391fae33c122228af937594142600e:\n\n\n\n  test-lib-functions.sh: use the $( ... ) construct for command\nsubstitution (2014-05-14 05:34:52 -0700)\n\n----------------------------------------------------------------\n\nI have re-created a branch with these patches based on a previous\nobservation made here\n\nhttp://www.spinics.net/lists/git/msg230236.html\n\nThanks very much to the people (Matthieu i think)\nthat will make the reviews, I understand it's boring\n\n----------------------------------------------------------------\n\nElia Pinto (83):\n\n      t5003-archive-zip.sh: use the $( ... ) construct for command substitution\n\n      t5517-push-mirror.sh: use the $( ... ) construct for command substitution\n\n      t6002-rev-list-bisect.sh: use the $( ... ) construct for command\nsubstitution\n\n      t7700-repack.sh: use the $( ... ) construct for command substitution\n\n      t5100-mailinfo.sh: use the $( ... ) construct for command substitution\n\n      t5520-pull.sh: use the $( ... ) construct for command substitution\n\n      t6015-rev-list-show-all-parents.sh: use the $( ... ) construct\nfor command substitution\n\n      t8003-blame-corner-cases.sh: use the $( ... ) construct for\ncommand substitution\n\n      t5300-pack-object.sh: use the $( ... ) construct for command substitution\n\n      t5522-pull-symlink.sh: use the $( ... ) construct for command substitution\n\n      t6032-merge-large-rename.sh: use the $( ... ) construct for\ncommand substitution\n\n      t5301-sliding-window.sh: use the $( ... ) construct for command\nsubstitution\n\n      t5530-upload-pack-error.sh: use the $( ... ) construct for\ncommand substitution\n\n      t6034-merge-rename-nocruft.sh: use the $( ... ) construct for\ncommand substitution\n\n      t9100-git-svn-basic.sh: use the $( ... ) construct for command\nsubstitution\n\n      t5302-pack-index.sh: use the $( ... ) construct for command substitution\n\n      t5537-fetch-shallow.sh: use the $( ... ) construct for command\nsubstitution\n\n      t6132-pathspec-exclude.sh: use the $( ... ) construct for\ncommand substitution\n\n      t9101-git-svn-props.sh: use the $( ... ) construct for command\nsubstitution\n\n      t5303-pack-corruption-resilience.sh: use the $( ... ) construct\nfor command substitution\n\n      t5538-push-shallow.sh: use the $( ... ) construct for command substitution\n\n      t7001-mv.sh: use the $( ... ) construct for command substitution\n\n      t9104-git-svn-follow-parent.sh: use the $( ... ) construct for\ncommand substitution\n\n      t5304-prune.sh: use the $( ... ) construct for command substitution\n\n      t5550-http-fetch-dumb.sh: use the $( ... ) construct for command\nsubstitution\n\n      t7003-filter-branch.sh: use the $( ... ) construct for command\nsubstitution\n\n      t9105-git-svn-commit-diff.sh: use the $( ... ) construct for\ncommand substitution\n\n      t5305-include-tag.sh: use the $( ... ) construct for command substitution\n\n      t5551-http-fetch-smart.sh: use the $( ... ) construct for\ncommand substitution\n\n      t7004-tag.sh: use the $( ... ) construct for command substitution\n\n      t9107-git-svn-migrate.sh: use the $( ... ) construct for command\nsubstitution\n\n      t5500-fetch-pack.sh: use the $( ... ) construct for command substitution\n\n      t5570-git-daemon.sh: use the $( ... ) construct for command substitution\n\n      t7006-pager.sh: use the $( ... ) construct for command substitution\n\n      t9108-git-svn-glob.sh: use the $( ... ) construct for command substitution\n\n      t5505-remote.sh: use the $( ... ) construct for command substitution\n\n      t5601-clone.sh: use the $( ... ) construct for command substitution\n\n      t7103-reset-bare.sh: use the $( ... ) construct for command substitution\n\n      t9109-git-svn-multi-glob.sh: use the $( ... ) construct for\ncommand substitution\n\n      t5506-remote-groups.sh: use the $( ... ) construct for command\nsubstitution\n\n      t5700-clone-reference.sh: use the $( ... ) construct for command\nsubstitution\n\n      t7406-submodule-update.sh: use the $( ... ) construct for\ncommand substitution\n\n      t9110-git-svn-use-svm-props.sh: use the $( ... ) construct for\ncommand substitution\n\n      t5510-fetch.sh: use the $( ... ) construct for command substitution\n\n      t5710-info-alternate.sh: use the $( ... ) construct for command\nsubstitution\n\n      t7408-submodule-reference.sh: use the $( ... ) construct for\ncommand substitution\n\n      t9114-git-svn-dcommit-merge.sh: use the $( ... ) construct for\ncommand substitution\n\n      t5515-fetch-merge-logic.sh: use the $( ... ) construct for\ncommand substitution\n\n      t5900-repo-selection.sh: use the $( ... ) construct for command\nsubstitution\n\n      t7504-commit-msg-hook.sh: use the $( ... ) construct for command\nsubstitution\n\n      t5516-fetch-push.sh: use the $( ... ) construct for command substitution\n\n      t6001-rev-list-graft.sh: use the $( ... ) construct for command\nsubstitution\n\n      t7602-merge-octopus-many.sh: use the $( ... ) construct for\ncommand substitution\n\n      unimplemented.sh: use the $( ... ) construct for command substitution\n\n      t1100-commit-tree-options.sh: use the $( ... ) construct for\ncommand substitution\n\n      t1401-symbolic-ref.sh: use the $( ... ) construct for command substitution\n\n      t1410-reflog.sh: use the $( ... ) construct for command substitution\n\n      t1511-rev-parse-caret.sh: use the $( ... ) construct for command\nsubstitution\n\n      t1512-rev-parse-disambiguation.sh: use the $( ... ) construct\nfor command substitution\n\n      t2102-update-index-symlinks.sh: use the $( ... ) construct for\ncommand substitution\n\n      t3030-merge-recursive.sh: use the $( ... ) construct for command\nsubstitution\n\n      t3100-ls-tree-restrict.sh: use the $( ... ) construct for\ncommand substitution\n\n      t3101-ls-tree-dirname.sh: use the $( ... ) construct for command\nsubstitution\n\n      t3210-pack-refs.sh: use the $( ... ) construct for command substitution\n\n      t3403-rebase-skip.sh: use the $( ... ) construct for command substitution\n\n      t3511-cherry-pick-x.sh: use the $( ... ) construct for command\nsubstitution\n\n      t3600-rm.sh: use the $( ... ) construct for command substitution\n\n      t3700-add.sh: use the $( ... ) construct for command substitution\n\n      t7505-prepare-commit-msg-hook.sh: use the $( ... ) construct for\ncommand substitution\n\n      t9118-git-svn-funky-branch-names.sh: use the $( ... ) construct\nfor command substitution\n\n      t9119-git-svn-info.sh: use the $( ... ) construct for command substitution\n\n      t9129-git-svn-i18n-commitencoding.sh: use the $( ... ) construct\nfor command substitution\n\n      t9130-git-svn-authors-file.sh: use the $( ... ) construct for\ncommand substitution\n\n      t9132-git-svn-broken-symlink.sh: use the $( ... ) construct for\ncommand substitution\n\n      t9137-git-svn-dcommit-clobber-series.sh: use the $( ... )\nconstruct for command substitution\n\n      t9138-git-svn-authors-prog.sh: use the $( ... ) construct for\ncommand substitution\n\n      t9145-git-svn-master-branch.sh: use the $( ... ) construct for\ncommand substitution\n\n      t9150-svk-mergetickets.sh: use the $( ... ) construct for\ncommand substitution\n\n      t9300-fast-import.sh: use the $( ... ) construct for command substitution\n\n      t9350-fast-export.sh: use the $( ... ) construct for command substitution\n\n      t9501-gitweb-standalone-http-status.sh: use the $( ... )\nconstruct for command substitution\n\n      t9901-git-web--browse.sh: use the $( ... ) construct for command\nsubstitution\n\n      test-lib-functions.sh: use the $( ... ) construct for command substitution\n\n\n\n t/t1100-commit-tree-options.sh            |    4 ++--\n\n t/t1401-symbolic-ref.sh                   |    2 +-\n\n t/t1410-reflog.sh                         |   24 ++++++++++++------------\n\n t/t1511-rev-parse-caret.sh                |    4 ++--\n\n t/t1512-rev-parse-disambiguation.sh       |    8 ++++----\n\n t/t2102-update-index-symlinks.sh          |    2 +-\n\n t/t3030-merge-recursive.sh                |    2 +-\n\n t/t3100-ls-tree-restrict.sh               |    2 +-\n\n t/t3101-ls-tree-dirname.sh                |    2 +-\n\n t/t3210-pack-refs.sh                      |    2 +-\n\n t/t3403-rebase-skip.sh                    |    2 +-\n\n t/t3511-cherry-pick-x.sh                  |   14 +++++++-------\n\n t/t3600-rm.sh                             |    4 ++--\n\n t/t3700-add.sh                            |   16 ++++++++--------\n\n t/t5003-archive-zip.sh                    |    2 +-\n\n t/t5100-mailinfo.sh                       |   12 ++++++------\n\n t/t5300-pack-object.sh                    |   18 +++++++++---------\n\n t/t5301-sliding-window.sh                 |   14 +++++++-------\n\n t/t5302-pack-index.sh                     |   34\n+++++++++++++++++-----------------\n\n t/t5303-pack-corruption-resilience.sh     |    8 ++++----\n\n t/t5304-prune.sh                          |    2 +-\n\n t/t5305-include-tag.sh                    |    8 ++++----\n\n t/t5500-fetch-pack.sh                     |   16 ++++++++--------\n\n t/t5505-remote.sh                         |    2 +-\n\n t/t5506-remote-groups.sh                  |    2 +-\n\n t/t5510-fetch.sh                          |   10 +++++-----\n\n t/t5515-fetch-merge-logic.sh              |    4 ++--\n\n t/t5516-fetch-push.sh                     |    4 ++--\n\n t/t5517-push-mirror.sh                    |    2 +-\n\n t/t5520-pull.sh                           |   10 +++++-----\n\n t/t5522-pull-symlink.sh                   |    2 +-\n\n t/t5530-upload-pack-error.sh              |    2 +-\n\n t/t5537-fetch-shallow.sh                  |    4 ++--\n\n t/t5538-push-shallow.sh                   |    4 ++--\n\n t/t5550-http-fetch-dumb.sh                |    8 ++++----\n\n t/t5551-http-fetch-smart.sh               |    2 +-\n\n t/t5570-git-daemon.sh                     |    8 ++++----\n\n t/t5601-clone.sh                          |    2 +-\n\n t/t5700-clone-reference.sh                |    2 +-\n\n t/t5710-info-alternate.sh                 |    2 +-\n\n t/t5900-repo-selection.sh                 |    2 +-\n\n t/t6001-rev-list-graft.sh                 |   12 ++++++------\n\n t/t6002-rev-list-bisect.sh                |    6 +++---\n\n t/t6015-rev-list-show-all-parents.sh      |    6 +++---\n\n t/t6032-merge-large-rename.sh             |    2 +-\n\n t/t6034-merge-rename-nocruft.sh           |    2 +-\n\n t/t6132-pathspec-exclude.sh               |    2 +-\n\n t/t7001-mv.sh                             |    4 ++--\n\n t/t7003-filter-branch.sh                  |    6 +++---\n\n t/t7004-tag.sh                            |   16 ++++++++--------\n\n t/t7006-pager.sh                          |    2 +-\n\n t/t7103-reset-bare.sh                     |    2 +-\n\n t/t7406-submodule-update.sh               |    4 ++--\n\n t/t7408-submodule-reference.sh            |    2 +-\n\n t/t7504-commit-msg-hook.sh                |    2 +-\n\n t/t7505-prepare-commit-msg-hook.sh        |   32\n++++++++++++++++----------------\n\n t/t7602-merge-octopus-many.sh             |    8 ++++----\n\n t/t7700-repack.sh                         |    4 ++--\n\n t/t8003-blame-corner-cases.sh             |    4 ++--\n\n t/t9100-git-svn-basic.sh                  |   24 ++++++++++++------------\n\n t/t9101-git-svn-props.sh                  |   30 +++++++++++++++---------------\n\n t/t9104-git-svn-follow-parent.sh          |   48\n++++++++++++++++++++++++------------------------\n\n t/t9105-git-svn-commit-diff.sh            |    4 ++--\n\n t/t9107-git-svn-migrate.sh                |   16 ++++++++--------\n\n t/t9108-git-svn-glob.sh                   |   20 ++++++++++----------\n\n t/t9109-git-svn-multi-glob.sh             |   32\n++++++++++++++++----------------\n\n t/t9110-git-svn-use-svm-props.sh          |    2 +-\n\n t/t9114-git-svn-dcommit-merge.sh          |   12 ++++++------\n\n t/t9118-git-svn-funky-branch-names.sh     |    2 +-\n\n t/t9119-git-svn-info.sh                   |    2 +-\n\n t/t9129-git-svn-i18n-commitencoding.sh    |    4 ++--\n\n t/t9130-git-svn-authors-file.sh           |   12 ++++++------\n\n t/t9132-git-svn-broken-symlink.sh         |    4 ++--\n\n t/t9137-git-svn-dcommit-clobber-series.sh |   24 ++++++++++++------------\n\n t/t9138-git-svn-authors-prog.sh           |    2 +-\n\n t/t9145-git-svn-master-branch.sh          |    4 ++--\n\n t/t9150-svk-mergetickets.sh               |    2 +-\n\n t/t9300-fast-import.sh                    |   68\n++++++++++++++++++++++++++++++++++----------------------------------\n\n t/t9350-fast-export.sh                    |    6 +++---\n\n t/t9501-gitweb-standalone-http-status.sh  |    6 +++---\n\n t/t9901-git-web--browse.sh                |    2 +-\n\n t/test-lib-functions.sh                   |    8 ++++----\n\n unimplemented.sh                          |    2 +-\n\n 83 files changed, 363 insertions(+), 363 deletions(-)\n"},{"id":"241512","messageId":"vpqoaz0i8od.fsf@anie.imag.fr","threadId":"36654","inReplyTo":"CA+EOSBk4YvQHTG=gRd1TF9gX0OgjLpjRidh7NAa9wmjr6bSkBQ@mail.gmail.com","subject":"Re: Please pull the patch series \"use the $( ... ) construct for command substitution\"","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-05-14T17:14:58Z","receivedAt":"2014-05-14T17:14:58Z","isPatch":false,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Elia Pinto <gitter.spiros@gmail.com> writes:\n\n> The following changes since commit 6308767f0bb58116cb405e1f4f77f5dfc1589920:\n>\n>\n>   Merge branch 'fc/prompt-zsh-read-from-file' (2014-05-13 11:53:14 -0700)\n>\n>\n> are available in the git repository at:\n>\n>\n>   https://github.com/devzero2000/git-core.git  ep/shell-command-substitution-v4\n\nThere's a mis-replacement of multiple `..` `..` on the same line in\nt9300-fast-import.sh. I've sent you a pull request with a fixup.\n\nI'm not sure about this one:\n\ncommit e69c77e580d56d587381066f56027c8a596c237a\nAuthor: Elia Pinto <gitter.spiros@gmail.com>\nDate:   Wed May 14 03:28:11 2014 -0700\n\n    t9137-git-svn-dcommit-clobber-series.sh: use the $( ... ) construct for command substitution\n[...]\n@@ -38,20 +38,20 @@ test_expect_success 'some unrelated changes to git' \"\n        \"\n \n test_expect_success 'change file but in unrelated area' \"\n-       test x\\\"\\`sed -n -e 4p < file\\`\\\" = x4 &&\n-       test x\\\"\\`sed -n -e 7p < file\\`\\\" = x7 &&\n+       test x\"$(sed -n -e 4p < file)\" = x4 &&\n+       test x\"$(sed -n -e 7p < file)\" = x7 &&\n              ^\n            here\n\nWe're inside \" from the test_expect_success line. We used to have a\nliteral \" (because of the backslash), we now have a closing \" because\nyou removed the \\. So, the sed command used to be protected by\ndouble-quotes, and it is now outside them. Compare:\n\n$ sh -c \"echo \\\"\\`date\\`\\\"\"                 \nWed May 14 18:47:54 MEST 2014\n$ sh -c \"echo \"$(date)\"\"                    \nWed\n\nIn your case, it doesn't break because the expected output of sed\ncontains no space, but that seems dangerous to me.\n\nI do not understand the use of the \\ in front of the ` in the original\ncode.\n\nThe correct code should be\n\n\ttest x\\\"$(sed -n -e 4p < file)\\\" = x4 &&\n\nI guess.\n\n        perl -i.bak -p -e 's/^4\\$/4444/' file &&\n        perl -i.bak -p -e 's/^7\\$/7777/' file &&\n-       test x\\\"\\`sed -n -e 4p < file\\`\\\" = x4444 &&\n-       test x\\\"\\`sed -n -e 7p < file\\`\\\" = x7777 &&\n+       test x\"$(sed -n -e 4p < file)\" = x4444 &&\n+       test x\"$(sed -n -e 7p < file)\" = x7777 &&\n\nLikewise.\n\n-               test x\\\"\\`sed -n -e 4p < file\\`\\\" = x4444 &&\n-               test x\\\"\\`sed -n -e 7p < file\\`\\\" = x7777 &&\n-               test x\\\"\\`sed -n -e 58p < file\\`\\\" = x5588 &&\n-               test x\\\"\\`sed -n -e 61p < file\\`\\\" = x6611\n+               test x\"$(sed -n -e 4p < file)\" = x4444 &&\n+               test x\"$(sed -n -e 7p < file)\" = x7777 &&\n+               test x\"$(sed -n -e 58p < file)\" = x5588 &&\n+               test x\"$(sed -n -e 61p < file)\" = x6611\n\nLikewise.\n\n\nMore or less the same issue with\n\ncommit 020568b9c36c023810a3482b7b73bcadd6406a85\nAuthor: Elia Pinto <gitter.spiros@gmail.com>\nDate:   Mon Apr 28 05:49:50 2014 -0700\n\n    t9114-git-svn-dcommit-merge.sh: use the $( ... ) construct for command substitution\n[...]\ndiff --git a/t/t9114-git-svn-dcommit-merge.sh b/t/t9114-git-svn-dcommit-merge.sh\nindex fb41876..cf2e25f 100755\n--- a/t/t9114-git-svn-dcommit-merge.sh\n+++ b/t/t9114-git-svn-dcommit-merge.sh\n@@ -68,8 +68,8 @@ test_expect_success 'setup git mirror and merge' '\n test_debug 'gitk --all & sleep 1'\n \n test_expect_success 'verify pre-merge ancestry' \"\n-       test x\\`git rev-parse --verify refs/heads/svn^2\\` = \\\n-            x\\`git rev-parse --verify refs/heads/merge\\` &&\n+       test x\\$(git rev-parse --verify refs/heads/svn^2\\) = \\\n+            x\\$(git rev-parse --verify refs/heads/merge\\) &&\n        git cat-file commit refs/heads/svn^ | grep '^friend$'\n        \"\n\nI'm not sure what's the intent of the \\ in front of ` in the original\ncode, but changing it to $(...) changes the meaning:\n\n$ sh -c \"echo \\`date\\`\" \nWed May 14 18:58:19 MEST 2014\n$ sh -c \"echo \\$(date\\)\"\nsh: 1: Syntax error: end of file unexpected (expecting \")\")\n\nI didn't investigate closely, but I'm getting test failures without your\npatch, and the script stops in the middle with it so it does break\nsomething.\n\n@@ -80,10 +80,10 @@ test_expect_success 'git svn dcommit merges' \"\n test_debug 'gitk --all & sleep 1'\n \n test_expect_success 'verify post-merge ancestry' \"\n-       test x\\`git rev-parse --verify refs/heads/svn\\` = \\\n-            x\\`git rev-parse --verify refs/remotes/origin/trunk \\` &&\n-       test x\\`git rev-parse --verify refs/heads/svn^2\\` = \\\n-            x\\`git rev-parse --verify refs/heads/merge\\` &&\n+       test x\\$(git rev-parse --verify refs/heads/svn\\) = \\\n+            x\\$(git rev-parse --verify refs/remotes/origin/trunk \\) &&\n+       test x\\$(git rev-parse --verify refs/heads/svn^2\\) = \\\n+            x\\$(git rev-parse --verify refs/heads/merge\\) &&\n\nLikewise.\n\n\ncommit 7e29ac501ce24aa5af3a50f839cd3ad176481a96\nAuthor: Elia Pinto <gitter.spiros@gmail.com>\nDate:   Wed Mar 26 04:48:40 2014 -0700\n\n    t9100-git-svn-basic.sh: use the $( ... ) construct for command substitution\n    \n-test_expect_success 'able to dcommit to a subdirectory' \"\n+test_expect_success 'able to dcommit to a subdirectory' '\n\nThere is an actual change other than sed + review and trivial fix here.\nThat makes the review harder. Such change should IMHO not be part of the\nsame series.\n\n-       git commit -m '/bar/d should be in the log' &&\n+       git commit -m \"bar/d should be in the log\" &&\n\nYou lost a / here.\n\n        git svn dcommit -i bar &&\n-       test -z \\\"\\`git diff refs/heads/my-bar refs/remotes/bar\\`\\\" &&\n+       test -z $(git diff refs/heads/my-bar refs/remotes/bar) &&\n\nDid you not loos the \\\"...\\\" whitespace protection here?\n\n-       test -z \\\"\\`git diff refs/heads/my-bar refs/remotes/bar\\`\\\" &&\n+       test -z \"$(git diff refs/heads/my-bar refs/remotes/bar)\" &&\n\nThat seems to be the correct way of doing what you tried right above.\n\ncommit b438455b7b97d90a1b8da4ec4e9de0063c20f63c\nAuthor: Elia Pinto <gitter.spiros@gmail.com>\nDate:   Wed Mar 26 04:48:40 2014 -0700\n\n    t9107-git-svn-migrate.sh: use the $( ... ) construct for command substitution\n\n[...]\n\n@@ -75,7 +75,7 @@ test_expect_success 'multi-fetch works on partial urls + paths' \"\n        for i in trunk a b tags/0.1 tags/0.2 tags/0.3; do\n                git rev-parse --verify refs/remotes/origin/\\$i^0 >> refs.out || exit 1;\n            done &&\n-       test -z \\\"\\`sort < refs.out | uniq -d\\`\\\" &&\n+       test -z \\\"\\$(sort < refs.out | uniq -d\\)\\\" &&\n\nSame problem as above, this \\$( is broken.\n\nMy advice: apply my small fix for multiple `...` `...`, and eject the\nother patches from the series for now, they are distracting reviewers.\n\nThat should lead to a trivially-correct series with ~80 patches. Once\nthis one is accepted, the 4 remaining patches can be fixed and reviewed\nmore carefully (Cc Eric Wong <normalperson@yhbt.net> since the patches\nare about git-svn).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"241573","messageId":"CAPig+cT-Nn_hnqE0CK=c0-9+_QG9hpsp9j4cERsvvjwkrLmSMw@mail.gmail.com","threadId":"36654","inReplyTo":"vpqoaz0i8od.fsf@anie.imag.fr","subject":"Re: Please pull the patch series \"use the $( ... ) construct for command substitution\"","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-05-14T21:21:00Z","receivedAt":"2014-05-14T21:21:00Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, May 14, 2014 at 1:14 PM, Matthieu Moy\n<Matthieu.Moy@grenoble-inp.fr> wrote:\n> Elia Pinto <gitter.spiros@gmail.com> writes:\n>\n>> The following changes since commit 6308767f0bb58116cb405e1f4f77f5dfc1589920:\n>>   Merge branch 'fc/prompt-zsh-read-from-file' (2014-05-13 11:53:14 -0700)\n>> are available in the git repository at:\n>>   https://github.com/devzero2000/git-core.git  ep/shell-command-substitution-v4\n>\n> There's a mis-replacement of multiple `..` `..` on the same line in\n> t9300-fast-import.sh. I've sent you a pull request with a fixup.\n>\n> I'm not sure about this one:\n>\n> commit e69c77e580d56d587381066f56027c8a596c237a\n> Author: Elia Pinto <gitter.spiros@gmail.com>\n> Date:   Wed May 14 03:28:11 2014 -0700\n>\n>     t9137-git-svn-dcommit-clobber-series.sh: use the $( ... ) construct for command substitution\n> [...]\n> @@ -38,20 +38,20 @@ test_expect_success 'some unrelated changes to git' \"\n>         \"\n>\n>  test_expect_success 'change file but in unrelated area' \"\n> -       test x\\\"\\`sed -n -e 4p < file\\`\\\" = x4 &&\n> -       test x\\\"\\`sed -n -e 7p < file\\`\\\" = x7 &&\n> +       test x\"$(sed -n -e 4p < file)\" = x4 &&\n> +       test x\"$(sed -n -e 7p < file)\" = x7 &&\n>               ^\n>             here\n>\n> We're inside \" from the test_expect_success line. We used to have a\n> literal \" (because of the backslash), we now have a closing \" because\n> you removed the \\. So, the sed command used to be protected by\n> double-quotes, and it is now outside them. Compare:\n>\n> $ sh -c \"echo \\\"\\`date\\`\\\"\"\n> Wed May 14 18:47:54 MEST 2014\n> $ sh -c \"echo \"$(date)\"\"\n> Wed\n>\n> In your case, it doesn't break because the expected output of sed\n> contains no space, but that seems dangerous to me.\n>\n> I do not understand the use of the \\ in front of the ` in the original\n> code.\n\nThe second argument of test_expect_success is double-quoted, so a bare\n`...` would be evaluated before test_expect_success is even invoked.\nQuoting it as \\'...\\' correctly suppresses the automatic evaluation,\ngiving test_expect_success the opportunity to evaluate it on-demand.\n\nIn this case, it doesn't matter since \"file\" is populated outside the\ninvocation of test_expect_success, but it would matter if \"file\" was\npopulated or modified within the test itself. In that sense, the\noriginal code with delayed \\`...\\` evaluation is more robust and\nfuture-proof.\n\n> The correct code should be\n>\n>         test x\\\"$(sed -n -e 4p < file)\\\" = x4 &&\n>\n> I guess.\n\nSame issue. The $(...) is being evaluated even before\ntest_expect_success is invoked. Better would be to suspend evaluation\nvia \\$(...) to allow test_expect_success to evaluate it on-demand.\n\n>         perl -i.bak -p -e 's/^4\\$/4444/' file &&\n>         perl -i.bak -p -e 's/^7\\$/7777/' file &&\n> -       test x\\\"\\`sed -n -e 4p < file\\`\\\" = x4444 &&\n> -       test x\\\"\\`sed -n -e 7p < file\\`\\\" = x7777 &&\n> +       test x\"$(sed -n -e 4p < file)\" = x4444 &&\n> +       test x\"$(sed -n -e 7p < file)\" = x7777 &&\n>\n> Likewise.\n>\n> -               test x\\\"\\`sed -n -e 4p < file\\`\\\" = x4444 &&\n> -               test x\\\"\\`sed -n -e 7p < file\\`\\\" = x7777 &&\n> -               test x\\\"\\`sed -n -e 58p < file\\`\\\" = x5588 &&\n> -               test x\\\"\\`sed -n -e 61p < file\\`\\\" = x6611\n> +               test x\"$(sed -n -e 4p < file)\" = x4444 &&\n> +               test x\"$(sed -n -e 7p < file)\" = x7777 &&\n> +               test x\"$(sed -n -e 58p < file)\" = x5588 &&\n> +               test x\"$(sed -n -e 61p < file)\" = x6611\n>\n> Likewise.\n>\n>\n> More or less the same issue with\n>\n> commit 020568b9c36c023810a3482b7b73bcadd6406a85\n> Author: Elia Pinto <gitter.spiros@gmail.com>\n> Date:   Mon Apr 28 05:49:50 2014 -0700\n>\n>     t9114-git-svn-dcommit-merge.sh: use the $( ... ) construct for command substitution\n> [...]\n> diff --git a/t/t9114-git-svn-dcommit-merge.sh b/t/t9114-git-svn-dcommit-merge.sh\n> index fb41876..cf2e25f 100755\n> --- a/t/t9114-git-svn-dcommit-merge.sh\n> +++ b/t/t9114-git-svn-dcommit-merge.sh\n> @@ -68,8 +68,8 @@ test_expect_success 'setup git mirror and merge' '\n>  test_debug 'gitk --all & sleep 1'\n>\n>  test_expect_success 'verify pre-merge ancestry' \"\n> -       test x\\`git rev-parse --verify refs/heads/svn^2\\` = \\\n> -            x\\`git rev-parse --verify refs/heads/merge\\` &&\n> +       test x\\$(git rev-parse --verify refs/heads/svn^2\\) = \\\n> +            x\\$(git rev-parse --verify refs/heads/merge\\) &&\n>         git cat-file commit refs/heads/svn^ | grep '^friend$'\n>         \"\n>\n> I'm not sure what's the intent of the \\ in front of ` in the original\n> code, but changing it to $(...) changes the meaning:\n>\n> $ sh -c \"echo \\`date\\`\"\n> Wed May 14 18:58:19 MEST 2014\n> $ sh -c \"echo \\$(date\\)\"\n> sh: 1: Syntax error: end of file unexpected (expecting \")\")\n>\n> I didn't investigate closely, but I'm getting test failures without your\n> patch, and the script stops in the middle with it so it does break\n> something.\n>\n> @@ -80,10 +80,10 @@ test_expect_success 'git svn dcommit merges' \"\n>  test_debug 'gitk --all & sleep 1'\n>\n>  test_expect_success 'verify post-merge ancestry' \"\n> -       test x\\`git rev-parse --verify refs/heads/svn\\` = \\\n> -            x\\`git rev-parse --verify refs/remotes/origin/trunk \\` &&\n> -       test x\\`git rev-parse --verify refs/heads/svn^2\\` = \\\n> -            x\\`git rev-parse --verify refs/heads/merge\\` &&\n> +       test x\\$(git rev-parse --verify refs/heads/svn\\) = \\\n> +            x\\$(git rev-parse --verify refs/remotes/origin/trunk \\) &&\n> +       test x\\$(git rev-parse --verify refs/heads/svn^2\\) = \\\n> +            x\\$(git rev-parse --verify refs/heads/merge\\) &&\n>\n> Likewise.\n>\n>\n> commit 7e29ac501ce24aa5af3a50f839cd3ad176481a96\n> Author: Elia Pinto <gitter.spiros@gmail.com>\n> Date:   Wed Mar 26 04:48:40 2014 -0700\n>\n>     t9100-git-svn-basic.sh: use the $( ... ) construct for command substitution\n>\n> -test_expect_success 'able to dcommit to a subdirectory' \"\n> +test_expect_success 'able to dcommit to a subdirectory' '\n>\n> There is an actual change other than sed + review and trivial fix here.\n> That makes the review harder. Such change should IMHO not be part of the\n> same series.\n>\n> -       git commit -m '/bar/d should be in the log' &&\n> +       git commit -m \"bar/d should be in the log\" &&\n>\n> You lost a / here.\n>\n>         git svn dcommit -i bar &&\n> -       test -z \\\"\\`git diff refs/heads/my-bar refs/remotes/bar\\`\\\" &&\n> +       test -z $(git diff refs/heads/my-bar refs/remotes/bar) &&\n>\n> Did you not loos the \\\"...\\\" whitespace protection here?\n>\n> -       test -z \\\"\\`git diff refs/heads/my-bar refs/remotes/bar\\`\\\" &&\n> +       test -z \"$(git diff refs/heads/my-bar refs/remotes/bar)\" &&\n>\n> That seems to be the correct way of doing what you tried right above.\n>\n> commit b438455b7b97d90a1b8da4ec4e9de0063c20f63c\n> Author: Elia Pinto <gitter.spiros@gmail.com>\n> Date:   Wed Mar 26 04:48:40 2014 -0700\n>\n>     t9107-git-svn-migrate.sh: use the $( ... ) construct for command substitution\n>\n> [...]\n>\n> @@ -75,7 +75,7 @@ test_expect_success 'multi-fetch works on partial urls + paths' \"\n>         for i in trunk a b tags/0.1 tags/0.2 tags/0.3; do\n>                 git rev-parse --verify refs/remotes/origin/\\$i^0 >> refs.out || exit 1;\n>             done &&\n> -       test -z \\\"\\`sort < refs.out | uniq -d\\`\\\" &&\n> +       test -z \\\"\\$(sort < refs.out | uniq -d\\)\\\" &&\n>\n> Same problem as above, this \\$( is broken.\n>\n> My advice: apply my small fix for multiple `...` `...`, and eject the\n> other patches from the series for now, they are distracting reviewers.\n>\n> That should lead to a trivially-correct series with ~80 patches. Once\n> this one is accepted, the 4 remaining patches can be fixed and reviewed\n> more carefully (Cc Eric Wong <normalperson@yhbt.net> since the patches\n> are about git-svn).\n>\n> --\n> Matthieu Moy\n> http://www-verimag.imag.fr/~moy/\n"},{"id":"241649","messageId":"vpq38gbfpt9.fsf@anie.imag.fr","threadId":"36654","inReplyTo":"CAPig+cT-Nn_hnqE0CK=c0-9+_QG9hpsp9j4cERsvvjwkrLmSMw@mail.gmail.com","subject":"Re: Please pull the patch series \"use the $( ... ) construct for command substitution\"","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-05-15T07:45:22Z","receivedAt":"2014-05-15T07:45:22Z","isPatch":false,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> On Wed, May 14, 2014 at 1:14 PM, Matthieu Moy\n>\n>> I do not understand the use of the \\ in front of the ` in the original\n>> code.\n>\n> The second argument of test_expect_success is double-quoted, so a bare\n> `...` would be evaluated before test_expect_success is even invoked.\n> Quoting it as \\'...\\' correctly suppresses the automatic evaluation,\n> giving test_expect_success the opportunity to evaluate it on-demand.\n\nAh, of course, you're right.\n\n>> The correct code should be\n>>\n>>         test x\\\"$(sed -n -e 4p < file)\\\" = x4 &&\n>>\n>> I guess.\n>\n> Same issue. The $(...) is being evaluated even before\n> test_expect_success is invoked. Better would be to suspend evaluation\n> via \\$(...) to allow test_expect_success to evaluate it on-demand.\n\nOK, then the correct code should be\n\n        test x\\\"\\$(sed -n -e 4p < file)\\\" = x4 &&\n\nAnd the other attempt was close:\n\n> >  test_expect_success 'verify pre-merge ancestry' \"\n> > -       test x\\`git rev-parse --verify refs/heads/svn^2\\` = \\\n> > -            x\\`git rev-parse --verify refs/heads/merge\\` &&\n> > +       test x\\$(git rev-parse --verify refs/heads/svn^2\\) = \\\n> > +            x\\$(git rev-parse --verify refs/heads/merge\\) &&\n\n\\-escaping the $ was right, but the \\) should be a single ).\n\nIn any case, the fact that we need to discuss this supports my claim\n\"this should be reviewed as a separate series\".\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"}]}