Skip to contentSkip to conversation

t1501: avoid bashisms

PATCH In review Nguyễn Thái Ngọc Duy 3 participants 9 messages Jun 15, 2016 Kbuild

lore
Share & tools
lore

Nguyễn Thái Ngọc Duy

author
#0 lore raw

PATCH 1/2 t1501: avoid bashisms

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>
---
 t/t1501-worktree.sh |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
t/t1501-worktree.sh +4 -1
index 2c8f01f..488160e 100755
@@ -322,7 +322,10 @@ test_expect_success 'git grep' '
 test_expect_success 'git commit' '
 	(
 		cd repo.git &&
-		GIT_DIR=. GIT_WORK_TREE=work git commit -a -m done
+		GIT_DIR=. &&
+		GIT_WORK_TREE=work &&
+		export GIT_DIR GIT_WORK_TREE &&
+		git commit -a -m done
 	)
 '
Signature · 1 line
1.7.3.4.878.g439c7
author
#1 lore raw

PATCH 2/2 setup_work_tree: adjust relative $GIT_WORK_TREE after moving cwd

When setup_work_tree() is called, it moves cwd to $GIT_WORK_TREE and makes internal copy of $GIT_WORK_TREE absolute. The environt variable, if set by user, remains unchanged. If the variable is relative, it is no longer correct because its base dir has changed.

Instead of making $GIT_WORK_TREE absolute too, we just say "." and let subsequence git processes handle it.

Reported-by Michel Briand <[email protected]>
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>
---
 .gitignore          |    1 +
 Makefile            |    1 +
 setup.c             |    8 ++++++++
 t/t1501-worktree.sh |   11 +++++++++++
 test-subprocess.c   |   18 ++++++++++++++++++
 5 files changed, 39 insertions(+), 0 deletions(-)
 create mode 100644 test-subprocess.c
.gitignore +1 -0
index 87b833c..3dd6ef7 100644
@@ -177,6 +177,7 @@
 /test-sha1
 /test-sigchain
 /test-string-pool
+/test-subprocess
 /test-svn-fe
 /test-treap
 /common-cmds.h
Makefile +1 -0
index 57d9c65..bdf86a3 100644
@@ -431,6 +431,7 @@ TEST_PROGRAMS_NEED_X += test-run-command
 TEST_PROGRAMS_NEED_X += test-sha1
 TEST_PROGRAMS_NEED_X += test-sigchain
 TEST_PROGRAMS_NEED_X += test-string-pool
+TEST_PROGRAMS_NEED_X += test-subprocess
 TEST_PROGRAMS_NEED_X += test-svn-fe
 TEST_PROGRAMS_NEED_X += test-treap
 TEST_PROGRAMS_NEED_X += test-index-version
setup.c +8 -0
index 91887a4..3833569 100644
@@ -239,6 +239,14 @@ void setup_work_tree(void)
 		git_dir = make_absolute_path(git_dir);
 	if (!work_tree || chdir(work_tree))
 		die("This operation must be run in a work tree");
+
+	/*
+	 * Make sure subsequent git processes find correct worktree
+	 * if $GIT_WORK_TREE is set relative
+	 */
+	if (getenv(GIT_WORK_TREE_ENVIRONMENT))
+		setenv(GIT_WORK_TREE_ENVIRONMENT, ".", 1);
+
 	set_git_dir(make_relative_path(git_dir, work_tree));
 	initialized = 1;
 }
t/t1501-worktree.sh +11 -0
index 488160e..16c953b 100755
@@ -343,4 +343,15 @@ test_expect_success 'make_relative_path handles double slashes in GIT_DIR' '
 	git --git-dir="$(pwd)//repo.git" --work-tree="$(pwd)" add dummy_file
 '
 
+test_expect_success 'relative $GIT_WORK_TREE and git subprocesses' '
+	(
+	GIT_DIR=repo.git
+	GIT_WORK_TREE=repo.git/work
+	export GIT_DIR GIT_WORK_TREE
+	test-subprocess rev-parse --show-toplevel >actual &&
+	echo "`pwd`/repo.git/work" >expected &&
+	test_cmp expected actual
+	)
+'
+
 test_done
test-subprocess.c new file +18 -0
new file mode 100644
index 0000000..55ad719
@@ -0,0 +1,18 @@
+#include "cache.h"
+#include "run-command.h"
+
+int main(int argc, char **argv)
+{
+	const char *prefix;
+	struct child_process cp;
+	int nogit;
+
+	prefix = setup_git_directory_gently(&nogit);
+	if (nogit)
+		die("No git repo found");
+	setup_work_tree();
+	memset(&cp, 0, sizeof(cp));
+	cp.git_cmd = 1;
+	cp.argv = (const char **)argv+1;
+	return run_command(&cp);
+}
Signature · 1 line
1.7.3.4.878.g439c7
author
#2 lore raw

PATCH 1/1 setup_work_tree: adjust relative $GIT_WORK_TREE after moving cwd

When setup_work_tree() is called, it moves cwd to $GIT_WORK_TREE and makes internal copy of $GIT_WORK_TREE absolute. The environt variable, if set by user, remains unchanged. If the variable is relative, it is no longer correct because its base dir has changed.

Instead of making $GIT_WORK_TREE absolute too, we just say "." and let subsequent git processes handle it.

Reported-by Michel Briand <[email protected]>
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
This one is better, on top of master because the next one is on top of nd/setup.

And I forgot to tell why I did not put the fault in real life [1] into tests: I don't like git-merge spawning another process just for resetting worktree. Sooner or later it should be replaced to use unpack_trees() directly. When that happens, the test would become invalid.

[1] http://thread.gmane.org/gmane.comp.version-control.git/164066/focus=164171

 .gitignore          |    1 +
 Makefile            |    1 +
 setup.c             |    8 ++++++++
 t/t1501-worktree.sh |   11 +++++++++++
 test-subprocess.c   |   21 +++++++++++++++++++++
 5 files changed, 42 insertions(+), 0 deletions(-)
 create mode 100644 test-subprocess.c
.gitignore +1 -0
index 87b833c..3dd6ef7 100644
@@ -177,6 +177,7 @@
 /test-sha1
 /test-sigchain
 /test-string-pool
+/test-subprocess
 /test-svn-fe
 /test-treap
 /common-cmds.h
Makefile +1 -0
index 57d9c65..bdf86a3 100644
@@ -431,6 +431,7 @@ TEST_PROGRAMS_NEED_X += test-run-command
 TEST_PROGRAMS_NEED_X += test-sha1
 TEST_PROGRAMS_NEED_X += test-sigchain
 TEST_PROGRAMS_NEED_X += test-string-pool
+TEST_PROGRAMS_NEED_X += test-subprocess
 TEST_PROGRAMS_NEED_X += test-svn-fe
 TEST_PROGRAMS_NEED_X += test-treap
 TEST_PROGRAMS_NEED_X += test-index-version
setup.c +8 -0
index 91887a4..3833569 100644
@@ -239,6 +239,14 @@ void setup_work_tree(void)
 		git_dir = make_absolute_path(git_dir);
 	if (!work_tree || chdir(work_tree))
 		die("This operation must be run in a work tree");
+
+	/*
+	 * Make sure subsequent git processes find correct worktree
+	 * if $GIT_WORK_TREE is set relative
+	 */
+	if (getenv(GIT_WORK_TREE_ENVIRONMENT))
+		setenv(GIT_WORK_TREE_ENVIRONMENT, ".", 1);
+
 	set_git_dir(make_relative_path(git_dir, work_tree));
 	initialized = 1;
 }
t/t1501-worktree.sh +11 -0
index 2c8f01f..1f3b50d 100755
@@ -340,4 +340,15 @@ test_expect_success 'make_relative_path handles double slashes in GIT_DIR' '
 	git --git-dir="$(pwd)//repo.git" --work-tree="$(pwd)" add dummy_file
 '
 
+test_expect_success 'relative $GIT_WORK_TREE and git subprocesses' '
+	(
+	GIT_DIR=repo.git &&
+	GIT_WORK_TREE=repo.git/work &&
+	export GIT_DIR GIT_WORK_TREE &&
+	test-subprocess --setup-work-tree rev-parse --show-toplevel >actual &&
+	echo "`pwd`/repo.git/work" >expected &&
+	test_cmp expected actual
+	)
+'
+
 test_done
test-subprocess.c new file +21 -0
new file mode 100644
index 0000000..667d3e5
@@ -0,0 +1,21 @@
+#include "cache.h"
+#include "run-command.h"
+
+int main(int argc, char **argv)
+{
+	const char *prefix;
+	struct child_process cp;
+	int nogit = 0;
+
+	prefix = setup_git_directory_gently(&nogit);
+	if (nogit)
+		die("No git repo found");
+	if (!strcmp(argv[1], "--setup-work-tree")) {
+		setup_work_tree();
+		argv++;
+	}
+	memset(&cp, 0, sizeof(cp));
+	cp.git_cmd = 1;
+	cp.argv = (const char **)argv+1;
+	return run_command(&cp);
+}
Signature · 1 line
1.7.3.4.878.g439c7
author
#3 lore raw

PATCH 1/1 setup_explicit_git_dir: adjust relative $GIT_WORK_TREE after moving cwd

setup_explicit_git_dir() can move cwd. If $GIT_WORK_TREE is relative to original cwd, then the subsequent git processes will take wrong worktree.

Instead of making $GIT_WORK_TREE absolute too, we just say "." and let subsequent git processes handle it.

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
This one is on top of nd/setup. If nd/setup does not make it in the next release, I can still make similar patch for master, but I don't think this happens in real life, therefore not really urgent to fix.

There are only few cases where git spawns more git processes. For those that do that, setup_work_tree() is likely called by git.c already, which is what the first patch is for.

 setup.c             |    7 +++++++
 t/t1501-worktree.sh |   12 ++++++++++++
 2 files changed, 19 insertions(+), 0 deletions(-)
setup.c +7 -0
index 3d73269..10b8f16 100644
@@ -392,6 +392,13 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,
 		set_git_dir(make_absolute_path(gitdirenv));
 		if (chdir(worktree))
 			die_errno("Could not chdir to '%s'", worktree);
+		/*
+		 * Make sure subsequent git processes find correct worktree
+		 * if $GIT_WORK_TREE is set relative
+		 */
+		if (work_tree_env)
+			setenv(GIT_WORK_TREE_ENVIRONMENT, ".", 1);
+
 		cwd[len++] = '/';
 		cwd[len] = '\0';
 		free(gitfile);
t/t1501-worktree.sh +12 -0
index 1f3b50d..fa35c3e 100755
@@ -351,4 +351,16 @@ test_expect_success 'relative $GIT_WORK_TREE and git subprocesses' '
 	)
 '
 
+test_expect_success 'relative $GIT_WORK_TREE and git subprocesses (2)' '
+	(
+	cd repo.git/work/sub &&
+	GIT_DIR=../.. &&
+	GIT_WORK_TREE=.. &&
+	export GIT_DIR GIT_WORK_TREE &&
+	test-subprocess rev-parse --show-toplevel >actual &&
+	echo "$TRASH_DIRECTORY/repo.git/work" >expected &&
+	test_cmp expected actual
+	)
+'
+
 test_done
Signature · 1 line
1.7.3.4.878.g439c7
author
#4 lore raw

PATCH 1/1 setup_work_tree: adjust relative $GIT_WORK_TREE after moving cwd

In reply to Nguyễn Thái Ngọc Duy

When setup_work_tree() is called, it moves cwd to $GIT_WORK_TREE and makes internal copy of $GIT_WORK_TREE absolute. The environt variable, if set by user, remains unchanged. If the variable is relative, it is no longer correct because its base dir has changed.

Instead of making $GIT_WORK_TREE absolute too, we just say "." and let subsequent git processes handle it.

Reported-by Michel Briand <[email protected]>
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
Third round. Remove subshell and pwd in t1501.31

 .gitignore          |    1 +
 Makefile            |    1 +
 setup.c             |    8 ++++++++
 t/t1501-worktree.sh |    7 +++++++
 test-subprocess.c   |   21 +++++++++++++++++++++
 5 files changed, 38 insertions(+), 0 deletions(-)
 create mode 100644 test-subprocess.c
.gitignore +1 -0
index 87b833c..3dd6ef7 100644
@@ -177,6 +177,7 @@
 /test-sha1
 /test-sigchain
 /test-string-pool
+/test-subprocess
 /test-svn-fe
 /test-treap
 /common-cmds.h
Makefile +1 -0
index 57d9c65..bdf86a3 100644
@@ -431,6 +431,7 @@ TEST_PROGRAMS_NEED_X += test-run-command
 TEST_PROGRAMS_NEED_X += test-sha1
 TEST_PROGRAMS_NEED_X += test-sigchain
 TEST_PROGRAMS_NEED_X += test-string-pool
+TEST_PROGRAMS_NEED_X += test-subprocess
 TEST_PROGRAMS_NEED_X += test-svn-fe
 TEST_PROGRAMS_NEED_X += test-treap
 TEST_PROGRAMS_NEED_X += test-index-version
setup.c +8 -0
index 91887a4..3833569 100644
@@ -239,6 +239,14 @@ void setup_work_tree(void)
 		git_dir = make_absolute_path(git_dir);
 	if (!work_tree || chdir(work_tree))
 		die("This operation must be run in a work tree");
+
+	/*
+	 * Make sure subsequent git processes find correct worktree
+	 * if $GIT_WORK_TREE is set relative
+	 */
+	if (getenv(GIT_WORK_TREE_ENVIRONMENT))
+		setenv(GIT_WORK_TREE_ENVIRONMENT, ".", 1);
+
 	set_git_dir(make_relative_path(git_dir, work_tree));
 	initialized = 1;
 }
t/t1501-worktree.sh +7 -0
index 2c8f01f..f072a8e 100755
@@ -340,4 +340,11 @@ test_expect_success 'make_relative_path handles double slashes in GIT_DIR' '
 	git --git-dir="$(pwd)//repo.git" --work-tree="$(pwd)" add dummy_file
 '
 
+test_expect_success 'relative $GIT_WORK_TREE and git subprocesses' '
+	GIT_DIR=repo.git GIT_WORK_TREE=repo.git/work \
+	test-subprocess --setup-work-tree rev-parse --show-toplevel >actual &&
+	echo "$TRASH_DIRECTORY/repo.git/work" >expected &&
+	test_cmp expected actual
+'
+
 test_done
test-subprocess.c new file +21 -0
new file mode 100644
index 0000000..667d3e5
@@ -0,0 +1,21 @@
+#include "cache.h"
+#include "run-command.h"
+
+int main(int argc, char **argv)
+{
+	const char *prefix;
+	struct child_process cp;
+	int nogit = 0;
+
+	prefix = setup_git_directory_gently(&nogit);
+	if (nogit)
+		die("No git repo found");
+	if (!strcmp(argv[1], "--setup-work-tree")) {
+		setup_work_tree();
+		argv++;
+	}
+	memset(&cp, 0, sizeof(cp));
+	cp.git_cmd = 1;
+	cp.argv = (const char **)argv+1;
+	return run_command(&cp);
+}
Signature · 1 line
1.7.3.4.878.g439c7
#5 lore raw
In reply to Nguyễn Thái Ngọc Duy

t/t1501-worktree.sh test_expect_success 'git grep' ' · line 325

Quoted 5 lines · Nguyễn Thái Ngọc Duy
t/t1501-worktree.sh +0 -1
@@ -322,7 +322,10 @@ test_expect_success 'git grep' '
 test_expect_success 'git commit' '
 	(
 		cd repo.git &&
-		GIT_DIR=. GIT_WORK_TREE=work git commit -a -m done

In which way is that not portable?

Andreas.

Signature · 3 lines
Andreas Schwab, [email protected]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
author
#6 lore raw

2010/12/25 Andreas Schwab <[email protected]>:

Quoted 9 lines
Quoted 5 lines · Nguyễn Thái Ngọc Duy
(quoted diff) +0 -0
@@ -322,7 +322,10 @@ test_expect_success 'git grep' '

 test_expect_success 'git commit' '
      (
              cd repo.git &&
-             GIT_DIR=. GIT_WORK_TREE=work git commit -a -m done

In which way is that not portable?

I admit that I rarely leave bash, so I'll quote Johannes answer [1]

-- 8< --
Sure, it is (bashisms). This:

GIT_DIR="$TRASH_DIRECTORY/2/.git" test_repo 2/sub

does not work the same way in all shells when test_repo is a shell function. You have to export GIT_DIR explicitly before the function call. (But since in this case, test_repo invokes its own subshell anyway, you better do it in the function.)
-- 8< --

[1] http://article.gmane.org/gmane.comp.version-control.git/162207

Signature · 1 line
Duy
author
#7 lore raw
Quoted 13 lines · Nguyen Thai Ngoc Duy

2010/12/25 Andreas Schwab <[email protected]>:

Quoted 9 lines
Quoted 5 lines · Nguyễn Thái Ngọc Duy
(quoted diff) +0 -0
@@ -322,7 +322,10 @@ test_expect_success 'git grep' '

 test_expect_success 'git commit' '
      (
              cd repo.git &&
-             GIT_DIR=. GIT_WORK_TREE=work git commit -a -m done

In which way is that not portable?

I admit that I rarely leave bash, so I'll quote Johannes answer [1]

Quoted 7 lines

-- 8< --
Sure, it is (bashisms). This:

  GIT_DIR="$TRASH_DIRECTORY/2/.git" test_repo 2/sub

does not work the same way in all shells when test_repo is a shell function. You have to export GIT_DIR explicitly before the function call.

Hmm.. I misread it. That's only for shell _functions_. [1] says variable assignments for simple commands are actually exported.

Junio, please don't pick up this patch.

[1] http://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html

Signature · 1 line
Duy
#8 lore raw
Quoted 1 line · Nguyen Thai Ngoc Duy

Junio, please don't pick up this patch.

Won't. And [2/2] needs to be updated with something like this?

 t/t1501-worktree.sh |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)
t/t1501-worktree.sh +2 -6
index 1429a65..8cab065 100755
@@ -341,14 +341,10 @@ test_expect_success 'make_relative_path handles double slashes in GIT_DIR' '
 '
 
 test_expect_success 'relative $GIT_WORK_TREE and git subprocesses' '
-	(
-	GIT_DIR=repo.git
-	GIT_WORK_TREE=repo.git/work
-	export GIT_DIR GIT_WORK_TREE
+	GIT_DIR=repo.git GIT_WORK_TREE=repo.git/work \
 	test-subprocess rev-parse --show-toplevel >actual &&
-	echo "`pwd`/repo.git/work" >expected &&
+	echo "$(pwd)/repo.git/work" >expected &&
 	test_cmp expected actual
-	)
 '
 
 test_done