Home » Mailing list archives » git » 2008 » April » 9

Re: [PATCH 1/8] git-rebase.sh: Fix --merge --abort failures when path contains whitespace

Previous thread: Re: ANNOUNCE: Git Forum by DigitalPig on Tuesday, April 8, 2008 - 5:49 pm. (2 messages)

Next thread: [PATCH] Fix documentation syntax of optional arguments in short options. by Carlos Rica on Tuesday, April 8, 2008 - 9:24 pm. (2 messages)
[view original message]
From: Bryan Donlan
Subject: [PATCH 0/8] Fix git's test suite to pass when the path contains spaces
Date: Tuesday, April 8, 2008 - 6:29 pm

This patch series fixes bugs in git and git's testsuite to allow all tests
to pass when the working directory contains whitespace and/or shell
metacharacters.

The first three patches in this series fix bugs in git itself that were
uncovered in the process of fixing the test suite. Each contains additional
tests and/or updates to existing tests to exercise the bug in question.

The remaining patches fix bugs in the test suite itself.

Bryan Donlan (8):
  git-rebase.sh: Fix --merge --abort failures when path contains
    whitespace
  config.c: Escape backslashes in section names properly
  git-send-email.perl: Handle shell metacharacters in $EDITOR properly
  test-lib.sh: Handle properly cases where the git checkout path
    contains whitespace
  test-lib.sh: Add a test_set_editor function to safely set $VISUAL
  lib-git-svn.sh: Handle paths with shell metacharacters correctly
  Use test_set_editor in t9001-send-email.sh
  Fix tests breaking when checkout path contains shell metacharacters

 config.c                                      |    2 +-
 git-rebase.sh                                 |    4 +-
 git-send-email.perl                           |    2 +-
 t/lib-git-svn.sh                              |   11 +++--
 t/t0000-basic.sh                              |    4 +-
 t/t0001-init.sh                               |    2 +-
 t/t1020-subdirectory.sh                       |   24 +++++-----
 t/t1303-wacky-config.sh                       |    6 +++
 t/t1501-worktree.sh                           |   14 +++---
 t/t3050-subprojects-fetch.sh                  |    2 +-
 t/t3404-rebase-interactive.sh                 |    3 +-
 t/t3407-rebase-abort.sh                       |   55 ++++++++++++++----------
 t/t5500-fetch-pack.sh                         |    2 +-
 t/t5512-ls-remote.sh                          |    2 +-
 t/t5516-fetch-push.sh                         |    8 ++--
 t/t5700-clone-reference.sh                    |    4 +-
 t/t5710-info-alternate.sh                     |  ...
[ message continues ]
[view original message]
From: Bryan Donlan
Subject: [PATCH 1/8] git-rebase.sh: Fix --merge --abort failures when path contains whitespace
Date: Tuesday, April 8, 2008 - 6:29 pm

Also update t/t3407-rebase-abort.sh to exercise the bug

Signed-off-by: Bryan Donlan <bdonlan@fushizen.net>
---
 git-rebase.sh           |    4 +-
 t/t3407-rebase-abort.sh |   55 +++++++++++++++++++++++++++-------------------
 2 files changed, 34 insertions(+), 25 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index 60c458f..389b5cb 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -42,7 +42,7 @@ To restore the original branch and stop rebasing run \"git rebase --abort\".
 unset newbase
 strategy=recursive
 do_merge=
-dotest=$GIT_DIR/.dotest-merge
+dotest="$GIT_DIR/.dotest-merge"
 prec=4
 verbose=
 git_am_opt=
@@ -214,7 +214,7 @@ do
 		else
 			die "No rebase in progress?"
 		fi
-		git reset --hard $(cat $dotest/orig-head)
+		git reset --hard $(cat "$dotest/orig-head")
 		rm -r "$dotest"
 		exit
 		;;
diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 37944c3..396a354 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -4,7 +4,13 @@ test_description='git rebase --abort tests'
 
 . ./test-lib.sh
 
+### Test that we handle strange characters properly
+work_dir="$(pwd)/test \" ' \$ \\ dir"
+
 test_expect_success setup '
+	mkdir -p "$work_dir" &&
+	cd "$work_dir" &&
+	git init &&
 	echo a > a &&
 	git add a &&
 	git commit -m a &&
@@ -27,42 +33,45 @@ testrebase() {
 	type=$1
 	dotest=$2
 
-	test_expect_success "rebase$type --abort" '
+	test_expect_success "rebase$type --abort" "
+		cd \"\$work_dir\" &&
 		# Clean up the state from the previous one
-		git reset --hard pre-rebase
-		test_must_fail git rebase'"$type"' master &&
-		test -d '$dotest' &&
+		git reset --hard pre-rebase &&
+		test_must_fail git rebase$type master &&
+		test -d \"\$dotest\" &&
 		git rebase --abort &&
-		test $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) &&
-		test ! -d '$dotest'
-	'
+		test \$(git rev-parse to-rebase) = \$(git rev-parse pre-rebase) &&
+		test ! -d \"\$dotest\"
+	"
 
-	test_expect_success ...
[ message continues ]
[view original message]
From: Bryan Donlan
Subject: [PATCH 2/8] config.c: Escape backslashes in section names properly
Date: Tuesday, April 8, 2008 - 6:29 pm

Signed-off-by: Bryan Donlan <bdonlan@fushizen.net>
---
 config.c                |    2 +-
 t/t1303-wacky-config.sh |    6 ++++++
 2 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/config.c b/config.c
index 0624494..6c07245 100644
--- a/config.c
+++ b/config.c
@@ -672,7 +672,7 @@ static int store_write_section(int fd, const char* key)
 	if (dot) {
 		strbuf_addf(&sb, "[%.*s \"", (int)(dot - key), key);
 		for (i = dot - key + 1; i < store.baselen; i++) {
-			if (key[i] == '"')
+			if (key[i] == '"' || key[i] == '\\')
 				strbuf_addch(&sb, '\\');
 			strbuf_addch(&sb, key[i]);
 		}
diff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh
index 99985dc..f366b53 100755
--- a/t/t1303-wacky-config.sh
+++ b/t/t1303-wacky-config.sh
@@ -34,4 +34,10 @@ test_expect_success 'add key in different section' '
 	check section2.key bar
 '
 
+SECTION="test.q\"s\\sq'sp e.key"
+test_expect_success 'make sure git-config escapes section names properly' '
+	git config "$SECTION" bar &&
+	check "$SECTION" bar
+'
+
 test_done
-- 
1.5.5.8.gbbd98

--


[ message continues ]
[view original message]
From: Bryan Donlan
Subject: [PATCH 3/8] git-send-email.perl: Handle shell metacharacters in $EDITOR properly
Date: Tuesday, April 8, 2008 - 6:29 pm

Also, update t/t9001-send-email.sh to test for this bug.

Signed-off-by: Bryan Donlan <bdonlan@fushizen.net>
---
 git-send-email.perl   |    2 +-
 t/t9001-send-email.sh |    8 ++++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index be4a20d..975df1c 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -510,7 +510,7 @@ EOT
 	close(C);
 
 	my $editor = $ENV{GIT_EDITOR} || $repo->config("core.editor") || $ENV{VISUAL} || $ENV{EDITOR} || "vi";
-	system('sh', '-c', '$0 $@', $editor, $compose_filename);
+	system('sh', '-c', $editor.' $@', $editor, $compose_filename);
 
 	open(C2,">",$compose_filename . ".final")
 		or die "Failed to open $compose_filename.final : " . $!;
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index c0973b4..030f66c 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -139,15 +139,19 @@ test_expect_success 'Valid In-Reply-To when prompting' '
 
 test_expect_success 'setup fake editor' '
 	(echo "#!/bin/sh" &&
-	 echo "echo fake edit >>\$1"
+	 echo "echo fake edit >>\"\$1\""
 	) >fake-editor &&
 	chmod +x fake-editor
 '
 
+FAKE_EDITOR="$(pwd)/fake-editor"
+export FAKE_EDITOR
+GIT_EDITOR='"$FAKE_EDITOR"'
+export GIT_EDITOR
+
 test_expect_success '--compose works' '
 	clean_fake_sendmail &&
 	echo y | \
-		GIT_EDITOR=$(pwd)/fake-editor \
 		GIT_SEND_EMAIL_NOTTY=1 \
 		git send-email \
 		--compose --subject foo \
-- 
1.5.5.8.gbbd98

--


[ message continues ]
[view original message]
From: Bryan Donlan
Subject: [PATCH 4/8] test-lib.sh: Fix some missing path quoting
Date: Tuesday, April 8, 2008 - 6:30 pm

Signed-off-by: Bryan Donlan <bdonlan@fushizen.net>
---
 t/test-lib.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 7c2a8ba..17d4bc0 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -329,7 +329,7 @@ test_create_repo () {
 	repo="$1"
 	mkdir "$repo"
 	cd "$repo" || error "Cannot setup test environment"
-	"$GIT_EXEC_PATH/git" init --template=$GIT_EXEC_PATH/templates/blt/ >/dev/null 2>&1 ||
+	"$GIT_EXEC_PATH/git" init "--template=$GIT_EXEC_PATH/templates/blt/" >/dev/null 2>&1 ||
 	error "cannot run git init -- have you built things yet?"
 	mv .git/hooks .git/hooks-disabled
 	cd "$owd"
-- 
1.5.5.8.gbbd98

--


[ message continues ]
[view original message]
From: Bryan Donlan
Subject: [PATCH 5/8] test-lib.sh: Add a test_set_editor function to safely set $VISUAL
Date: Tuesday, April 8, 2008 - 6:30 pm

In particular, this function correctly handles cases where the pwd contains
spaces, quotes, and other troublesome metacharacters.

Signed-off-by: Bryan Donlan <bdonlan@fushizen.net>
---
 t/test-lib.sh |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 17d4bc0..6f43376 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -160,6 +160,15 @@ die () {
 
 trap 'die' exit
 
+test_set_editor () {
+	# If the editor path contains a quote, just using VISUAL='"path"' isn't
+	# enough.
+	FAKE_EDITOR="$1"
+	export FAKE_EDITOR
+	VISUAL='"$FAKE_EDITOR"'
+	export VISUAL
+}
+
 test_tick () {
 	if test -z "${test_tick+set}"
 	then
-- 
1.5.5.8.gbbd98

--


[ message continues ]
[view original message]
From: Bryan Donlan
Subject: [PATCH 6/8] lib-git-svn.sh: Fix quoting issues with paths containing shell metacharacters
Date: Tuesday, April 8, 2008 - 6:30 pm

Signed-off-by: Bryan Donlan <bdonlan@fushizen.net>
---
 t/lib-git-svn.sh |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh
index 9decd2e..1b37ba8 100644
--- a/t/lib-git-svn.sh
+++ b/t/lib-git-svn.sh
@@ -7,9 +7,9 @@ then
 	exit
 fi
 
-GIT_DIR=$PWD/.git
-GIT_SVN_DIR=$GIT_DIR/svn/git-svn
-SVN_TREE=$GIT_SVN_DIR/svn-tree
+GIT_DIR="$PWD/.git"
+GIT_SVN_DIR="$GIT_DIR/svn/git-svn"
+SVN_TREE="$GIT_SVN_DIR/svn-tree"
 
 svn >/dev/null 2>&1
 if test $? -ne 1
@@ -19,13 +19,14 @@ then
     exit
 fi
 
-svnrepo=$PWD/svnrepo
+svnrepo="$PWD/svnrepo"
+export svnrepo
 
 perl -w -e "
 use SVN::Core;
 use SVN::Repos;
 \$SVN::Core::VERSION gt '1.1.0' or exit(42);
-system(qw/svnadmin create --fs-type fsfs/, '$svnrepo') == 0 or exit(41);
+system(qw/svnadmin create --fs-type fsfs/, \$ENV{svnrepo}) == 0 or exit(41);
 " >&3 2>&4
 x=$?
 if test $x -ne 0
-- 
1.5.5.8.gbbd98

--


[ message continues ]
[view original message]
From: Bryan Donlan
Subject: [PATCH 7/8] Use test_set_editor in t9001-send-email.sh
Date: Tuesday, April 8, 2008 - 6:30 pm

Signed-off-by: Bryan Donlan <bdonlan@fushizen.net>
---
 t/t9001-send-email.sh |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 030f66c..0a65785 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -144,10 +144,7 @@ test_expect_success 'setup fake editor' '
 	chmod +x fake-editor
 '
 
-FAKE_EDITOR="$(pwd)/fake-editor"
-export FAKE_EDITOR
-GIT_EDITOR='"$FAKE_EDITOR"'
-export GIT_EDITOR
+test_set_editor "$(pwd)/fake-editor"
 
 test_expect_success '--compose works' '
 	clean_fake_sendmail &&
-- 
1.5.5.8.gbbd98

--


[ message continues ]
[view original message]
From: Bryan Donlan
Subject: [PATCH 8/8] Fix tests breaking when checkout path contains shell metacharacters
Date: Tuesday, April 8, 2008 - 6:30 pm

This fixes the remainder of the issues where the test script itself is at
fault for failing when the git checkout path contains whitespace or other
shell metacharacters.

Signed-off-by: Bryan Donlan <bdonlan@fushizen.net>
---
 t/t0000-basic.sh                              |    4 +-
 t/t0001-init.sh                               |    2 +-
 t/t1020-subdirectory.sh                       |   24 +++++-----
 t/t1501-worktree.sh                           |   14 +++---
 t/t3050-subprojects-fetch.sh                  |    2 +-
 t/t3404-rebase-interactive.sh                 |    3 +-
 t/t5500-fetch-pack.sh                         |    2 +-
 t/t5512-ls-remote.sh                          |    2 +-
 t/t5516-fetch-push.sh                         |    8 ++--
 t/t5700-clone-reference.sh                    |    4 +-
 t/t5710-info-alternate.sh                     |    4 +-
 t/t7003-filter-branch.sh                      |    4 +-
 t/t7010-setup.sh                              |    2 +-
 t/t7300-clean.sh                              |    2 +-
 t/t7501-commit.sh                             |    8 ++--
 t/t7504-commit-msg-hook.sh                    |   20 +++++-----
 t/t7505-prepare-commit-msg-hook.sh            |   14 +++---
 t/t9100-git-svn-basic.sh                      |   54 ++++++++++++------------
 t/t9101-git-svn-props.sh                      |    6 +-
 t/t9102-git-svn-deep-rmdir.sh                 |    6 +-
 t/t9103-git-svn-tracked-directory-removed.sh  |   30 +++++++-------
 t/t9104-git-svn-follow-parent.sh              |   50 +++++++++++-----------
 t/t9105-git-svn-commit-diff.sh                |   12 +++---
 t/t9106-git-svn-commit-diff-clobber.sh        |   14 +++---
 t/t9106-git-svn-dcommit-clobber-series.sh     |    6 +-
 t/t9107-git-svn-migrate.sh                    |   48 +++++++++++-----------
 t/t9108-git-svn-glob.sh                       |    8 ++--
 t/t9110-git-svn-use-svm-props.sh              |    8 ++--
 t/t9111-git-svn-use-svnsync-props.sh          |    8 ++--
 ...
[ message continues ]
[view original message]
From: Johannes Sixt
Subject: Re: [PATCH 7/8] Use test_set_editor in t9001-send-email.sh
Date: Wednesday, April 9, 2008 - 12:01 am

I'd squash this into 5/8 or 8/8. Dunno.

-- Hannes

--


[ message continues ]
[view original message]
From: Bryan Donlan
Subject: Re: [PATCH 7/8] Use test_set_editor in t9001-send-email.sh
Date: Wednesday, April 9, 2008 - 7:16 am

I split it out as it's not really related to the goals of 5/8 or 8/8,
but is rather pure elimination of code duplication. However if it's
considered better to squash it into 5/8 I'd be happy to do so.

Thanks,

Bryan
--


[ message continues ]
[view original message]
From: Johannes Sixt
Subject: Re: [PATCH 5/8] test-lib.sh: Add a test_set_editor function to safely set $VISUAL
Date: Tuesday, April 8, 2008 - 11:36 pm

Clever! It assumes that $VISUAL is always run as sh -c "$VISUAL ...", but
I think this assumption is valid. A hint along these lines in the comment
could turn out helpful.

-- Hannes

--


[ message continues ]
[view original message]
From: Johannes Sixt
Subject: Re: [PATCH 3/8] git-send-email.perl: Handle shell metacharacters in $EDITOR properly
Date: Tuesday, April 8, 2008 - 11:31 pm

Your defintion of "properly"? I didn't immediately see what is wrong with
the status quo and, hence, why your fix would solve a problem.

-- Hannes

--


[ message continues ]
[view original message]
From: Bryan Donlan
Subject: Re: [PATCH 3/8] git-send-email.perl: Handle shell metacharacters in $EDITOR properly
Date: Wednesday, April 9, 2008 - 7:28 am

This fixes the git-send-perl semantics for launching an editor when
$GIT_EDITOR (or friends) contains shell metacharacters to match
launch_editor() in builtin-tag.c. If we use the current approach
(sh -c '$0 $@' "$EDITOR" files ...), we see it fails when $EDITOR has
shell metacharacters:

$ sh -x -c '$0 $@' "$VISUAL" "foo"
+ "$FAKE_EDITOR" foo
"$FAKE_EDITOR": 1: "$FAKE_EDITOR": not found

Whereas builtin-tag.c will invoke sh -c "$EDITOR \"$@\"".

Note that with the method git-send-perl uses currently, it's difficult
(impossible?) to deal with a editor path that contains arbitrary shell
metacharacters. It's certainly impossible when the various git tools use
different semantics :)

I'll amend the commit message along with the other suggested changes.
--


[ message continues ]
[view original message]
From: Junio C Hamano
Subject: Re: [PATCH 3/8] git-send-email.perl: Handle shell metacharacters in $EDITOR properly
Date: Wednesday, April 9, 2008 - 8:39 pm

I agree the description of this series is a bit too sketchy.  I suspect
even Bryan would not remember what the broken behaviour was and what's the
new accepted behaviour is three months from now.
--


[ message continues ]
[view original message]
From: Johannes Sixt
Subject: Re: [PATCH 2/8] config.c: Escape backslashes in section names properly
Date: Tuesday, April 8, 2008 - 11:31 pm

What is your definition of "properly"? Please give an example of what went
wrong.

-- Hannes

--


[ message continues ]
[view original message]
From: Bryan Donlan
Subject: Re: [PATCH 2/8] config.c: Escape backslashes in section names properly
Date: Wednesday, April 9, 2008 - 7:13 am

The included patch to the test suite is an example - specifically if an
element of the configuration key name other than the first or last
contains a backslash, it would not be escaped on output, but would be
treated as an escape sequence on input. Thus the backslash would be
lost.
--


[ message continues ]
[view original message]
From: Johannes Sixt
Subject: Re: [PATCH 2/8] config.c: Escape backslashes in section names properly
Date: Wednesday, April 9, 2008 - 7:25 am

Could you then please add this to the commit message, most importantly,
the last sentence?

-- Hannes
--


[ message continues ]
[view original message]
From: Johannes Sixt
Subject: Re: [PATCH 1/8] git-rebase.sh: Fix --merge --abort failures when path contains whitespace
Date: Tuesday, April 8, 2008 - 11:55 pm

This is not strictly necessary: The RHS expression of an assignment does
not undergo IFS splitting; but better safe than sorry. (But note that
'export foo=$bar', which is not POSIX, is *not* an assignment, and

In effect, you modify only this test to stress-test strange characters,
but other tests in the test suite still run in a "sane" environment. IOW,
I don't think you should go to this extreme for this one test only. The
better approach would be to rename 'trash' in test-lib.sh to this strange


Ditto.

Apart from that, this looks good.

-- Hannes

--


[ message continues ]
[view original message]
From: Bryan Donlan
Subject: Re: [PATCH 1/8] git-rebase.sh: Fix --merge --abort failures when path contains whitespace
Date: Wednesday, April 9, 2008 - 7:37 am

Since Junio asked(?) that it be removed, I'll drop it from the next rev

I can do that, but it'd have to come as the last patch in the patchset,
or it would obviously cause test failures. My goal here was to ensure
that the bug I fixed in the patch would be tested in an isolated manner.

If you like I can add a change to the trash directory to the next rev of

I assumed that if git reset failed here we'd probably want to know :)

Thanks,

Bryan
--


[ message continues ]
[view original message]
From: Johannes Sixt
Subject: Re: [PATCH 1/8] git-rebase.sh: Fix --merge --abort failures when path contains whitespace
Date: Wednesday, April 9, 2008 - 7:51 am

I for my taste could live without a test because the fix is so obvious.
Given that you have to prepend 'cd "$work_dir"' to *all* the tests, this
really just highlights that for the purpose of this test the name 'trash'
of the regular test directory is just too simple ;)

Of course, it's Junio's draw, and he likes to have tests for fixes that


On second sight, this indeed looks just like an omission and your change
is good.

-- Hannes

--


[ message continues ]
[view original message]
From: Bryan Donlan
Subject: Re: [PATCH 1/8] git-rebase.sh: Fix --merge --abort failures when path contains whitespace
Date: Wednesday, April 9, 2008 - 8:02 am

How exotic ought it be? I'm not entirely sure which characters are
allowed on windows or any other non-unixes that git supports (are there
any?)

Thanks,

Bryan
--


[ message continues ]
[view original message]
From: Johannes Sixt
Subject: Re: [PATCH 1/8] git-rebase.sh: Fix --merge --abort failures when path contains whitespace
Date: Wednesday, April 9, 2008 - 11:10 pm

What you had in your patch is OK: " ' $ The backslash is also a notable
special character, but it is a directory separator on Windows, so it
should not be in the name.

-- Hannes

--


[ message continues ]
[view original message]
From: Bryan Donlan
Subject: [PATCH v2 00/10] Fix git's test suite to pass when the path contains spaces
Date: Wednesday, April 9, 2008 - 11:50 pm

This patch series fixes bugs in git and git's testsuite to allow all tests
to pass when the working directory contains whitespace and/or shell
metacharacters.

The first three patches in this series fix bugs in git itself that were
uncovered in the process of fixing the test suite. Each contains additional
tests and/or updates to existing tests to exercise the bug in question.

Patches 4 and 5 set up an abstraction for safe handling of editor variables
in tests when we have odd paths, and 6-8 fix bugs in the test suite itself.
Patch 9 cleans up some instances of the unportable export FOO=bar construct
in the test suite; and 10 arranges for the test suite to always run with the
trash directory name containing various metacharacters, to help prevent
regressions in the future.

Signifigant changes since v1:
  * Rebase against master
  * Drop some unnecessary quotation additions
  * Use "$@" not $@ in editor invocations in the fix to git-send-email, to
    match builtin-tag.c
  * Be more verbose in the config.c and git-send-email.perl patch commit
    messages
  * Rename 't/trash' to 't/trash directory' to try to trip up more bugs in
    the future
  * Add a patch to convert all export FOO=bar to FOO=bar; export FOO in t/*.sh
  * Noted why t/t7505-prepare-commit-msg-hook.sh isn't using test_set_editor
    in a comment.
  * Clarified the purpose and implementation of test_set_editor in a comment.

This series is also available as a git branch:

  git pull git://repo.or.cz/git/bdonlan.git tags/wst-submit2

Bryan Donlan (10):
  git-rebase.sh: Fix --merge --abort failures when path contains
    whitespace
  config.c: Escape backslashes in section names properly
  git-send-email.perl: Handle shell metacharacters in $EDITOR properly
  test-lib.sh: Add a test_set_editor function to safely set $VISUAL
  Use test_set_editor in t9001-send-email.sh
  test-lib.sh: Fix some missing path quoting
  lib-git-svn.sh: Fix quoting issues with paths containing shell
    metacharacters
  ...
[ message continues ]
[view original message]
From: Bryan Donlan
Subject: [PATCH v2 01/10] git-rebase.sh: Fix --merge --abort failures when path contains whitespace
Date: Wednesday, April 9, 2008 - 11:50 pm

Also update t/t3407-rebase-abort.sh to exercise the bug

Signed-off-by: Bryan Donlan <bdonlan@fushizen.net>
---
 git-rebase.sh           |    2 +-
 t/t3407-rebase-abort.sh |   55 +++++++++++++++++++++++++++-------------------
 2 files changed, 33 insertions(+), 24 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index 9b13b83..c43afe5 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -214,7 +214,7 @@ do
 		else
 			die "No rebase in progress?"
 		fi
-		git reset --hard $(cat $dotest/orig-head)
+		git reset --hard $(cat "$dotest/orig-head")
 		rm -r "$dotest"
 		exit
 		;;
diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 37944c3..396a354 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -4,7 +4,13 @@ test_description='git rebase --abort tests'
 
 . ./test-lib.sh
 
+### Test that we handle strange characters properly
+work_dir="$(pwd)/test \" ' \$ \\ dir"
+
 test_expect_success setup '
+	mkdir -p "$work_dir" &&
+	cd "$work_dir" &&
+	git init &&
 	echo a > a &&
 	git add a &&
 	git commit -m a &&
@@ -27,42 +33,45 @@ testrebase() {
 	type=$1
 	dotest=$2
 
-	test_expect_success "rebase$type --abort" '
+	test_expect_success "rebase$type --abort" "
+		cd \"\$work_dir\" &&
 		# Clean up the state from the previous one
-		git reset --hard pre-rebase
-		test_must_fail git rebase'"$type"' master &&
-		test -d '$dotest' &&
+		git reset --hard pre-rebase &&
+		test_must_fail git rebase$type master &&
+		test -d \"\$dotest\" &&
 		git rebase --abort &&
-		test $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) &&
-		test ! -d '$dotest'
-	'
+		test \$(git rev-parse to-rebase) = \$(git rev-parse pre-rebase) &&
+		test ! -d \"\$dotest\"
+	"
 
-	test_expect_success "rebase$type --abort after --skip" '
+	test_expect_success "rebase$type --abort after --skip" "
+		cd \"\$work_dir\" &&
 		# Clean up the state from the previous one
-		git reset --hard pre-rebase
-		test_must_fail git rebase'"$type"' master ...
[ message continues ]
[view original message]
From: Bryan Donlan
Subject: [PATCH v2 02/10] config.c: Escape backslashes in section names properly
Date: Wednesday, April 9, 2008 - 11:50 pm

If an element of the configuration key name other than the first or last
contains a backslash, it is not escaped on output, but is treated as an escape
sequence on input. Thus, the backslash is lost when re-loading the
configuration.

This patch corrects this by having backslashes escaped properly, and
introduces a new test for this bug.

Signed-off-by: Bryan Donlan <bdonlan@fushizen.net>
---
 config.c                |    2 +-
 t/t1303-wacky-config.sh |    6 ++++++
 2 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/config.c b/config.c
index 0624494..6c07245 100644
--- a/config.c
+++ b/config.c
@@ -672,7 +672,7 @@ static int store_write_section(int fd, const char* key)
 	if (dot) {
 		strbuf_addf(&sb, "[%.*s \"", (int)(dot - key), key);
 		for (i = dot - key + 1; i < store.baselen; i++) {
-			if (key[i] == '"')
+			if (key[i] == '"' || key[i] == '\\')
 				strbuf_addch(&sb, '\\');
 			strbuf_addch(&sb, key[i]);
 		}
diff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh
index 99985dc..f366b53 100755
--- a/t/t1303-wacky-config.sh
+++ b/t/t1303-wacky-config.sh
@@ -34,4 +34,10 @@ test_expect_success 'add key in different section' '
 	check section2.key bar
 '
 
+SECTION="test.q\"s\\sq'sp e.key"
+test_expect_success 'make sure git-config escapes section names properly' '
+	git config "$SECTION" bar &&
+	check "$SECTION" bar
+'
+
 test_done
-- 
1.5.5.33.gc0a39.dirty

--


[ message continues ]
[view original message]
From: Bryan Donlan
Subject: [PATCH v2 03/10] git-send-email.perl: Handle shell metacharacters in $EDITOR properly
Date: Wednesday, April 9, 2008 - 11:50 pm

This fixes the git-send-perl semantics for launching an editor when
$GIT_EDITOR (or friends) contains shell metacharacters to match
launch_editor() in builtin-tag.c. If we use the current approach
(sh -c '$0 $@' "$EDITOR" files ...), we see it fails when $EDITOR has
shell metacharacters:

$ sh -x -c '$0 $@' "$VISUAL" "foo"
+ "$FAKE_EDITOR" foo
"$FAKE_EDITOR": 1: "$FAKE_EDITOR": not found

Whereas builtin-tag.c will invoke sh -c "$EDITOR \"$@\"".

Thus, this patch changes git-send-email.perl to use the same method as the
C utilities, and additionally updates t/t9001-send-email.sh to test for this bug.

Signed-off-by: Bryan Donlan <bdonlan@fushizen.net>
---
 git-send-email.perl   |    2 +-
 t/t9001-send-email.sh |    8 ++++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 9e568bf..b502396 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -512,7 +512,7 @@ EOT
 	close(C);
 
 	my $editor = $ENV{GIT_EDITOR} || Git::config(@repo, "core.editor") || $ENV{VISUAL} || $ENV{EDITOR} || "vi";
-	system('sh', '-c', '$0 $@', $editor, $compose_filename);
+	system('sh', '-c', $editor.' "$@"', $editor, $compose_filename);
 
 	open(C2,">",$compose_filename . ".final")
 		or die "Failed to open $compose_filename.final : " . $!;
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index c0973b4..030f66c 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -139,15 +139,19 @@ test_expect_success 'Valid In-Reply-To when prompting' '
 
 test_expect_success 'setup fake editor' '
 	(echo "#!/bin/sh" &&
-	 echo "echo fake edit >>\$1"
+	 echo "echo fake edit >>\"\$1\""
 	) >fake-editor &&
 	chmod +x fake-editor
 '
 
+FAKE_EDITOR="$(pwd)/fake-editor"
+export FAKE_EDITOR
+GIT_EDITOR='"$FAKE_EDITOR"'
+export GIT_EDITOR
+
 test_expect_success '--compose works' '
 	clean_fake_sendmail &&
 	echo y | \
-		GIT_EDITOR=$(pwd)/fake-editor \
 		GIT_SEND_EMAIL_NOTTY=1 \
 		git send-email \
 ...
[ message continues ]
[view original message]
From: Bryan Donlan
Subject: [PATCH v2 04/10] test-lib.sh: Add a test_set_editor function to safely set $VISUAL
Date: Wednesday, April 9, 2008 - 11:50 pm

In particular, this function correctly handles cases where the pwd contains
spaces, quotes, and other troublesome metacharacters.

Signed-off-by: Bryan Donlan <bdonlan@fushizen.net>
---
 t/test-lib.sh |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 7c2a8ba..d7ad13b 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -160,6 +160,22 @@ die () {
 
 trap 'die' exit
 
+# The semantics of the editor variables are that of invoking
+# sh -c "$EDITOR \"$@\"" files ...
+#
+# If our trash directory contains shell metacharacters, they will be
+# interpreted if we just set $EDITOR directly, so do a little dance with
+# environment variables to work around this.
+#
+# In particular, quoting isn't enough, as the path may contain the same quote
+# that we're using. 
+test_set_editor () {
+	FAKE_EDITOR="$1"
+	export FAKE_EDITOR
+	VISUAL='"$FAKE_EDITOR"'
+	export VISUAL
+}
+
 test_tick () {
 	if test -z "${test_tick+set}"
 	then
-- 
1.5.5.33.gc0a39.dirty

--


[ message continues ]
[view original message]
From: Bryan Donlan
Subject: [PATCH v2 05/10] Use test_set_editor in t9001-send-email.sh
Date: Wednesday, April 9, 2008 - 11:50 pm

Signed-off-by: Bryan Donlan <bdonlan@fushizen.net>
---
 t/t9001-send-email.sh |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 030f66c..0a65785 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -144,10 +144,7 @@ test_expect_success 'setup fake editor' '
 	chmod +x fake-editor
 '
 
-FAKE_EDITOR="$(pwd)/fake-editor"
-export FAKE_EDITOR
-GIT_EDITOR='"$FAKE_EDITOR"'
-export GIT_EDITOR
+test_set_editor "$(pwd)/fake-editor"
 
 test_expect_success '--compose works' '
 	clean_fake_sendmail &&
-- 
1.5.5.33.gc0a39.dirty

--


[ message continues ]
[view original message]
From: Bryan Donlan
Subject: [PATCH v2 06/10] test-lib.sh: Fix some missing path quoting
Date: Wednesday, April 9, 2008 - 11:50 pm

Signed-off-by: Bryan Donlan <bdonlan@fushizen.net>
---
 t/test-lib.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index d7ad13b..04e098b 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -345,7 +345,7 @@ test_create_repo () {
 	repo="$1"
 	mkdir "$repo"
 	cd "$repo" || error "Cannot setup test environment"
-	"$GIT_EXEC_PATH/git" init --template=$GIT_EXEC_PATH/templates/blt/ >/dev/null 2>&1 ||
+	"$GIT_EXEC_PATH/git" init "--template=$GIT_EXEC_PATH/templates/blt/" >/dev/null 2>&1 ||
 	error "cannot run git init -- have you built things yet?"
 	mv .git/hooks .git/hooks-disabled
 	cd "$owd"
-- 
1.5.5.33.gc0a39.dirty

--


[ message continues ]
[view original message]
From: Bryan Donlan
Subject: [PATCH v2 07/10] lib-git-svn.sh: Fix quoting issues with paths containing shell metacharacters
Date: Wednesday, April 9, 2008 - 11:50 pm

Signed-off-by: Bryan Donlan <bdonlan@fushizen.net>
---
 t/lib-git-svn.sh |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh
index 9decd2e..445df78 100644
--- a/t/lib-git-svn.sh
+++ b/t/lib-git-svn.sh
@@ -20,12 +20,13 @@ then
 fi
 
 svnrepo=$PWD/svnrepo
+export svnrepo
 
 perl -w -e "
 use SVN::Core;
 use SVN::Repos;
 \$SVN::Core::VERSION gt '1.1.0' or exit(42);
-system(qw/svnadmin create --fs-type fsfs/, '$svnrepo') == 0 or exit(41);
+system(qw/svnadmin create --fs-type fsfs/, \$ENV{svnrepo}) == 0 or exit(41);
 " >&3 2>&4
 x=$?
 if test $x -ne 0
-- 
1.5.5.33.gc0a39.dirty

--


[ message continues ]
[view original message]
From: Bryan Donlan
Subject: [PATCH v2 08/10] Don't use the export NAME=value form in the test scripts.
Date: Wednesday, April 9, 2008 - 11:50 pm

This form is not portable across all shells, so replace instances of:

  export FOO=bar

with:

  FOO=bar
  export FOO

Signed-off-by: Bryan Donlan <bdonlan@fushizen.net>
---
 t/lib-httpd.sh                         |    3 +-
 t/t1500-rev-parse.sh                   |    9 ++++---
 t/t1501-worktree.sh                    |   34 ++++++++++++++++---------------
 t/t3400-rebase.sh                      |    3 +-
 t/t3500-cherry.sh                      |    3 +-
 t/t5500-fetch-pack.sh                  |    2 +-
 t/t6000lib.sh                          |    9 +++++--
 t/t6010-merge-base.sh                  |    9 ++++---
 t/t7004-tag.sh                         |    3 +-
 t/t9500-gitweb-standalone-no-errors.sh |   16 ++++++++------
 10 files changed, 52 insertions(+), 39 deletions(-)

diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 7f206c5..a5c4436 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -61,7 +61,8 @@ prepare_httpd() {
 			-new -x509 -nodes \
 			-out $HTTPD_ROOT_PATH/httpd.pem \
 			-keyout $HTTPD_ROOT_PATH/httpd.pem
-		export GIT_SSL_NO_VERIFY=t
+		GIT_SSL_NO_VERIFY=t
+		export GIT_SSL_NO_VERIFY
 		HTTPD_PARA="$HTTPD_PARA -DSSL"
 	else
 		HTTPD_URL=http://127.0.0.1:$LIB_HTTPD_PORT
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 38a2bf0..85da4ca 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -51,8 +51,9 @@ test_rev_parse 'core.bare undefined' false false true
 
 mkdir work || exit 1
 cd work || exit 1
-export GIT_DIR=../.git
-export GIT_CONFIG="$(pwd)"/../.git/config
+GIT_DIR=../.git
+GIT_CONFIG="$(pwd)"/../.git/config
+export GIT_DIR GIT_CONFIG
 
 git config core.bare false
 test_rev_parse 'GIT_DIR=../.git, core.bare = false' false false true ''
@@ -64,8 +65,8 @@ git config --unset core.bare
 test_rev_parse 'GIT_DIR=../.git, core.bare undefined' false false true ''
 
 mv ../.git ../repo.git || exit 1
-export GIT_DIR=../repo.git
-export ...
[ message continues ]
[view original message]
From: Bryan Donlan
Subject: [PATCH v2 09/10] Fix tests breaking when checkout path contains shell metacharacters
Date: Wednesday, April 9, 2008 - 11:50 pm

This fixes the remainder of the issues where the test script itself is at
fault for failing when the git checkout path contains whitespace or other
shell metacharacters.

Signed-off-by: Bryan Donlan <bdonlan@fushizen.net>
---
 t/t0000-basic.sh                              |    4 +-
 t/t1020-subdirectory.sh                       |   22 +++++-----
 t/t3050-subprojects-fetch.sh                  |    2 +-
 t/t3404-rebase-interactive.sh                 |    3 +-
 t/t5500-fetch-pack.sh                         |    2 +-
 t/t5512-ls-remote.sh                          |    2 +-
 t/t5516-fetch-push.sh                         |    4 +-
 t/t5700-clone-reference.sh                    |    2 +-
 t/t5710-info-alternate.sh                     |    2 +-
 t/t7003-filter-branch.sh                      |    2 +-
 t/t7010-setup.sh                              |    2 +-
 t/t7300-clean.sh                              |    2 +-
 t/t7501-commit.sh                             |    8 ++--
 t/t7504-commit-msg-hook.sh                    |   23 ++++++-----
 t/t7505-prepare-commit-msg-hook.sh            |   17 +++++---
 t/t9100-git-svn-basic.sh                      |   54 ++++++++++++------------
 t/t9101-git-svn-props.sh                      |    6 +-
 t/t9102-git-svn-deep-rmdir.sh                 |    6 +-
 t/t9103-git-svn-tracked-directory-removed.sh  |   30 +++++++-------
 t/t9104-git-svn-follow-parent.sh              |   50 +++++++++++-----------
 t/t9105-git-svn-commit-diff.sh                |   12 +++---
 t/t9106-git-svn-commit-diff-clobber.sh        |   14 +++---
 t/t9106-git-svn-dcommit-clobber-series.sh     |    6 +-
 t/t9107-git-svn-migrate.sh                    |   48 +++++++++++-----------
 t/t9108-git-svn-glob.sh                       |    8 ++--
 t/t9110-git-svn-use-svm-props.sh              |    8 ++--
 t/t9111-git-svn-use-svnsync-props.sh          |    8 ++--
 t/t9112-git-svn-md5less-file.sh               |    4 +-
 t/t9113-git-svn-dcommit-new-file.sh           |    6 +-
 ...
[ message continues ]
[view original message]
From: Bryan Donlan
Subject: [PATCH v2 10/10] Rename the test trash directory to contain shell metacharacters.
Date: Wednesday, April 9, 2008 - 11:50 pm

In order to help prevent regressions in the future, rename the trash directory
for all tests to contain an assortment of shell metacharacters. This patch
also corrects two failures that were caused or exposed by this change.

Signed-off-by: Bryan Donlan <bdonlan@fushizen.net>
---
 t/.gitignore                         |    2 +-
 t/t6200-fmt-merge-msg.sh             |    6 +++---
 t/t9121-git-svn-fetch-renamed-dir.sh |   12 ++++++------
 t/test-lib.sh                        |    4 ++--
 4 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/t/.gitignore b/t/.gitignore
index fad67c0..fd07343 100644
--- a/t/.gitignore
+++ b/t/.gitignore
@@ -1 +1 @@
-trash
+/trash directory with characters like $ and " and ' in its name
diff --git a/t/t6200-fmt-merge-msg.sh b/t/t6200-fmt-merge-msg.sh
index 526d7d1..9c0b926 100755
--- a/t/t6200-fmt-merge-msg.sh
+++ b/t/t6200-fmt-merge-msg.sh
@@ -82,14 +82,14 @@ test_expect_success 'merge-msg test #1' '
 	git diff actual expected
 '
 
-cat >expected <<\EOF
-Merge branch 'left' of ../trash
+cat >expected <<EOF
+Merge branch 'left' of ../$test
 EOF
 
 test_expect_success 'merge-msg test #2' '
 
 	git checkout master &&
-	git fetch ../trash left &&
+	git fetch ../"$test" left &&
 
 	git fmt-merge-msg <.git/FETCH_HEAD >actual &&
 	git diff actual expected
diff --git a/t/t9121-git-svn-fetch-renamed-dir.sh b/t/t9121-git-svn-fetch-renamed-dir.sh
index 5143ed6..99230b0 100755
--- a/t/t9121-git-svn-fetch-renamed-dir.sh
+++ b/t/t9121-git-svn-fetch-renamed-dir.sh
@@ -7,14 +7,14 @@ test_description='git-svn can fetch renamed directories'
 
 . ./lib-git-svn.sh
 
-test_expect_success 'load repository with renamed directory' "
-	svnadmin load -q $rawsvnrepo < ../t9121/renamed-dir.dump
-	"
+test_expect_success 'load repository with renamed directory' '
+	svnadmin load -q "$rawsvnrepo" < ../t9121/renamed-dir.dump
+	'
 
-test_expect_success 'init and fetch repository' "
-	git svn init $svnrepo/newname &&
+test_expect_success 'init ...
[ message continues ]
[view original message]
From: Junio C Hamano
Subject: Re: [PATCH v2 01/10] git-rebase.sh: Fix --merge --abort failures when path contains whitespace
Date: Thursday, April 10, 2008 - 12:45 am

As you know that $work_dir and shell variables in general are available
inside test_expect_* framework without anything special, like the above

this part would become a lot easier to read if you just stayed inside the
original single quoted test script without introducing excessive
backslashes.

I am very inclined to replace t3407 with the following.

diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 37944c3..cadfb23 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -4,7 +4,13 @@ test_description='git rebase --abort tests'
 
 . ./test-lib.sh
 
+### Test that we handle strange characters properly
+work_dir="$(pwd)/test \" ' \$ \\ dir"
+
 test_expect_success setup '
+	mkdir -p "$work_dir" &&
+	cd "$work_dir" &&
+	git init &&
 	echo a > a &&
 	git add a &&
 	git commit -m a &&
@@ -28,32 +34,35 @@ testrebase() {
 	dotest=$2
 
 	test_expect_success "rebase$type --abort" '
+		cd "$work_dir" &&
 		# Clean up the state from the previous one
-		git reset --hard pre-rebase
-		test_must_fail git rebase'"$type"' master &&
-		test -d '$dotest' &&
+		git reset --hard pre-rebase &&
+		test_must_fail git rebase$type master &&
+		test -d "$dotest" &&
 		git rebase --abort &&
 		test $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) &&
-		test ! -d '$dotest'
+		test ! -d "$dotest"
 	'
 
 	test_expect_success "rebase$type --abort after --skip" '
+		cd "$work_dir" &&
 		# Clean up the state from the previous one
-		git reset --hard pre-rebase
-		test_must_fail git rebase'"$type"' master &&
-		test -d '$dotest' &&
+		git reset --hard pre-rebase &&
+		test_must_fail git rebase$type master &&
+		test -d "$dotest" &&
 		test_must_fail git rebase --skip &&
 		test $(git rev-parse HEAD) = $(git rev-parse master) &&
 		git-rebase --abort &&
 		test $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) &&
-		test ! -d '$dotest'
+		test ! -d "$dotest"
 	'
 
 	test_expect_success "rebase$type --abort after --continue" ...
[ message continues ]
[view original message]
From: Bryan Donlan
Subject: Re: [PATCH v2 01/10] git-rebase.sh: Fix --merge --abort failures when path contains whitespace
Date: Thursday, April 10, 2008 - 1:48 am

Oh, wow, what was I thinking? I'll fix that up... it must've been rather
late at night when I wrote that or something :)

Also, I saw your note about changing the git-svn tests to single-quoting
rather than escaping everywhere - but only after I hit send. I'll roll
that into the v3 (and probably wait longer than this time, so people
have time to finish reviewing...)

Thanks,

Bryan
--


[ message continues ]
[view original message]
From: Bryan Donlan
Subject: [PATCH v3 00/10] Fix git's test suite to pass when the path contains spaces
Date: Saturday, May 3, 2008 - 10:37 pm

This patch series fixes bugs in git and git's testsuite to allow all tests
to pass when the working directory contains whitespace and/or shell
metacharacters.

The first three patches in this series fix bugs in git itself that were
uncovered in the process of fixing the test suite. Each contains additional
tests and/or updates to existing tests to exercise the bug in question.

Patches 4 and 5 set up an abstraction for safe handling of editor variables
in tests when we have odd paths; and 6-7 fix bugs in the test libraries.
Patch 8 cleans up some instances of the unportable export FOO=bar construct
in the test suite; patch 9 fixes the remaining individual tests to be
space-sane, and finally 10 arranges for the test suite to always run with the
trash directory name containing spaces, to help prevent regressions in the
future.

Significant changes since v2:
  * Use only spaces in the trash dir names for Windows compatibility
  * Avoid ugliness like \"\$GIT_DIR\"

This series is also available by:
  git pull git://repo.or.cz/git/bdonlan.git wst-submit3

Sorry for the long delay in updating the series; I've been consumed with
schoolwork lately :)

Since it was mentioned providing conversion scripts was useful in review, here
is a hacky script I put together to help fixup the quotation:

http://fushizen.net/~bd/requote.pl

This script was applied on t/t*svn*.sh, then followed by a few manual fixes,
review, and git add -i to change only the chunks that I had made worse in the
previous patches. Note however that the conversion of "->' in places makes for
a large diff...

Bryan Donlan (10):
  git-rebase.sh: Fix --merge --abort failures when path contains
    whitespace
  config.c: Escape backslashes in section names properly
  git-send-email.perl: Handle shell metacharacters in $EDITOR properly
  test-lib.sh: Add a test_set_editor function to safely set $VISUAL
  Use test_set_editor in t9001-send-email.sh
  test-lib.sh: Fix some missing path quoting
  lib-git-svn.sh: Fix ...
[ message continues ]
[view original message]
From: Bryan Donlan
Subject: [PATCH v3 01/10] git-rebase.sh: Fix --merge --abort failures when path contains whitespace
Date: Saturday, May 3, 2008 - 10:37 pm

Also update t/t3407-rebase-abort.sh to exercise the bug

Signed-off-by: Bryan Donlan <bdonlan@fushizen.net>
---
 git-rebase.sh           |    2 +-
 t/t3407-rebase-abort.sh |   33 +++++++++++++++++++++------------
 2 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index 9b13b83..c43afe5 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -214,7 +214,7 @@ do
 		else
 			die "No rebase in progress?"
 		fi
-		git reset --hard $(cat $dotest/orig-head)
+		git reset --hard $(cat "$dotest/orig-head")
 		rm -r "$dotest"
 		exit
 		;;
diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 37944c3..1777ffe 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -4,7 +4,13 @@ test_description='git rebase --abort tests'
 
 . ./test-lib.sh
 
+### Test that we handle space characters properly
+work_dir="$(pwd)/test dir"
+
 test_expect_success setup '
+	mkdir -p "$work_dir" &&
+	cd "$work_dir" &&
+	git init &&
 	echo a > a &&
 	git add a &&
 	git commit -m a &&
@@ -28,32 +34,35 @@ testrebase() {
 	dotest=$2
 
 	test_expect_success "rebase$type --abort" '
+		cd "$work_dir" &&
 		# Clean up the state from the previous one
-		git reset --hard pre-rebase
-		test_must_fail git rebase'"$type"' master &&
-		test -d '$dotest' &&
+		git reset --hard pre-rebase &&
+		test_must_fail git rebase$type master &&
+		test -d "$dotest" &&
 		git rebase --abort &&
 		test $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) &&
-		test ! -d '$dotest'
+		test ! -d "$dotest"
 	'
 
 	test_expect_success "rebase$type --abort after --skip" '
+		cd "$work_dir" &&
 		# Clean up the state from the previous one
-		git reset --hard pre-rebase
-		test_must_fail git rebase'"$type"' master &&
-		test -d '$dotest' &&
+		git reset --hard pre-rebase &&
+		test_must_fail git rebase$type master &&
+		test -d "$dotest" &&
 		test_must_fail git rebase --skip &&
 		test $(git rev-parse HEAD) = $(git rev-parse master) &&
 ...
[ message continues ]
[view original message]
From: Bryan Donlan
Subject: [PATCH v3 02/10] config.c: Escape backslashes in section names properly
Date: Saturday, May 3, 2008 - 10:37 pm

If an element of the configuration key name other than the first or last
contains a backslash, it is not escaped on output, but is treated as an escape
sequence on input. Thus, the backslash is lost when re-loading the
configuration.

This patch corrects this by having backslashes escaped properly, and
introduces a new test for this bug.

Signed-off-by: Bryan Donlan <bdonlan@fushizen.net>
---
 config.c                |    2 +-
 t/t1303-wacky-config.sh |    6 ++++++
 2 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/config.c b/config.c
index b0ada51..f0ac456 100644
--- a/config.c
+++ b/config.c
@@ -680,7 +680,7 @@ static int store_write_section(int fd, const char* key)
 	if (dot) {
 		strbuf_addf(&sb, "[%.*s \"", (int)(dot - key), key);
 		for (i = dot - key + 1; i < store.baselen; i++) {
-			if (key[i] == '"')
+			if (key[i] == '"' || key[i] == '\\')
 				strbuf_addch(&sb, '\\');
 			strbuf_addch(&sb, key[i]);
 		}
diff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh
index 99985dc..f366b53 100755
--- a/t/t1303-wacky-config.sh
+++ b/t/t1303-wacky-config.sh
@@ -34,4 +34,10 @@ test_expect_success 'add key in different section' '
 	check section2.key bar
 '
 
+SECTION="test.q\"s\\sq'sp e.key"
+test_expect_success 'make sure git-config escapes section names properly' '
+	git config "$SECTION" bar &&
+	check "$SECTION" bar
+'
+
 test_done
-- 
1.5.5.1.128.g03a943

--


[ message continues ]
[view original message]
From: Bryan Donlan
Subject: [PATCH v3 03/10] git-send-email.perl: Handle shell metacharacters in $EDITOR properly
Date: Saturday, May 3, 2008 - 10:37 pm

This fixes the git-send-perl semantics for launching an editor when
$GIT_EDITOR (or friends) contains shell metacharacters to match
launch_editor() in builtin-tag.c. If we use the current approach
(sh -c '$0 $@' "$EDITOR" files ...), we see it fails when $EDITOR has
shell metacharacters:

$ sh -x -c '$0 $@' "$VISUAL" "foo"
+ "$FAKE_EDITOR" foo
"$FAKE_EDITOR": 1: "$FAKE_EDITOR": not found

Whereas builtin-tag.c will invoke sh -c "$EDITOR \"$@\"".

Thus, this patch changes git-send-email.perl to use the same method as the
C utilities, and additionally updates t/t9001-send-email.sh to test for this bug.

Signed-off-by: Bryan Donlan <bdonlan@fushizen.net>
---
 git-send-email.perl   |    2 +-
 t/t9001-send-email.sh |    8 ++++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 9e568bf..b502396 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -512,7 +512,7 @@ EOT
 	close(C);
 
 	my $editor = $ENV{GIT_EDITOR} || Git::config(@repo, "core.editor") || $ENV{VISUAL} || $ENV{EDITOR} || "vi";
-	system('sh', '-c', '$0 $@', $editor, $compose_filename);
+	system('sh', '-c', $editor.' "$@"', $editor, $compose_filename);
 
 	open(C2,">",$compose_filename . ".final")
 		or die "Failed to open $compose_filename.final : " . $!;
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index c0973b4..030f66c 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -139,15 +139,19 @@ test_expect_success 'Valid In-Reply-To when prompting' '
 
 test_expect_success 'setup fake editor' '
 	(echo "#!/bin/sh" &&
-	 echo "echo fake edit >>\$1"
+	 echo "echo fake edit >>\"\$1\""
 	) >fake-editor &&
 	chmod +x fake-editor
 '
 
+FAKE_EDITOR="$(pwd)/fake-editor"
+export FAKE_EDITOR
+GIT_EDITOR='"$FAKE_EDITOR"'
+export GIT_EDITOR
+
 test_expect_success '--compose works' '
 	clean_fake_sendmail &&
 	echo y | \
-		GIT_EDITOR=$(pwd)/fake-editor \
 		GIT_SEND_EMAIL_NOTTY=1 \
 		git send-email \
 ...
[ message continues ]
[view original message]
From: Bryan Donlan
Subject: [PATCH v3 04/10] test-lib.sh: Add a test_set_editor function to safely set $VISUAL
Date: Saturday, May 3, 2008 - 10:37 pm

In particular, this function correctly handles cases where the pwd contains
spaces, quotes, and other troublesome metacharacters.

Signed-off-by: Bryan Donlan <bdonlan@fushizen.net>
---
 t/test-lib.sh |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 7c2a8ba..d7ad13b 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -160,6 +160,22 @@ die () {
 
 trap 'die' exit
 
+# The semantics of the editor variables are that of invoking
+# sh -c "$EDITOR \"$@\"" files ...
+#
+# If our trash directory contains shell metacharacters, they will be
+# interpreted if we just set $EDITOR directly, so do a little dance with
+# environment variables to work around this.
+#
+# In particular, quoting isn't enough, as the path may contain the same quote
+# that we're using. 
+test_set_editor () {
+	FAKE_EDITOR="$1"
+	export FAKE_EDITOR
+	VISUAL='"$FAKE_EDITOR"'
+	export VISUAL
+}
+
 test_tick () {
 	if test -z "${test_tick+set}"
 	then
-- 
1.5.5.1.128.g03a943

--


[ message continues ]
[view original message]
From: Bryan Donlan
Subject: [PATCH v3 05/10] Use test_set_editor in t9001-send-email.sh
Date: Saturday, May 3, 2008 - 10:37 pm

Signed-off-by: Bryan Donlan <bdonlan@fushizen.net>
---
 t/t9001-send-email.sh |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 030f66c..0a65785 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -144,10 +144,7 @@ test_expect_success 'setup fake editor' '
 	chmod +x fake-editor
 '
 
-FAKE_EDITOR="$(pwd)/fake-editor"
-export FAKE_EDITOR
-GIT_EDITOR='"$FAKE_EDITOR"'
-export GIT_EDITOR
+test_set_editor "$(pwd)/fake-editor"
 
 test_expect_success '--compose works' '
 	clean_fake_sendmail &&
-- 
1.5.5.1.128.g03a943

--


[ message continues ]
[view original message]
From: Bryan Donlan
Subject: [PATCH v3 06/10] test-lib.sh: Fix some missing path quoting
Date: Saturday, May 3, 2008 - 10:37 pm

Signed-off-by: Bryan Donlan <bdonlan@fushizen.net>
---
 t/test-lib.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index d7ad13b..04e098b 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -345,7 +345,7 @@ test_create_repo () {
 	repo="$1"
 	mkdir "$repo"
 	cd "$repo" || error "Cannot setup test environment"
-	"$GIT_EXEC_PATH/git" init --template=$GIT_EXEC_PATH/templates/blt/ >/dev/null 2>&1 ||
+	"$GIT_EXEC_PATH/git" init "--template=$GIT_EXEC_PATH/templates/blt/" >/dev/null 2>&1 ||
 	error "cannot run git init -- have you built things yet?"
 	mv .git/hooks .git/hooks-disabled
 	cd "$owd"
-- 
1.5.5.1.128.g03a943

--


[ message continues ]
[view original message]
From: Bryan Donlan
Subject: [PATCH v3 07/10] lib-git-svn.sh: Fix quoting issues with paths containing shell metacharacters
Date: Saturday, May 3, 2008 - 10:37 pm

Signed-off-by: Bryan Donlan <bdonlan@fushizen.net>
---
 t/lib-git-svn.sh |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh
index 9decd2e..445df78 100644
--- a/t/lib-git-svn.sh
+++ b/t/lib-git-svn.sh
@@ -20,12 +20,13 @@ then
 fi
 
 svnrepo=$PWD/svnrepo
+export svnrepo
 
 perl -w -e "
 use SVN::Core;
 use SVN::Repos;
 \$SVN::Core::VERSION gt '1.1.0' or exit(42);
-system(qw/svnadmin create --fs-type fsfs/, '$svnrepo') == 0 or exit(41);
+system(qw/svnadmin create --fs-type fsfs/, \$ENV{svnrepo}) == 0 or exit(41);
 " >&3 2>&4
 x=$?
 if test $x -ne 0
-- 
1.5.5.1.128.g03a943

--


[ message continues ]
[view original message]
From: Bryan Donlan
Subject: [PATCH v3 08/10] Don't use the export NAME=value form in the test scripts.
Date: Saturday, May 3, 2008 - 10:37 pm

This form is not portable across all shells, so replace instances of:

  export FOO=bar

with:

  FOO=bar
  export FOO

Signed-off-by: Bryan Donlan <bdonlan@fushizen.net>
---
 t/lib-httpd.sh                         |    3 +-
 t/t1500-rev-parse.sh                   |    9 ++++---
 t/t1501-worktree.sh                    |   34 ++++++++++++++++---------------
 t/t3400-rebase.sh                      |    3 +-
 t/t3500-cherry.sh                      |    3 +-
 t/t5500-fetch-pack.sh                  |    2 +-
 t/t6000lib.sh                          |    9 +++++--
 t/t6010-merge-base.sh                  |    9 ++++---
 t/t7004-tag.sh                         |    3 +-
 t/t9500-gitweb-standalone-no-errors.sh |   16 ++++++++------
 10 files changed, 52 insertions(+), 39 deletions(-)

diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 7f206c5..a5c4436 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -61,7 +61,8 @@ prepare_httpd() {
 			-new -x509 -nodes \
 			-out $HTTPD_ROOT_PATH/httpd.pem \
 			-keyout $HTTPD_ROOT_PATH/httpd.pem
-		export GIT_SSL_NO_VERIFY=t
+		GIT_SSL_NO_VERIFY=t
+		export GIT_SSL_NO_VERIFY
 		HTTPD_PARA="$HTTPD_PARA -DSSL"
 	else
 		HTTPD_URL=http://127.0.0.1:$LIB_HTTPD_PORT
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 38a2bf0..85da4ca 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -51,8 +51,9 @@ test_rev_parse 'core.bare undefined' false false true
 
 mkdir work || exit 1
 cd work || exit 1
-export GIT_DIR=../.git
-export GIT_CONFIG="$(pwd)"/../.git/config
+GIT_DIR=../.git
+GIT_CONFIG="$(pwd)"/../.git/config
+export GIT_DIR GIT_CONFIG
 
 git config core.bare false
 test_rev_parse 'GIT_DIR=../.git, core.bare = false' false false true ''
@@ -64,8 +65,8 @@ git config --unset core.bare
 test_rev_parse 'GIT_DIR=../.git, core.bare undefined' false false true ''
 
 mv ../.git ../repo.git || exit 1
-export GIT_DIR=../repo.git
-export ...
[ message continues ]
[view original message]
From: Johannes Sixt
Subject: Re: [PATCH v3 00/10] Fix git's test suite to pass when the path contains spaces
Date: Monday, May 5, 2008 - 12:03 am

The last patch breaks t9400-git-cvsserver-server.sh on my system:

* expecting success: cat request-anonymous | git-cvsserver --strict-paths
pserver $SERVERDIR >log 2>&1 &&
   sed -ne \$p log | grep "^I LOVE YOU$"
* FAIL 9: req_Root (strict paths)
        cat request-anonymous | git-cvsserver --strict-paths pserver
$SERVERDIR >log 2>&1 &&
           sed -ne \$p log | grep "^I LOVE YOU$"

D-Quoting $SERVERDIR helps. ;)

Please post an incremental diff when you resend it. Feel free to add:

Tested-by: Johannes Sixt <johannes.sixt@telecom.at>

-- Hannes

--


[ message continues ]
[view original message]
From: Bryan Donlan
Subject: Re: [PATCH v3 00/10] Fix git's test suite to pass when the path contains spaces
Date: Monday, May 5, 2008 - 12:59 am

When you say post an incremental diff, do you mean send just an
additional patch to squash in, or re-send the series with an interdiff

--


[ message continues ]
[view original message]
From: Johannes Sixt
Subject: Re: [PATCH v3 00/10] Fix git's test suite to pass when the path contains spaces
Date: Monday, May 5, 2008 - 1:19 am

The latter. Since you'll most likely use rebase -i, this will amount to
just 'git diff wst-submit3@{yesterday}..' or so.

(This assumes that the rebased series originates from the same commit; if
it doesn't because you have meanwhile updated your origin/master, you
should first rebase without any changes, then rebase -i again, and make
changes during this second rebase; then the interdiff is 'git diff

You can add it to every patch below the SOB line.

-- Hannes

--


[ message continues ]
Previous thread: Re: ANNOUNCE: Git Forum by DigitalPig on Tuesday, April 8, 2008 - 5:49 pm. (2 messages)

Next thread: [PATCH] Fix documentation syntax of optional arguments in short options. by Carlos Rica on Tuesday, April 8, 2008 - 9:24 pm. (2 messages)