* 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; +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
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
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
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.