threads / discuss / 42306

t3404 static check of bad SHA-1 failure

Subject: t3404 static check of bad SHA-1 failure

## tl;dr

19 messages between May 13, 2016 and May 14, 2016.

replies: 18people: 5as markdown or json

Armin Kunaschik· May 13, 2016, 16:09 UTC · lore
Hello,

in t3404 test 91 - static check of bad SHA-1 fails (with ksh) with a syntax error in git-rebase. git-rebase[6]: test: argument expected

Reason is, again, a test -z without quotes in git-rebase--interactive:
*** ../git-rebase--interactive.orig     Tue May 10 17:36:59 2016
--- ../git-rebase--interactive  Fri May 13 17:57:27 2016
***************
*** 870,876 ****
                badsha=1
        else
                sha1_verif="$(git rev-parse --verify --quiet $1^{commit})"
!               if test -z $sha1_verif
                then
                        badsha=1
                fi
--- 870,876 ----
                badsha=1
        else
                sha1_verif="$(git rev-parse --verify --quiet $1^{commit})"
!               if test -z "$sha1_verif"
                then
                        badsha=1
                fi

Maybe it would be a good idea, to quote the test -z $1 in the same function check_commit_sha too. The test completes successfully without it, but since it's an user option, I think it should be quoted. Then there would be no more unquoted test -z anymore :-)

Armin
Jeff King· May 13, 2016, 18:23 UTC · re: Armin Kunaschik · lore

Re: t3404 static check of bad SHA-1 failure

On Fri, May 13, 2016 at 06:09:35PM +0200, Armin Kunaschik wrote:
> in t3404 test 91 - static check of bad SHA-1 fails (with ksh) with a
> syntax error in git-rebase.
> git-rebase[6]: test: argument expected
Here's a fix that covers these and another I found:
-- >8 --
Subject: [PATCH] always quote arguments to "test -z" in shell
Modern shells are pretty forgiving about us doing:
  test -z $foo
If $foo is indeed empty, the test command will see only:
  test -z

and treat the missing argument as "yes, this is empty". But some older shells, reportedly ksh88, complain about the missing argument. We can be more portable by spelling this as:

  test -z "$foo"
so that "test" sees an empty argument, not a missing one.
This covers all cases detected by:
  git grep 'test -z [^"]'

(though note that has a few false positives for tests which need an extra layer of quoting to do '\"').

Reported-by: Armin Kunaschik <megabreit@googlemail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
Actually, this misses the case in t4151 which already has a fix queued
on pu. Arguably these should all just be squashed together (and I am
happy, Junio, if you want to do so and leave Armin as the author of the
new commit).
 git-rebase--interactive.sh | 4 ++--
 git-stash.sh               | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 9ea3075..470413b 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -866,12 +866,12 @@ add_exec_commands () {
 # $3: the input filename
 check_commit_sha () {
 	badsha=0
-	if test -z $1
+	if test -z "$1"
 	then
 		badsha=1
 	else
 		sha1_verif="$(git rev-parse --verify --quiet $1^{commit})"
-		if test -z $sha1_verif
+		if test -z "$sha1_verif"
 		then
 			badsha=1
 		fi
diff --git a/git-stash.sh b/git-stash.sh
index c7c65e2..57f9dc1 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -185,7 +185,7 @@ store_stash () {
 
 	git update-ref --create-reflog -m "$stash_msg" $ref_stash $w_commit
 	ret=$?
-	test $ret != 0 && test -z $quiet &&
+	test $ret != 0 && test -z "$quiet" &&
 	die "$(eval_gettext "Cannot update \$ref_stash with \$w_commit")"
 	return $ret
 }

-- 
2.8.2.825.gea31738
Junio C Hamano· May 13, 2016, 19:52 UTC · re: Jeff King · lore

Re: t3404 static check of bad SHA-1 failure

Jeff King <peff@peff.net> writes:
Show 42 quoted lines
> On Fri, May 13, 2016 at 06:09:35PM +0200, Armin Kunaschik wrote:
>
>> in t3404 test 91 - static check of bad SHA-1 fails (with ksh) with a
>> syntax error in git-rebase.
>> git-rebase[6]: test: argument expected
>
> Here's a fix that covers these and another I found:
>
> -- >8 --
> Subject: [PATCH] always quote arguments to "test -z" in shell
>
> Modern shells are pretty forgiving about us doing:
>
>   test -z $foo
>
> If $foo is indeed empty, the test command will see only:
>
>   test -z
>
> and treat the missing argument as "yes, this is empty". But
> some older shells, reportedly ksh88, complain about the
> missing argument. We can be more portable by spelling this
> as:
>
>   test -z "$foo"
>
> so that "test" sees an empty argument, not a missing one.
>
> This covers all cases detected by:
>
>   git grep 'test -z [^"]'
>
> (though note that has a few false positives for tests which
> need an extra layer of quoting to do '\"').
>
> Reported-by: Armin Kunaschik <megabreit@googlemail.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Actually, this misses the case in t4151 which already has a fix queued
> on pu. Arguably these should all just be squashed together (and I am
> happy, Junio, if you want to do so and leave Armin as the author of the
> new commit).
I _think_ "test -z" should succeed according to POSIX, because
 (1) it is not "test -z string" because it lacks string,
 (2) it is not any of the other "test -<option> thing" because -z,
    and
 (3) the only thing it matches in the supported form of "test" is
     "test <string>" that tests if the <string> is not the null
     string, and "-z" indeed is not the null string.
For the same reason, "test -n" succeeds.

But working around older/broken shells is easy and the resulting script it more readable, so let's take this. It makes the resulting code easier to understand even when we know we run it under POSIX shell.

Thanks.
Show 36 quoted lines
>  git-rebase--interactive.sh | 4 ++--
>  git-stash.sh               | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 9ea3075..470413b 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -866,12 +866,12 @@ add_exec_commands () {
>  # $3: the input filename
>  check_commit_sha () {
>  	badsha=0
> -	if test -z $1
> +	if test -z "$1"
>  	then
>  		badsha=1
>  	else
>  		sha1_verif="$(git rev-parse --verify --quiet $1^{commit})"
> -		if test -z $sha1_verif
> +		if test -z "$sha1_verif"
>  		then
>  			badsha=1
>  		fi
> diff --git a/git-stash.sh b/git-stash.sh
> index c7c65e2..57f9dc1 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -185,7 +185,7 @@ store_stash () {
>  
>  	git update-ref --create-reflog -m "$stash_msg" $ref_stash $w_commit
>  	ret=$?
> -	test $ret != 0 && test -z $quiet &&
> +	test $ret != 0 && test -z "$quiet" &&
>  	die "$(eval_gettext "Cannot update \$ref_stash with \$w_commit")"
>  	return $ret
>  }
Jeff King· May 13, 2016, 19:59 UTC · re: Junio C Hamano · lore

Re: t3404 static check of bad SHA-1 failure

On Fri, May 13, 2016 at 12:52:44PM -0700, Junio C Hamano wrote:
Show 12 quoted lines
> I _think_ "test -z" should succeed according to POSIX, because
> 
>  (1) it is not "test -z string" because it lacks string,
> 
>  (2) it is not any of the other "test -<option> thing" because -z,
>     and
> 
>  (3) the only thing it matches in the supported form of "test" is
>      "test <string>" that tests if the <string> is not the null
>      string, and "-z" indeed is not the null string.
> 
> For the same reason, "test -n" succeeds.

Yeah, I think you're right; POSIX is pretty clear that this falls under case 3. So that means "test -z" quietly does what we want. But it means that "test -n" does the _opposite_ of what we want.

And sadly,
  git grep 'test -n [^"]'
is not empty.
> But working around older/broken shells is easy and the resulting
> script it more readable, so let's take this.  It makes the resulting
> code easier to understand even when we know we run it under POSIX
> shell.

Yep. The POSIX-explanation of what is going on might be worth putting in the commit message for the "-z" case (i.e., it should work, but the "why" is subtle).

-Peff
Jeff King· May 13, 2016, 20:02 UTC · re: Jeff King · lore

Re: t3404 static check of bad SHA-1 failure

On Fri, May 13, 2016 at 03:59:11PM -0400, Jeff King wrote:
Show 33 quoted lines
> On Fri, May 13, 2016 at 12:52:44PM -0700, Junio C Hamano wrote:
> 
> > I _think_ "test -z" should succeed according to POSIX, because
> > 
> >  (1) it is not "test -z string" because it lacks string,
> > 
> >  (2) it is not any of the other "test -<option> thing" because -z,
> >     and
> > 
> >  (3) the only thing it matches in the supported form of "test" is
> >      "test <string>" that tests if the <string> is not the null
> >      string, and "-z" indeed is not the null string.
> > 
> > For the same reason, "test -n" succeeds.
> 
> Yeah, I think you're right; POSIX is pretty clear that this falls under
> case 3. So that means "test -z" quietly does what we want. But it means
> that "test -n" does the _opposite_ of what we want.
> 
> And sadly,
> 
>   git grep 'test -n [^"]'
> 
> is not empty.
> 
> > But working around older/broken shells is easy and the resulting
> > script it more readable, so let's take this.  It makes the resulting
> > code easier to understand even when we know we run it under POSIX
> > shell.
> 
> Yep. The POSIX-explanation of what is going on might be worth putting in
> the commit message for the "-z" case (i.e., it should work, but the
> "why" is subtle).

I think we can just lump all of these into a single patch, but there are a few related cleanups, including the SVN stuff I just sent. So let me send out a unified patch series in a moment.

-Peff
Junio C Hamano· May 13, 2016, 20:03 UTC · re: Jeff King · lore

Re: t3404 static check of bad SHA-1 failure

Jeff King <peff@peff.net> writes:
Show 18 quoted lines
> On Fri, May 13, 2016 at 12:52:44PM -0700, Junio C Hamano wrote:
>
>> I _think_ "test -z" should succeed according to POSIX, because
>> 
>>  (1) it is not "test -z string" because it lacks string,
>> 
>>  (2) it is not any of the other "test -<option> thing" because -z,
>>     and
>> 
>>  (3) the only thing it matches in the supported form of "test" is
>>      "test <string>" that tests if the <string> is not the null
>>      string, and "-z" indeed is not the null string.
>> 
>> For the same reason, "test -n" succeeds.
>
> Yeah, I think you're right; POSIX is pretty clear that this falls under
> case 3. So that means "test -z" quietly does what we want. But it means
> that "test -n" does the _opposite_ of what we want.
;-)
Show 5 quoted lines
> And sadly,
>
>   git grep 'test -n [^"]'
>
> is not empty.

Are you doing an audit? Otherwise I'm interested in taking a brief look.

Show 8 quoted lines
>> But working around older/broken shells is easy and the resulting
>> script it more readable, so let's take this.  It makes the resulting
>> code easier to understand even when we know we run it under POSIX
>> shell.
>
> Yep. The POSIX-explanation of what is going on might be worth putting in
> the commit message for the "-z" case (i.e., it should work, but the
> "why" is subtle).
Perhaps.

A small consolation for ksh fans around is that ksh93 seems to get this case right ;-)

Jeff King· May 13, 2016, 20:46 UTC · re: Junio C Hamano · lore

[PATCH 0/6] test -z/-n quoting fix + misc cleanups

On Fri, May 13, 2016 at 01:03:41PM -0700, Junio C Hamano wrote:
Show 8 quoted lines
> > And sadly,
> >
> >   git grep 'test -n [^"]'
> >
> > is not empty.
> 
> Are you doing an audit?  Otherwise I'm interested in taking a brief
> look.

There was only one buggy case there (in git-stash). The rest were false positives.

I didn't audit for:
  test $foo = bar
which also has problems. You can grep for:
  git grep 'test \$'

and there are a lot of hits. Many of them are probably fine, if they are variables that are known to be non-empty and not contain whitespace (e.g., $#). But some of them are questionable, like:

  git-request-pull.sh:if test $(git cat-file -t "$head") = tag

I suspect in practice that's fine just because we're likely to see either the empty string (in which case test will barf with "unary operator expected", which matches what we want), or a single-word response (which doesn't need further quoting).

> >> But working around older/broken shells is easy and the resulting
> >> script it more readable, so let's take this.  It makes the resulting
> >> code easier to understand even when we know we run it under POSIX
> >> shell.
Actually, it's not just older shells:
  foo='bar baz'
  test -z $foo

is "unspecified" according to POSIX, though in practice it will complain about "binary operator expected". You can get some weirdness, though, like:

  foo='!= bar'
  test -z $foo

which returns 0. Unlikely, but still clearly wrong for us not to be quoting.

Anyway. Here's a series that fixes the -n/-z cases, along with a bunch of cleanups that remove the false positives (most of which I sent out just a few minutes ago as "minor fixes to some svn tests").

  [1/6]: t/lib-git-svn: drop $remote_git_svn and $git_svn_id
  [2/6]: t9100,t3419: enclose all test code in single-quotes
  [3/6]: t9107: use "return 1" instead of "exit 1"
  [4/6]: t9107: switch inverted single/double quotes in test
  [5/6]: t9103: modernize test style
  [6/6]: always quote shell arguments to test -z/-n

You could take just 6/6 as its own series; the rest are just about removing the false positives, and fixing other issues. I put it last, though, because otherwise the "this grep is now empty" claim in it is not true. :)

-Peff
Jeff King· May 13, 2016, 20:47 UTC · re: Jeff King · lore

[PATCH 1/6] t/lib-git-svn: drop $remote_git_svn and $git_svn_id

These variables were added in 16805d3 (t/t91XX-svn: start removing use of "git-" from these tests, 2008-09-08) so that running:

  git grep git-

would return fewer hits. At the time, we were transitioning away from the use of the "dashed" git-foo form.

That transition has been over for years, and grepping for "git-" in the test suite yields thousands of hits anyway (all presumably false positives).

With their original purpose gone, these variables serve only to obfuscate the tests. Let's get rid of them.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/lib-git-svn.sh                              |  3 ---
 t/t9100-git-svn-basic.sh                      | 38 +++++++++++++--------------
 t/t9101-git-svn-props.sh                      | 12 ++++-----
 t/t9102-git-svn-deep-rmdir.sh                 |  2 +-
 t/t9106-git-svn-commit-diff-clobber.sh        |  6 ++---
 t/t9107-git-svn-migrate.sh                    | 14 +++++-----
 t/t9110-git-svn-use-svm-props.sh              | 18 ++++++-------
 t/t9111-git-svn-use-svnsync-props.sh          | 18 ++++++-------
 t/t9120-git-svn-clone-with-percent-escapes.sh |  6 ++---
 t/t9123-git-svn-rebuild-with-rewriteroot.sh   |  2 +-
 t/t9153-git-svn-rewrite-uuid.sh               |  4 +--
 11 files changed, 60 insertions(+), 63 deletions(-)
diff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh
index 6a50b87..fb88232 100644
--- a/t/lib-git-svn.sh
+++ b/t/lib-git-svn.sh
@@ -1,8 +1,5 @@
 . ./test-lib.sh
 
-remotes_git_svn=remotes/git""-svn
-git_svn_id=git""-svn-id
-
 if test -n "$NO_SVN_TESTS"
 then
 	skip_all='skipping git svn tests, NO_SVN_TESTS defined'
diff --git a/t/t9100-git-svn-basic.sh b/t/t9100-git-svn-basic.sh
index 22d8367..6ec73ee 100755
--- a/t/t9100-git-svn-basic.sh
+++ b/t/t9100-git-svn-basic.sh
@@ -45,13 +45,13 @@ test_expect_success "checkout from svn" 'svn co "$svnrepo" "$SVN_TREE"'
 
 name='try a deep --rmdir with a commit'
 test_expect_success "$name" '
-	git checkout -f -b mybranch ${remotes_git_svn} &&
+	git checkout -f -b mybranch remotes/git-svn &&
 	mv dir/a/b/c/d/e/file dir/file &&
 	cp dir/file file &&
 	git update-index --add --remove dir/a/b/c/d/e/file dir/file file &&
 	git commit -m "$name" &&
 	git svn set-tree --find-copies-harder --rmdir \
-		${remotes_git_svn}..mybranch &&
+		remotes/git-svn..mybranch &&
 	svn_cmd up "$SVN_TREE" &&
 	test -d "$SVN_TREE"/dir && test ! -d "$SVN_TREE"/dir/a'
 
@@ -65,14 +65,14 @@ test_expect_success "$name" "
 	git update-index --add dir/file/file &&
 	git commit -m '$name' &&
 	test_must_fail git svn set-tree --find-copies-harder --rmdir \
-		${remotes_git_svn}..mybranch
+		remotes/git-svn..mybranch
 "
 
 
 name='detect node change from directory to file #1'
 test_expect_success "$name" '
 	rm -rf dir "$GIT_DIR"/index &&
-	git checkout -f -b mybranch2 ${remotes_git_svn} &&
+	git checkout -f -b mybranch2 remotes/git-svn &&
 	mv bar/zzz zzz &&
 	rm -rf bar &&
 	mv zzz bar &&
@@ -80,14 +80,14 @@ test_expect_success "$name" '
 	git update-index --add -- bar &&
 	git commit -m "$name" &&
 	test_must_fail git svn set-tree --find-copies-harder --rmdir \
-		${remotes_git_svn}..mybranch2
+		remotes/git-svn..mybranch2
 '
 
 
 name='detect node change from file to directory #2'
 test_expect_success "$name" '
 	rm -f "$GIT_DIR"/index &&
-	git checkout -f -b mybranch3 ${remotes_git_svn} &&
+	git checkout -f -b mybranch3 remotes/git-svn &&
 	rm bar/zzz &&
 	git update-index --remove bar/zzz &&
 	mkdir bar/zzz &&
@@ -95,7 +95,7 @@ test_expect_success "$name" '
 	git update-index --add bar/zzz/yyy &&
 	git commit -m "$name" &&
 	git svn set-tree --find-copies-harder --rmdir \
-		${remotes_git_svn}..mybranch3 &&
+		remotes/git-svn..mybranch3 &&
 	svn_cmd up "$SVN_TREE" &&
 	test -d "$SVN_TREE"/bar/zzz &&
 	test -e "$SVN_TREE"/bar/zzz/yyy
@@ -104,7 +104,7 @@ test_expect_success "$name" '
 name='detect node change from directory to file #2'
 test_expect_success "$name" '
 	rm -f "$GIT_DIR"/index &&
-	git checkout -f -b mybranch4 ${remotes_git_svn} &&
+	git checkout -f -b mybranch4 remotes/git-svn &&
 	rm -rf dir &&
 	git update-index --remove -- dir/file &&
 	touch dir &&
@@ -112,19 +112,19 @@ test_expect_success "$name" '
 	git update-index --add -- dir &&
 	git commit -m "$name" &&
 	test_must_fail git svn set-tree --find-copies-harder --rmdir \
-		${remotes_git_svn}..mybranch4
+		remotes/git-svn..mybranch4
 '
 
 
 name='remove executable bit from a file'
 test_expect_success POSIXPERM "$name" '
 	rm -f "$GIT_DIR"/index &&
-	git checkout -f -b mybranch5 ${remotes_git_svn} &&
+	git checkout -f -b mybranch5 remotes/git-svn &&
 	chmod -x exec.sh &&
 	git update-index exec.sh &&
 	git commit -m "$name" &&
 	git svn set-tree --find-copies-harder --rmdir \
-		${remotes_git_svn}..mybranch5 &&
+		remotes/git-svn..mybranch5 &&
 	svn_cmd up "$SVN_TREE" &&
 	test ! -x "$SVN_TREE"/exec.sh'
 
@@ -135,7 +135,7 @@ test_expect_success POSIXPERM "$name" '
 	git update-index exec.sh &&
 	git commit -m "$name" &&
 	git svn set-tree --find-copies-harder --rmdir \
-		${remotes_git_svn}..mybranch5 &&
+		remotes/git-svn..mybranch5 &&
 	svn_cmd up "$SVN_TREE" &&
 	test -x "$SVN_TREE"/exec.sh'
 
@@ -147,7 +147,7 @@ test_expect_success SYMLINKS "$name" '
 	git update-index exec.sh &&
 	git commit -m "$name" &&
 	git svn set-tree --find-copies-harder --rmdir \
-		${remotes_git_svn}..mybranch5 &&
+		remotes/git-svn..mybranch5 &&
 	svn_cmd up "$SVN_TREE" &&
 	test -h "$SVN_TREE"/exec.sh'
 
@@ -159,7 +159,7 @@ test_expect_success POSIXPERM,SYMLINKS "$name" '
 	git update-index --add file exec-2.sh &&
 	git commit -m "$name" &&
 	git svn set-tree --find-copies-harder --rmdir \
-		${remotes_git_svn}..mybranch5 &&
+		remotes/git-svn..mybranch5 &&
 	svn_cmd up "$SVN_TREE" &&
 	test -x "$SVN_TREE"/file &&
 	test -h "$SVN_TREE"/exec-2.sh'
@@ -172,7 +172,7 @@ test_expect_success POSIXPERM,SYMLINKS "$name" '
 	git update-index exec-2.sh &&
 	git commit -m "$name" &&
 	git svn set-tree --find-copies-harder --rmdir \
-		${remotes_git_svn}..mybranch5 &&
+		remotes/git-svn..mybranch5 &&
 	svn_cmd up "$SVN_TREE" &&
 	test -f "$SVN_TREE"/exec-2.sh &&
 	test ! -h "$SVN_TREE"/exec-2.sh &&
@@ -194,7 +194,7 @@ GIT_SVN_ID=alt
 export GIT_SVN_ID
 test_expect_success "$name" \
     'git svn init "$svnrepo" && git svn fetch &&
-     git rev-list --pretty=raw ${remotes_git_svn} | grep ^tree | uniq > a &&
+     git rev-list --pretty=raw remotes/git-svn | grep ^tree | uniq > a &&
      git rev-list --pretty=raw remotes/alt | grep ^tree | uniq > b &&
      test_cmp a b'
 
@@ -219,7 +219,7 @@ test_expect_success POSIXPERM,SYMLINKS "$name" "test_cmp a expected"
 
 test_expect_success 'exit if remote refs are ambigious' "
         git config --add svn-remote.svn.fetch \
-                              bar:refs/${remotes_git_svn} &&
+		bar:refs/remotes/git-svn &&
 	test_must_fail git svn migrate
 "
 
@@ -227,7 +227,7 @@ test_expect_success 'exit if init-ing a would clobber a URL' '
         svnadmin create "${PWD}/svnrepo2" &&
         svn mkdir -m "mkdir bar" "${svnrepo}2/bar" &&
         git config --unset svn-remote.svn.fetch \
-                                "^bar:refs/${remotes_git_svn}$" &&
+		"^bar:refs/remotes/git-svn$" &&
 	test_must_fail git svn init "${svnrepo}2/bar"
         '
 
@@ -237,7 +237,7 @@ test_expect_success \
         git config --get svn-remote.svn.fetch \
                               "^bar:refs/remotes/bar$" &&
         git config --get svn-remote.svn.fetch \
-                              "^:refs/${remotes_git_svn}$"
+			      "^:refs/remotes/git-svn$"
         '
 
 test_expect_success 'dcommit $rev does not clobber current branch' '
diff --git a/t/t9101-git-svn-props.sh b/t/t9101-git-svn-props.sh
index e8173d5..07bfb63 100755
--- a/t/t9101-git-svn-props.sh
+++ b/t/t9101-git-svn-props.sh
@@ -73,11 +73,11 @@ test_expect_success 'fetch revisions from svn' 'git svn fetch'
 
 name='test svn:keywords ignoring'
 test_expect_success "$name" \
-	'git checkout -b mybranch ${remotes_git_svn} &&
+	'git checkout -b mybranch remotes/git-svn &&
 	echo Hi again >> kw.c &&
 	git commit -a -m "test keywords ignoring" &&
-	git svn set-tree ${remotes_git_svn}..mybranch &&
-	git pull . ${remotes_git_svn}'
+	git svn set-tree remotes/git-svn..mybranch &&
+	git pull . remotes/git-svn'
 
 expect='/* $Id$ */'
 got="$(sed -ne 2p kw.c)"
@@ -95,7 +95,7 @@ test_expect_success "propset CR on crlf files" '
 
 test_expect_success 'fetch and pull latest from svn and checkout a new wc' \
 	'git svn fetch &&
-	 git pull . ${remotes_git_svn} &&
+	 git pull . remotes/git-svn &&
 	 svn_cmd co "$svnrepo" new_wc'
 
 for i in crlf ne_crlf lf ne_lf cr ne_cr empty_cr empty_lf empty empty_crlf
@@ -117,7 +117,7 @@ cd test_wc
 	 svn_cmd commit -m "propset CRLF on cr files"'
 cd ..
 test_expect_success 'fetch and pull latest from svn' \
-	'git svn fetch && git pull . ${remotes_git_svn}'
+	'git svn fetch && git pull . remotes/git-svn'
 
 b_cr="$(git hash-object cr)"
 b_ne_cr="$(git hash-object ne_cr)"
@@ -168,7 +168,7 @@ cat >create-ignore-index.expect <<\EOF
 EOF
 
 test_expect_success 'test create-ignore' "
-	git svn fetch && git pull . ${remotes_git_svn} &&
+	git svn fetch && git pull . remotes/git-svn &&
 	git svn create-ignore &&
 	cmp ./.gitignore create-ignore.expect &&
 	cmp ./deeply/.gitignore create-ignore.expect &&
diff --git a/t/t9102-git-svn-deep-rmdir.sh b/t/t9102-git-svn-deep-rmdir.sh
index eb70f48..66cd511 100755
--- a/t/t9102-git-svn-deep-rmdir.sh
+++ b/t/t9102-git-svn-deep-rmdir.sh
@@ -17,7 +17,7 @@ test_expect_success 'initialize repo' '
 test_expect_success 'mirror via git svn' '
 	git svn init "$svnrepo" &&
 	git svn fetch &&
-	git checkout -f -b test-rmdir ${remotes_git_svn}
+	git checkout -f -b test-rmdir remotes/git-svn
 	'
 
 test_expect_success 'Try a commit on rmdir' '
diff --git a/t/t9106-git-svn-commit-diff-clobber.sh b/t/t9106-git-svn-commit-diff-clobber.sh
index f6d7ac7..dbe8dea 100755
--- a/t/t9106-git-svn-commit-diff-clobber.sh
+++ b/t/t9106-git-svn-commit-diff-clobber.sh
@@ -44,7 +44,7 @@ test_expect_success 'commit complementing change from git' '
 test_expect_success 'dcommit fails to commit because of conflict' '
 	git svn init "$svnrepo" &&
 	git svn fetch &&
-	git reset --hard refs/${remotes_git_svn} &&
+	git reset --hard refs/remotes/git-svn &&
 	svn_cmd co "$svnrepo" t.svn &&
 	(
 		cd t.svn &&
@@ -59,7 +59,7 @@ test_expect_success 'dcommit fails to commit because of conflict' '
 	'
 
 test_expect_success 'dcommit does the svn equivalent of an index merge' "
-	git reset --hard refs/${remotes_git_svn} &&
+	git reset --hard refs/remotes/git-svn &&
 	echo 'index merge' > file2 &&
 	git update-index --add file2 &&
 	git commit -a -m 'index merge' &&
@@ -81,7 +81,7 @@ test_expect_success 'commit another change from svn side' '
 	'
 
 test_expect_success 'multiple dcommit from git svn will not clobber svn' "
-	git reset --hard refs/${remotes_git_svn} &&
+	git reset --hard refs/remotes/git-svn &&
 	echo new file >> new-file &&
 	git update-index --add new-file &&
 	git commit -a -m 'new file' &&
diff --git a/t/t9107-git-svn-migrate.sh b/t/t9107-git-svn-migrate.sh
index 9060198..6efc2ab 100755
--- a/t/t9107-git-svn-migrate.sh
+++ b/t/t9107-git-svn-migrate.sh
@@ -19,9 +19,9 @@ test_expect_success 'setup old-looking metadata' '
 	git svn init "$svnrepo" &&
 	git svn fetch &&
 	rm -rf "$GIT_DIR"/svn &&
-	git update-ref refs/heads/git-svn-HEAD refs/${remotes_git_svn} &&
-	git update-ref refs/heads/svn-HEAD refs/${remotes_git_svn} &&
-	git update-ref -d refs/${remotes_git_svn} refs/${remotes_git_svn}
+	git update-ref refs/heads/git-svn-HEAD refs/remotes/git-svn &&
+	git update-ref refs/heads/svn-HEAD refs/remotes/git-svn &&
+	git update-ref -d refs/remotes/git-svn refs/remotes/git-svn
 	'
 
 head=$(git rev-parse --verify refs/heads/git-svn-HEAD^0)
@@ -35,11 +35,11 @@ test_expect_success 'initialize old-style (v0) git svn layout' '
 	echo "$svnrepo" > "$GIT_DIR"/svn/info/url &&
 	git svn migrate &&
 	! test -d "$GIT_DIR"/git-svn &&
-	git rev-parse --verify refs/${remotes_git_svn}^0 &&
+	git rev-parse --verify refs/remotes/git-svn^0 &&
 	git rev-parse --verify refs/remotes/svn^0 &&
 	test "$(git config --get svn-remote.svn.url)" = "$svnrepo_escaped" &&
 	test $(git config --get svn-remote.svn.fetch) = \
-             ":refs/${remotes_git_svn}"
+		":refs/remotes/git-svn"
 	'
 
 test_expect_success 'initialize a multi-repository repo' '
@@ -66,7 +66,7 @@ test_expect_success 'initialize a multi-repository repo' '
 	grep "^tags/0\.1:refs/remotes/origin/tags/0\.1$" fetch.out &&
 	grep "^tags/0\.2:refs/remotes/origin/tags/0\.2$" fetch.out &&
 	grep "^tags/0\.3:refs/remotes/origin/tags/0\.3$" fetch.out &&
-	grep "^:refs/${remotes_git_svn}" fetch.out
+	grep "^:refs/remotes/git-svn" fetch.out
 	'
 
 # refs should all be different, but the trees should all be the same:
@@ -104,7 +104,7 @@ test_expect_success 'migrate --minimize on old inited layout' '
 	grep "^tags/0\.1:refs/remotes/origin/tags/0\.1$" fetch.out &&
 	grep "^tags/0\.2:refs/remotes/origin/tags/0\.2$" fetch.out &&
 	grep "^tags/0\.3:refs/remotes/origin/tags/0\.3$" fetch.out &&
-	grep "^:refs/${remotes_git_svn}" fetch.out
+	grep "^:refs/remotes/git-svn" fetch.out
 	'
 
 test_expect_success  ".rev_db auto-converted to .rev_map.UUID" '
diff --git a/t/t9110-git-svn-use-svm-props.sh b/t/t9110-git-svn-use-svm-props.sh
index 29fbdfd..dde0a3c 100755
--- a/t/t9110-git-svn-use-svm-props.sh
+++ b/t/t9110-git-svn-use-svm-props.sh
@@ -22,31 +22,31 @@ uuid=161ce429-a9dd-4828-af4a-52023f968c89
 bar_url=http://mayonaise/svnrepo/bar
 test_expect_success 'verify metadata for /bar' "
 	git cat-file commit refs/remotes/bar | \
-	   grep '^${git_svn_id}: $bar_url@12 $uuid$' &&
+	   grep '^git-svn-id: $bar_url@12 $uuid$' &&
 	git cat-file commit refs/remotes/bar~1 | \
-	   grep '^${git_svn_id}: $bar_url@11 $uuid$' &&
+	   grep '^git-svn-id: $bar_url@11 $uuid$' &&
 	git cat-file commit refs/remotes/bar~2 | \
-	   grep '^${git_svn_id}: $bar_url@10 $uuid$' &&
+	   grep '^git-svn-id: $bar_url@10 $uuid$' &&
 	git cat-file commit refs/remotes/bar~3 | \
-	   grep '^${git_svn_id}: $bar_url@9 $uuid$' &&
+	   grep '^git-svn-id: $bar_url@9 $uuid$' &&
 	git cat-file commit refs/remotes/bar~4 | \
-	   grep '^${git_svn_id}: $bar_url@6 $uuid$' &&
+	   grep '^git-svn-id: $bar_url@6 $uuid$' &&
 	git cat-file commit refs/remotes/bar~5 | \
-	   grep '^${git_svn_id}: $bar_url@1 $uuid$'
+	   grep '^git-svn-id: $bar_url@1 $uuid$'
 	"
 
 e_url=http://mayonaise/svnrepo/dir/a/b/c/d/e
 test_expect_success 'verify metadata for /dir/a/b/c/d/e' "
 	git cat-file commit refs/remotes/e | \
-	   grep '^${git_svn_id}: $e_url@1 $uuid$'
+	   grep '^git-svn-id: $e_url@1 $uuid$'
 	"
 
 dir_url=http://mayonaise/svnrepo/dir
 test_expect_success 'verify metadata for /dir' "
 	git cat-file commit refs/remotes/dir | \
-	   grep '^${git_svn_id}: $dir_url@2 $uuid$' &&
+	   grep '^git-svn-id: $dir_url@2 $uuid$' &&
 	git cat-file commit refs/remotes/dir~1 | \
-	   grep '^${git_svn_id}: $dir_url@1 $uuid$'
+	   grep '^git-svn-id: $dir_url@1 $uuid$'
 	"
 
 test_expect_success 'find commit based on SVN revision number' "
diff --git a/t/t9111-git-svn-use-svnsync-props.sh b/t/t9111-git-svn-use-svnsync-props.sh
index bd081c2..22b6e5e 100755
--- a/t/t9111-git-svn-use-svnsync-props.sh
+++ b/t/t9111-git-svn-use-svnsync-props.sh
@@ -21,31 +21,31 @@ uuid=161ce429-a9dd-4828-af4a-52023f968c89
 bar_url=http://mayonaise/svnrepo/bar
 test_expect_success 'verify metadata for /bar' "
 	git cat-file commit refs/remotes/bar | \
-	   grep '^${git_svn_id}: $bar_url@12 $uuid$' &&
+	   grep '^git-svn-id: $bar_url@12 $uuid$' &&
 	git cat-file commit refs/remotes/bar~1 | \
-	   grep '^${git_svn_id}: $bar_url@11 $uuid$' &&
+	   grep '^git-svn-id: $bar_url@11 $uuid$' &&
 	git cat-file commit refs/remotes/bar~2 | \
-	   grep '^${git_svn_id}: $bar_url@10 $uuid$' &&
+	   grep '^git-svn-id: $bar_url@10 $uuid$' &&
 	git cat-file commit refs/remotes/bar~3 | \
-	   grep '^${git_svn_id}: $bar_url@9 $uuid$' &&
+	   grep '^git-svn-id: $bar_url@9 $uuid$' &&
 	git cat-file commit refs/remotes/bar~4 | \
-	   grep '^${git_svn_id}: $bar_url@6 $uuid$' &&
+	   grep '^git-svn-id: $bar_url@6 $uuid$' &&
 	git cat-file commit refs/remotes/bar~5 | \
-	   grep '^${git_svn_id}: $bar_url@1 $uuid$'
+	   grep '^git-svn-id: $bar_url@1 $uuid$'
 	"
 
 e_url=http://mayonaise/svnrepo/dir/a/b/c/d/e
 test_expect_success 'verify metadata for /dir/a/b/c/d/e' "
 	git cat-file commit refs/remotes/e | \
-	   grep '^${git_svn_id}: $e_url@1 $uuid$'
+	   grep '^git-svn-id: $e_url@1 $uuid$'
 	"
 
 dir_url=http://mayonaise/svnrepo/dir
 test_expect_success 'verify metadata for /dir' "
 	git cat-file commit refs/remotes/dir | \
-	   grep '^${git_svn_id}: $dir_url@2 $uuid$' &&
+	   grep '^git-svn-id: $dir_url@2 $uuid$' &&
 	git cat-file commit refs/remotes/dir~1 | \
-	   grep '^${git_svn_id}: $dir_url@1 $uuid$'
+	   grep '^git-svn-id: $dir_url@1 $uuid$'
 	"
 
 test_done
diff --git a/t/t9120-git-svn-clone-with-percent-escapes.sh b/t/t9120-git-svn-clone-with-percent-escapes.sh
index 1c84ce1..59465b1 100755
--- a/t/t9120-git-svn-clone-with-percent-escapes.sh
+++ b/t/t9120-git-svn-clone-with-percent-escapes.sh
@@ -22,7 +22,7 @@ test_expect_success 'test clone with percent escapes' '
 	git svn clone "$svnrepo/pr%20ject" clone &&
 	(
 		cd clone &&
-		git rev-parse refs/${remotes_git_svn}
+		git rev-parse refs/remotes/git-svn
 	)
 '
 
@@ -42,7 +42,7 @@ test_expect_success 'test clone trunk with percent escapes and minimize-url' '
 	git svn clone --minimize-url "$svnrepo/pr%20ject/trunk" minimize &&
 	(
 		cd minimize &&
-		git rev-parse refs/${remotes_git_svn}
+		git rev-parse refs/remotes/git-svn
 	)
 '
 
@@ -50,7 +50,7 @@ test_expect_success 'test clone trunk with percent escapes' '
 	git svn clone "$svnrepo/pr%20ject/trunk" trunk &&
 	(
 		cd trunk &&
-		git rev-parse refs/${remotes_git_svn}
+		git rev-parse refs/remotes/git-svn
 	)
 '
 
diff --git a/t/t9123-git-svn-rebuild-with-rewriteroot.sh b/t/t9123-git-svn-rebuild-with-rewriteroot.sh
index fd81847..ead4045 100755
--- a/t/t9123-git-svn-rebuild-with-rewriteroot.sh
+++ b/t/t9123-git-svn-rebuild-with-rewriteroot.sh
@@ -17,7 +17,7 @@ rm -rf import
 test_expect_success 'init, fetch and checkout repository' '
 	git svn init --rewrite-root=http://invalid.invalid/ "$svnrepo" &&
 	git svn fetch &&
-	git checkout -b mybranch ${remotes_git_svn}
+	git checkout -b mybranch remotes/git-svn
 	'
 
 test_expect_success 'remove rev_map' '
diff --git a/t/t9153-git-svn-rewrite-uuid.sh b/t/t9153-git-svn-rewrite-uuid.sh
index 88a2cfa..372ef15 100755
--- a/t/t9153-git-svn-rewrite-uuid.sh
+++ b/t/t9153-git-svn-rewrite-uuid.sh
@@ -17,9 +17,9 @@ test_expect_success 'load svn repo' "
 
 test_expect_success 'verify uuid' "
 	git cat-file commit refs/remotes/git-svn~0 | \
-	   grep '^${git_svn_id}: .*@2 $uuid$' &&
+	   grep '^git-svn-id: .*@2 $uuid$' &&
 	git cat-file commit refs/remotes/git-svn~1 | \
-	   grep '^${git_svn_id}: .*@1 $uuid$'
+	   grep '^git-svn-id: .*@1 $uuid$'
 	"
 
 test_done

-- 
2.8.2.825.gea31738
Jeff King· May 13, 2016, 20:47 UTC · re: Jeff King · lore

[PATCH 2/6] t9100,t3419: enclose all test code in single-quotes

A few tests here use double-quotes around the snippets of shell code to run the tests. None of these tests wants to do any interpolation at all, and it just leads to an extra layer of quoting around all double-quotes and dollar signs inside the snippet. Let's switch to single quotes, like most other test scripts.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t3419-rebase-patch-id.sh | 12 ++++++------
 t/t9100-git-svn-basic.sh   | 28 ++++++++++++++--------------
 2 files changed, 20 insertions(+), 20 deletions(-)
diff --git a/t/t3419-rebase-patch-id.sh b/t/t3419-rebase-patch-id.sh
index 217dd79..49f548c 100755
--- a/t/t3419-rebase-patch-id.sh
+++ b/t/t3419-rebase-patch-id.sh
@@ -73,17 +73,17 @@ do_tests () {
 		run git format-patch --stdout --ignore-if-in-upstream master
 	"
 
-	test_expect_success $pr 'detect upstream patch' "
+	test_expect_success $pr 'detect upstream patch' '
 		git checkout -q master &&
 		scramble file &&
 		git add file &&
-		git commit -q -m 'change big file again' &&
+		git commit -q -m "change big file again" &&
 		git checkout -q other^{} &&
 		git rebase master &&
-		test_must_fail test -n \"\$(git rev-list master...HEAD~)\"
-	"
+		test_must_fail test -n "$(git rev-list master...HEAD~)"
+	'
 
-	test_expect_success $pr 'do not drop patch' "
+	test_expect_success $pr 'do not drop patch' '
 		git branch -f squashed master &&
 		git checkout -q -f squashed &&
 		git reset -q --soft HEAD~2 &&
@@ -91,7 +91,7 @@ do_tests () {
 		git checkout -q other^{} &&
 		test_must_fail git rebase squashed &&
 		rm -rf .git/rebase-apply
-	"
+	'
 }
 
 do_tests 500
diff --git a/t/t9100-git-svn-basic.sh b/t/t9100-git-svn-basic.sh
index 6ec73ee..28082b1 100755
--- a/t/t9100-git-svn-basic.sh
+++ b/t/t9100-git-svn-basic.sh
@@ -217,11 +217,11 @@ EOF
 
 test_expect_success POSIXPERM,SYMLINKS "$name" "test_cmp a expected"
 
-test_expect_success 'exit if remote refs are ambigious' "
+test_expect_success 'exit if remote refs are ambigious' '
         git config --add svn-remote.svn.fetch \
 		bar:refs/remotes/git-svn &&
 	test_must_fail git svn migrate
-"
+'
 
 test_expect_success 'exit if init-ing a would clobber a URL' '
         svnadmin create "${PWD}/svnrepo2" &&
@@ -259,26 +259,26 @@ test_expect_success 'dcommit $rev does not clobber current branch' '
 	git branch -D my-bar
 	'
 
-test_expect_success 'able to dcommit to a subdirectory' "
+test_expect_success 'able to dcommit to a subdirectory' '
 	git svn fetch -i bar &&
 	git checkout -b my-bar refs/remotes/bar &&
 	echo abc > d &&
 	git update-index --add d &&
-	git commit -m '/bar/d should be in the log' &&
+	git commit -m "/bar/d should be in the log" &&
 	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)" &&
 	mkdir newdir &&
 	echo new > newdir/dir &&
 	git update-index --add newdir/dir &&
-	git commit -m 'add a new directory' &&
+	git commit -m "add a new directory" &&
 	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)" &&
 	echo foo >> newdir/dir &&
 	git update-index newdir/dir &&
-	git commit -m 'modify a file in new directory' &&
+	git commit -m "modify a file in new directory" &&
 	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)"
+'
 
 test_expect_success 'dcommit should not fail with a touched file' '
 	test_commit "commit-new-file-foo2" foo2 &&
@@ -291,13 +291,13 @@ test_expect_success 'rebase should not fail with a touched file' '
 	git svn rebase
 '
 
-test_expect_success 'able to set-tree to a subdirectory' "
+test_expect_success 'able to set-tree to a subdirectory' '
 	echo cba > d &&
 	git update-index d &&
-	git commit -m 'update /bar/d' &&
+	git commit -m "update /bar/d" &&
 	git svn set-tree -i bar HEAD &&
-	test -z \"\$(git diff refs/heads/my-bar refs/remotes/bar)\"
-	"
+	test -z "$(git diff refs/heads/my-bar refs/remotes/bar)"
+'
 
 test_expect_success 'git-svn works in a bare repository' '
 	mkdir bare-repo &&

-- 
2.8.2.825.gea31738
Jeff King· May 13, 2016, 20:47 UTC · re: Jeff King · lore

[PATCH 3/6] t9107: use "return 1" instead of "exit 1"

When a test runs a loop, it cannot rely on the usual &&-chaining to propagate a failure inside the loop; it needs to break out with a failure signal. However, unless you are in a subshell, doing so with "exit 1" will exit the entire test script, not just the test snippet we are in (and cause the harness to complain that test_done was never reached).

So the fundamental point of this patch is s/exit/return/. But while we're there, let's fix a number of style and readability issues:

  - snippets in double-quotes need an extra layer of quoting
    for their meta-characters; let's avoid that by using
    single quotes
  - accumulating loop output by appending to a file in each
    iteration is brittle, as it can be affected by content
    left in the file by earlier tests. Instead, it's better
    to redirect stdout for the whole loop, so we know the
    output only comes from that loop.
  - using "test -z" to check that diff output is empty is
    overly verbose; we can just ask diff to use --exit-code.
  - we can factor out long lists of refs to make it more
    obvious we're using the same ones in each loop
  - subshells are unnecessary when ending an &&-chain with
    "|| return 1"
  - minor style fixups like space-after-redirection, and
    "do" and "done" on their own lines
Signed-off-by: Jeff King <peff@peff.net>
---
 t/t9107-git-svn-migrate.sh | 42 ++++++++++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 16 deletions(-)
diff --git a/t/t9107-git-svn-migrate.sh b/t/t9107-git-svn-migrate.sh
index 6efc2ab..2908aef 100755
--- a/t/t9107-git-svn-migrate.sh
+++ b/t/t9107-git-svn-migrate.sh
@@ -56,9 +56,11 @@ test_expect_success 'initialize a multi-repository repo' '
 	                        "^tags/\*:refs/remotes/origin/tags/\*$" &&
 	git config --add svn-remote.svn.fetch "branches/a:refs/remotes/origin/a" &&
 	git config --add svn-remote.svn.fetch "branches/b:refs/remotes/origin/b" &&
-	for i in tags/0.1 tags/0.2 tags/0.3; do
+	for i in tags/0.1 tags/0.2 tags/0.3
+	do
 		git config --add svn-remote.svn.fetch \
-		                 $i:refs/remotes/origin/$i || exit 1; done &&
+			$i:refs/remotes/origin/$i || return 1
+	done &&
 	git config --get-all svn-remote.svn.fetch > fetch.out &&
 	grep "^trunk:refs/remotes/origin/trunk$" fetch.out &&
 	grep "^branches/a:refs/remotes/origin/a$" fetch.out &&
@@ -70,30 +72,38 @@ test_expect_success 'initialize a multi-repository repo' '
 	'
 
 # refs should all be different, but the trees should all be the same:
-test_expect_success 'multi-fetch works on partial urls + paths' "
+test_expect_success 'multi-fetch works on partial urls + paths' '
+	refs="trunk a b tags/0.1 tags/0.2 tags/0.3" &&
 	git svn multi-fetch &&
-	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)\" &&
-	for i in trunk a b tags/0.1 tags/0.2 tags/0.3; do
-	  for j in trunk a b tags/0.1 tags/0.2 tags/0.3; do
-		if test \$j != \$i; then continue; fi
-	    test -z \"\$(git diff refs/remotes/origin/\$i \
-				 refs/remotes/origin/\$j)\" ||exit 1; done; done
-	"
+	for i in $refs
+	do
+		git rev-parse --verify refs/remotes/origin/$i^0 || return 1;
+	done >refs.out &&
+	test -z "$(sort <refs.out | uniq -d)" &&
+	>expect &&
+	for i in $refs
+	do
+		for j in $refs
+		do
+			git diff --exit-code refs/remotes/origin/$i refs/remotes/origin/$j ||
+				return 1
+		done
+	done
+'
 
 test_expect_success 'migrate --minimize on old inited layout' '
 	git config --unset-all svn-remote.svn.fetch &&
 	git config --unset-all svn-remote.svn.url &&
 	rm -rf "$GIT_DIR"/svn &&
-	for i in $(cat fetch.out); do
+	for i in $(cat fetch.out)
+	do
 		path=$(expr $i : "\([^:]*\):.*$")
 		ref=$(expr $i : "[^:]*:\(refs/remotes/.*\)$")
 		if test -z "$ref"; then continue; fi
 		if test -n "$path"; then path="/$path"; fi
-		( mkdir -p "$GIT_DIR"/svn/$ref/info/ &&
-		echo "$svnrepo"$path > "$GIT_DIR"/svn/$ref/info/url ) || exit 1;
+		mkdir -p "$GIT_DIR"/svn/$ref/info/ &&
+		echo "$svnrepo"$path >"$GIT_DIR"/svn/$ref/info/url ||
+		return 1
 	done &&
 	git svn migrate --minimize &&
 	test -z "$(git config -l | grep "^svn-remote\.git-svn\.")" &&

-- 
2.8.2.825.gea31738
Eric Wong· May 13, 2016, 22:59 UTC · re: Jeff King · lore

Re: [PATCH 3/6] t9107: use "return 1" instead of "exit 1"

Jeff King <peff@peff.net> wrote:
> +			git diff --exit-code refs/remotes/origin/$i refs/remotes/origin/$j ||
> +				return 1

80 columns; but I guess Junio picked it up, already. Otherwise the rest of the series looks good. Thanks.

Eric Sunshine· May 13, 2016, 23:45 UTC · re: Jeff King · lore

Re: [PATCH 3/6] t9107: use "return 1" instead of "exit 1"

On Fri, May 13, 2016 at 4:47 PM, Jeff King <peff@peff.net> wrote:
Show 33 quoted lines
> When a test runs a loop, it cannot rely on the usual
> &&-chaining to propagate a failure inside the loop; it needs
> to break out with a failure signal. However, unless you are
> in a subshell, doing so with "exit 1" will exit the entire
> test script, not just the test snippet we are in (and cause
> the harness to complain that test_done was never reached).
> [...snip...]
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> diff --git a/t/t9107-git-svn-migrate.sh b/t/t9107-git-svn-migrate.sh
> @@ -70,30 +72,38 @@ test_expect_success 'initialize a multi-repository repo' '
>  # refs should all be different, but the trees should all be the same:
> -test_expect_success 'multi-fetch works on partial urls + paths' "
> +test_expect_success 'multi-fetch works on partial urls + paths' '
> +       refs="trunk a b tags/0.1 tags/0.2 tags/0.3" &&
>         git svn multi-fetch &&
> -       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)\" &&
> -       for i in trunk a b tags/0.1 tags/0.2 tags/0.3; do
> -         for j in trunk a b tags/0.1 tags/0.2 tags/0.3; do
> -               if test \$j != \$i; then continue; fi
> -           test -z \"\$(git diff refs/remotes/origin/\$i \
> -                                refs/remotes/origin/\$j)\" ||exit 1; done; done
> -       "
> +       for i in $refs
> +       do
> +               git rev-parse --verify refs/remotes/origin/$i^0 || return 1;
> +       done >refs.out &&
> +       test -z "$(sort <refs.out | uniq -d)" &&
> +       >expect &&

What's this 'expect' file for? Is it leftover gunk from before you settled on 'diff --exit-code'?

Show 9 quoted lines
> +       for i in $refs
> +       do
> +               for j in $refs
> +               do
> +                       git diff --exit-code refs/remotes/origin/$i refs/remotes/origin/$j ||
> +                               return 1
> +               done
> +       done
> +'
Jeff King· May 13, 2016, 23:47 UTC · re: Eric Sunshine · lore

Re: [PATCH 3/6] t9107: use "return 1" instead of "exit 1"

On Fri, May 13, 2016 at 07:45:42PM -0400, Eric Sunshine wrote:
> > +       >expect &&
> 
> What's this 'expect' file for? Is it leftover gunk from before you
> settled on 'diff --exit-code'?
Oops, yes, that's exactly it.
-Peff
Junio C Hamano· May 14, 2016, 17:37 UTC · re: Jeff King · lore

Re: [PATCH 3/6] t9107: use "return 1" instead of "exit 1"

Jeff King <peff@peff.net> writes:
Show 10 quoted lines
> On Fri, May 13, 2016 at 07:45:42PM -0400, Eric Sunshine wrote:
>
>> > +       >expect &&
>> 
>> What's this 'expect' file for? Is it leftover gunk from before you
>> settled on 'diff --exit-code'?
>
> Oops, yes, that's exactly it.
>
> -Peff
Thanks for sharp eyes.  Let's squash this in, perhaps?
 t/t9107-git-svn-migrate.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/t9107-git-svn-migrate.sh b/t/t9107-git-svn-migrate.sh
index 2908aef..b6a99b8 100755
--- a/t/t9107-git-svn-migrate.sh
+++ b/t/t9107-git-svn-migrate.sh
@@ -80,12 +80,12 @@ test_expect_success 'multi-fetch works on partial urls + paths' '
 		git rev-parse --verify refs/remotes/origin/$i^0 || return 1;
 	done >refs.out &&
 	test -z "$(sort <refs.out | uniq -d)" &&
-	>expect &&
 	for i in $refs
 	do
 		for j in $refs
 		do
-			git diff --exit-code refs/remotes/origin/$i refs/remotes/origin/$j ||
+			git diff --exit-code refs/remotes/origin/$i \
+					     refs/remotes/origin/$j ||
 				return 1
 		done
 	done
Jeff King· May 14, 2016, 19:51 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/6] t9107: use "return 1" instead of "exit 1"

On Sat, May 14, 2016 at 10:37:07AM -0700, Junio C Hamano wrote:
Show 24 quoted lines
> Thanks for sharp eyes.  Let's squash this in, perhaps?
> 
>  t/t9107-git-svn-migrate.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/t/t9107-git-svn-migrate.sh b/t/t9107-git-svn-migrate.sh
> index 2908aef..b6a99b8 100755
> --- a/t/t9107-git-svn-migrate.sh
> +++ b/t/t9107-git-svn-migrate.sh
> @@ -80,12 +80,12 @@ test_expect_success 'multi-fetch works on partial urls + paths' '
>  		git rev-parse --verify refs/remotes/origin/$i^0 || return 1;
>  	done >refs.out &&
>  	test -z "$(sort <refs.out | uniq -d)" &&
> -	>expect &&
>  	for i in $refs
>  	do
>  		for j in $refs
>  		do
> -			git diff --exit-code refs/remotes/origin/$i refs/remotes/origin/$j ||
> +			git diff --exit-code refs/remotes/origin/$i \
> +					     refs/remotes/origin/$j ||
>  				return 1
>  		done
>  	done
Yeah, that looks perfect. Thanks.
-Peff
Jeff King· May 13, 2016, 20:47 UTC · re: Jeff King · lore

[PATCH 4/6] t9107: switch inverted single/double quotes in test

One of the test snippets in t9107 is enclosed in double quotes, but then uses single quotes to surround an interpolated variable inside the snippet, like:

  test_expect_success '...' "
	test -n '$head'
  "

This happens to work because the variable is interpolated _before_ the snippet is run, and the result is eval'd. So as long as the variable does not contain any single quotes, the two are equivalent. And it doesn't, as we know it is a sha1 from rev-parse above. But this construct is unnecessarily confusing.

But we can go a step further in cleaning up. The test is really checking that a particular ref has a value. Rather than checking if rev-parse produced output, we can just move rev-parse into the test itself, and rely on the exit code from --verify. Nobody else cares about the $head variable at all.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t9107-git-svn-migrate.sh | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/t/t9107-git-svn-migrate.sh b/t/t9107-git-svn-migrate.sh
index 2908aef..bb03722 100755
--- a/t/t9107-git-svn-migrate.sh
+++ b/t/t9107-git-svn-migrate.sh
@@ -24,8 +24,9 @@ test_expect_success 'setup old-looking metadata' '
 	git update-ref -d refs/remotes/git-svn refs/remotes/git-svn
 	'
 
-head=$(git rev-parse --verify refs/heads/git-svn-HEAD^0)
-test_expect_success 'git-svn-HEAD is a real HEAD' "test -n '$head'"
+test_expect_success 'git-svn-HEAD is a real HEAD' '
+	git rev-parse --verify refs/heads/git-svn-HEAD^0
+'
 
 svnrepo_escaped=$(echo $svnrepo | sed 's/ /%20/')
 

-- 
2.8.2.825.gea31738
Jeff King· May 13, 2016, 20:47 UTC · re: Jeff King · lore

[PATCH 5/6] t9103: modernize test style

The main goal here was to avoid double-quotes for surrounding the test snippet, since it makes the code hard to read (and to grep for common problems).

But while we're here, we can fix a few other things:
  - use test_path_* helpers, which are more robust and give
    better error messages
  - only "cd" inside a subshell, which leaves the
    environment pristine if further tests are added
  - consistently quote shell arguments. These aren't wrong
    if we assume find-rev output doesn't have any
    whitespace, but it doesn't hurt to be careful.
  - replace the old-style 'test x$foo = x' with 'test -z
    "$foo"'. Besides the quoting fix, this is the form we
    generally use in our test suite.
Signed-off-by: Jeff King <peff@peff.net>
---
 t/t9103-git-svn-tracked-directory-removed.sh | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/t/t9103-git-svn-tracked-directory-removed.sh b/t/t9103-git-svn-tracked-directory-removed.sh
index 3413164..b282713 100755
--- a/t/t9103-git-svn-tracked-directory-removed.sh
+++ b/t/t9103-git-svn-tracked-directory-removed.sh
@@ -23,17 +23,19 @@ test_expect_success 'make history for tracking' '
 
 test_expect_success 'clone repo with git' '
 	git svn clone -s "$svnrepo" x &&
-	test -f x/FOLLOWME &&
-	test ! -f x/README
+	test_path_is_file x/FOLLOWME &&
+	test_path_is_missing x/README
 '
 
-test_expect_success 'make sure r2 still has old file' "
-	cd x &&
-		test -n \"\$(git svn find-rev r1)\" &&
-		git reset --hard \$(git svn find-rev r1) &&
-		test -f README &&
-		test ! -f FOLLOWME &&
-		test x\$(git svn find-rev r2) = x
-"
+test_expect_success 'make sure r2 still has old file' '
+	(
+		cd x &&
+		test -n "$(git svn find-rev r1)" &&
+		git reset --hard "$(git svn find-rev r1)" &&
+		test_path_is_file README &&
+		test_path_is_missing FOLLOWME &&
+		test -z "$(git svn find-rev r2)"
+	)
+'
 
 test_done

-- 
2.8.2.825.gea31738
Jeff King· May 13, 2016, 20:47 UTC · re: Jeff King · lore

[PATCH 6/6] always quote shell arguments to test -z/-n

In shell code like:
  test -z $foo
  test -n $foo

that does not quote its arguments, it's easy to think that it is actually looking at the contents of $foo in each case. But if $foo is empty, then "test" does not see any argument at all! The results are quite subtle.

POSIX specifies that test's behavior depends on the number of arguments it sees, and if $foo is empty, it sees only one. The behavior in this case is:

  1 argument: Exit true (0) if $1 is not null; otherwise,
              exit false.

So in the "-z $foo" case, if $foo is empty, then we check that "-z" is non-null, and it returns success. Which happens to match what we expected. But for "-n $foo", if $foo is empty, we'll see that "-n" is non-null and still return success. That's the opposite of what we intended!

Furthermore, if $foo contains whitespace, we'll end up with more than 2 arguments. The results in this case are generally unspecified (unless the first part of $foo happens to be a valid binary operator, in which case the results are specified but certainly not what we intended).

And on top of this, even though "test -z $foo" _should_ work for the empty case, some older shells (reportedly ksh88) complain about the missing argument.

So let's make sure we consistently quote our variable arguments to "test". After this patch, the results of:

  git grep 'test -[zn] [^"]'
are empty.
Reported-by: Armin Kunaschik <megabreit@googlemail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 git-rebase--interactive.sh | 4 ++--
 git-stash.sh               | 4 ++--
 t/t4151-am-abort.sh        | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 9ea3075..470413b 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -866,12 +866,12 @@ add_exec_commands () {
 # $3: the input filename
 check_commit_sha () {
 	badsha=0
-	if test -z $1
+	if test -z "$1"
 	then
 		badsha=1
 	else
 		sha1_verif="$(git rev-parse --verify --quiet $1^{commit})"
-		if test -z $sha1_verif
+		if test -z "$sha1_verif"
 		then
 			badsha=1
 		fi
diff --git a/git-stash.sh b/git-stash.sh
index c7c65e2..c7509e8 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -185,7 +185,7 @@ store_stash () {
 
 	git update-ref --create-reflog -m "$stash_msg" $ref_stash $w_commit
 	ret=$?
-	test $ret != 0 && test -z $quiet &&
+	test $ret != 0 && test -z "$quiet" &&
 	die "$(eval_gettext "Cannot update \$ref_stash with \$w_commit")"
 	return $ret
 }
@@ -277,7 +277,7 @@ save_stash () {
 			git clean --force --quiet -d $CLEAN_X_OPTION
 		fi
 
-		if test "$keep_index" = "t" && test -n $i_tree
+		if test "$keep_index" = "t" && test -n "$i_tree"
 		then
 			git read-tree --reset -u $i_tree
 		fi
diff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh
index ea5ace9..9473c27 100755
--- a/t/t4151-am-abort.sh
+++ b/t/t4151-am-abort.sh
@@ -82,7 +82,7 @@ test_expect_success 'am -3 --abort removes otherfile-4' '
 	test 4 = "$(cat otherfile-4)" &&
 	git am --abort &&
 	test_cmp_rev initial HEAD &&
-	test -z $(git ls-files -u) &&
+	test -z "$(git ls-files -u)" &&
 	test_path_is_missing otherfile-4
 '
 

-- 
2.8.2.825.gea31738
Junio C Hamano· May 13, 2016, 22:09 UTC · re: Jeff King · lore

Re: [PATCH 0/6] test -z/-n quoting fix + misc cleanups

Jeff King <peff@peff.net> writes:
Show 10 quoted lines
> Anyway. Here's a series that fixes the -n/-z cases, along with a bunch
> of cleanups that remove the false positives (most of which I sent out
> just a few minutes ago as "minor fixes to some svn tests").
>
>   [1/6]: t/lib-git-svn: drop $remote_git_svn and $git_svn_id
>   [2/6]: t9100,t3419: enclose all test code in single-quotes
>   [3/6]: t9107: use "return 1" instead of "exit 1"
>   [4/6]: t9107: switch inverted single/double quotes in test
>   [5/6]: t9103: modernize test style
>   [6/6]: always quote shell arguments to test -z/-n
All look sensible; thanks.

← back to recent threads