Add missing inside_work_tree setting in setup_git_directory_gently

PATCH In review Nguyễn Thái Ngọc Duy 3 participants 11 messages Jun 15, 2016 1 message not in the archive

Share & tools

Nguyễn Thái Ngọc Duy

author

2016-06-15 22:43 UTC #0 lore raw

PATCH 1/1 Add missing inside_work_tree setting in setup_git_directory_gently

Without this, work_tree handling code in setup_git_directory will be activated. If you stay in root work tree (no prefix), it does not harm. It does if you work from a subdirectory though.

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

---
Turns out my patch on NEED_WORK_TREE is fixing a wrong place.

 setup.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

setup.c +1 -0

index 145eca5..6f8f769 100644
@@ -240,6 +240,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
 			if (chdir(work_tree_env) < 0)
 				die ("Could not chdir to %s", work_tree_env);
 			strcat(buffer, "/");
+			inside_work_tree = 1;
 			return retval;
 		}
 		if (nongit_ok) {

Signature · 1 line

1.5.3.rc4.3.gab089

Nguyễn Thái Ngọc Duy

author

2016-06-15 22:43 UTC #1 lore raw

PATCH 1/1 Add missing inside_work_tree setting in setup_git_directory_gently

When both GIT_DIR and GIT_WORK_TREE are set, and setup_git_directory_gently() changes the current working directory accordingly, it should also set inside_work_tree = 1.

Without this, work_tree handling code in setup_git_directory() will be activated. If you stay in root work tree (no prefix), it does not harm. It does if you work from a subdirectory though.

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

---
Updated commit message accordingly

 setup.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

setup.c +1 -0

index 145eca5..6f8f769 100644
@@ -240,6 +240,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
 			if (chdir(work_tree_env) < 0)
 				die ("Could not chdir to %s", work_tree_env);
 			strcat(buffer, "/");
+			inside_work_tree = 1;
 			return retval;
 		}
 		if (nongit_ok) {

Signature · 1 line

1.5.3.rc4.3.gab089

Junio C Hamano

2016-06-15 22:43 UTC #2 lore raw

Quoted 7 lines · Nguyễn Thái Ngọc Duy

When both GIT_DIR and GIT_WORK_TREE are set, and setup_git_directory_gently() changes the current working directory accordingly, it should also set inside_work_tree = 1.
Without this, work_tree handling code in setup_git_directory() will be activated. If you stay in root work tree (no prefix), it does not harm. It does if you work from a subdirectory though.

Please add automated test script for this, thanks.

Nguyen Thai Ngoc Duy

author

2016-06-15 22:43 UTC #3 lore raw

Quoted 6 lines · Junio C Hamano

Quoted 4 lines · Nguyễn Thái Ngọc Duy
When both GIT_DIR and GIT_WORK_TREE are set, and setup_git_directory_gently() changes the current working directory accordingly, it should also set inside_work_tree = 1.

Quoted 6 lines

Quoted 3 lines
Without this, work_tree handling code in setup_git_directory() will be activated. If you stay in root work tree (no prefix), it does not harm. It does if you work from a subdirectory though.
Please add automated test script for this, thanks.

Thank you for reminding. I tried to put a test in t1501-worktree.sh and found out core.worktree can override inside_work_tree previously set by setup_git_directory_gently(), activating the worktree code in setup_git_directory() again.

This made me think setup_git_directory_gently() should use get_git_work_tree() instead. But then git_work_tree_cfg may not be initialized when get_git_work_tree() is called (starting from setup_git_directory(), git_work_tree_cfg is initialized in check_repository_format_version(), which is called _after_ setup_git_directory_gently()).

The interaction between these variables and functions is really beyond my knowledge. Johannes, can you have a look at this? In theory the following test should pass:

t/t1501-worktree.sh +5 -0

index 7ee3820..bdb7720 100755
@@ -103,6 +103,11 @@ test_expect_success 'repo finds its work tree from work tree, too' '
 	 test sub/dir/tracked = "$(git ls-files)")
 '
 
+test_expect_success 'Try a command from subdir in worktree' '
+	(cd repo.git/work/sub &&
+	GIT_DIR=../.. GIT_WORK_TREE=.. git blame dir/tracked)
+'
+
 test_expect_success '_gently() groks relative GIT_DIR & GIT_WORK_TREE' '
 	cd repo.git/work/sub/dir &&
 	GIT_DIR=../../.. GIT_WORK_TREE=../.. GIT_PAGER= \

Signature · 1 line

Duy

Junio C Hamano

2016-06-15 22:43 UTC #4 lore raw

Quoted 26 lines · Nguyen Thai Ngoc Duy

Quoted 2 lines · Junio C Hamano
Please add automated test script for this, thanks.
Thank you for reminding. I tried to put a test in t1501-worktree.sh and found out core.worktree can override inside_work_tree previously set by setup_git_directory_gently(), activating the worktree code in setup_git_directory() again.
This made me think setup_git_directory_gently() should use get_git_work_tree() instead. But then git_work_tree_cfg may not be initialized when get_git_work_tree() is called (starting from setup_git_directory(), git_work_tree_cfg is initialized in check_repository_format_version(), which is called _after_ setup_git_directory_gently()).
The interaction between these variables and functions is really beyond my knowledge. Johannes, can you have a look at this? In theory the following test should pass:
t/t1501-worktree.sh +0 -0
index 7ee3820..bdb7720 100755
@@ -103,6 +103,11 @@ test_expect_success 'repo finds its work tree from work tree, too' '
 	 test sub/dir/tracked = "$(git ls-files)")

t/t1501-worktree.sh test_expect_success 'repo finds its work tree from work tree, too' ' · line 113

Quoted 10 lines

t/t1501-worktree.sh +5 -0

 '
 
+test_expect_success 'Try a command from subdir in worktree' '
+	(cd repo.git/work/sub &&
+	GIT_DIR=../.. GIT_WORK_TREE=.. git blame dir/tracked)
+'
+
 test_expect_success '_gently() groks relative GIT_DIR & GIT_WORK_TREE' '
 	cd repo.git/work/sub/dir &&
 	GIT_DIR=../../.. GIT_WORK_TREE=../.. GIT_PAGER= \

I am wondering what happened to this thread...

Johannes Schindelin

2016-06-15 22:43 UTC #5 lore raw

Hi,

Quoted 30 lines · Junio C Hamano

Quoted 28 lines · Nguyen Thai Ngoc Duy
Quoted 2 lines · Junio C Hamano
Please add automated test script for this, thanks.
Thank you for reminding. I tried to put a test in t1501-worktree.sh and found out core.worktree can override inside_work_tree previously set by setup_git_directory_gently(), activating the worktree code in setup_git_directory() again.
This made me think setup_git_directory_gently() should use get_git_work_tree() instead. But then git_work_tree_cfg may not be initialized when get_git_work_tree() is called (starting from setup_git_directory(), git_work_tree_cfg is initialized in check_repository_format_version(), which is called _after_ setup_git_directory_gently()).
The interaction between these variables and functions is really beyond my knowledge. Johannes, can you have a look at this? In theory the following test should pass:
t/t1501-worktree.sh +0 -0
index 7ee3820..bdb7720 100755
@@ -103,6 +103,11 @@ test_expect_success 'repo finds its work tree from work tree, too' '
 	 test sub/dir/tracked = "$(git ls-files)")
 '
 

Quoted 10 lines

t/t1501-worktree.sh test_expect_success 'repo finds its work tree from work tree, too' ' · line 113
Quoted 8 lines
t/t1501-worktree.sh +5 -0
+test_expect_success 'Try a command from subdir in worktree' '
+	(cd repo.git/work/sub &&
+	GIT_DIR=../.. GIT_WORK_TREE=.. git blame dir/tracked)
+'
+
 test_expect_success '_gently() groks relative GIT_DIR & GIT_WORK_TREE' '
 	cd repo.git/work/sub/dir &&
 	GIT_DIR=../../.. GIT_WORK_TREE=../.. GIT_PAGER= \
I am wondering what happened to this thread...

It is still in my inbox, waiting for a time where I can actually concentrate.

Ciao,
Dscho

Johannes Schindelin

2016-06-15 22:43 UTC #6 lore raw

In reply to Nguyen Thai Ngoc Duy

Hi,

Quoted 36 lines · Nguyen Thai Ngoc Duy

Quoted 6 lines · Junio C Hamano
Quoted 4 lines · Nguyễn Thái Ngọc Duy
When both GIT_DIR and GIT_WORK_TREE are set, and setup_git_directory_gently() changes the current working directory accordingly, it should also set inside_work_tree = 1.
Quoted 6 lines
Quoted 3 lines
Without this, work_tree handling code in setup_git_directory() will be activated. If you stay in root work tree (no prefix), it does not harm. It does if you work from a subdirectory though.
Please add automated test script for this, thanks.
Thank you for reminding. I tried to put a test in t1501-worktree.sh and found out core.worktree can override inside_work_tree previously set by setup_git_directory_gently(), activating the worktree code in setup_git_directory() again.
This made me think setup_git_directory_gently() should use get_git_work_tree() instead. But then git_work_tree_cfg may not be initialized when get_git_work_tree() is called (starting from setup_git_directory(), git_work_tree_cfg is initialized in check_repository_format_version(), which is called _after_ setup_git_directory_gently()).
The interaction between these variables and functions is really beyond my knowledge. Johannes, can you have a look at this? In theory the following test should pass:
t/t1501-worktree.sh +0 -0
index 7ee3820..bdb7720 100755
@@ -103,6 +103,11 @@ test_expect_success 'repo finds its work tree from work tree, too' '
 	 test sub/dir/tracked = "$(git ls-files)")

t/t1501-worktree.sh test_expect_success 'repo finds its work tree from work tree, too' ' · line 113

Quoted 10 lines

t/t1501-worktree.sh +5 -0

 '
 
+test_expect_success 'Try a command from subdir in worktree' '
+	(cd repo.git/work/sub &&
+	GIT_DIR=../.. GIT_WORK_TREE=.. git blame dir/tracked)
+'
+
 test_expect_success '_gently() groks relative GIT_DIR & GIT_WORK_TREE' '
 	cd repo.git/work/sub/dir &&
 	GIT_DIR=../../.. GIT_WORK_TREE=../.. GIT_PAGER= \

This does not really test work_tree, but if blame uses the work_tree machinery correctly.

I will send out a patch to builtin-blame.c in a minute.

However, this test case still fails, since blame needs a HEAD revision! And in t1501 there is no commit done yet.

Ciao,
Dscho

Johannes Schindelin

2016-06-15 22:43 UTC #7 lore raw

PATCH 1/1 builtin-blame: set up the work_tree before the first file access

In reply to Nguyen Thai Ngoc Duy

We check in cmd_blame() if the specified path is there, but we failed to set up the working tree before that.

While at it, make setup_work_tree() just return if it was run before.

Signed-off-by Johannes Schindelin <[email protected]>

---

IMO both this patch and the recent patch to call setup_work_tree() are needed. Only the second call to setup_work_tree() will know if there was no revision specified, and the first one will know if a -- has been seen.

 builtin-blame.c |    1 +
 setup.c         |   10 ++++++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

builtin-blame.c +1 -0

index 55a3c0b..ba80bf8 100644
@@ -2295,6 +2295,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 			else if (i != argc - 1)
 				usage(blame_usage); /* garbage at end */
 
+			setup_work_tree();
 			if (!has_path_in_work_tree(path))
 				die("cannot stat path %s: %s",
 				    path, strerror(errno));

setup.c +8 -2

index 084d722..1421a2c 100644
@@ -207,12 +207,18 @@ static const char *set_work_tree(const char *dir)
 }
 
 void setup_work_tree(void) {
-	const char *work_tree = get_git_work_tree();
-	const char *git_dir = get_git_dir();
+	const char *work_tree, *git_dir;
+	static int initialized = 0;
+
+	if (initialized)
+		return;
+	work_tree = get_git_work_tree();
+	git_dir = get_git_dir();
 	if (!is_absolute_path(git_dir))
 		set_git_dir(make_absolute_path(git_dir));
 	if (!work_tree || chdir(work_tree))
 		die("This operation must be run in a work tree");
+	initialized = 1;
 }
 
 /*

Signature · 1 line

1.5.3.5.1645.g1f4df

Johannes Schindelin

2016-06-15 22:43 UTC #8 lore raw

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

Hi,

Quoted 11 lines · Nguyễn Thái Ngọc Duy

When both GIT_DIR and GIT_WORK_TREE are set, and setup_git_directory_gently() changes the current working directory accordingly, it should also set inside_work_tree = 1.
Without this, work_tree handling code in setup_git_directory() will be activated. If you stay in root work tree (no prefix), it does not harm. It does if you work from a subdirectory though.
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>
---
Updated commit message accordingly

Thanks,
Dscho

Johannes Schindelin

2016-06-15 22:43 UTC #9 lore raw

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

Hi,

Quoted 16 lines · Nguyễn Thái Ngọc Duy

Without this, work_tree handling code in setup_git_directory will be activated. If you stay in root work tree (no prefix), it does not harm. It does if you work from a subdirectory though.
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>
---
Turns out my patch on NEED_WORK_TREE is fixing a wrong place.
 setup.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
setup.c +0 -0
index 145eca5..6f8f769 100644

setup.c const char *setup_git_directory_gently(int *nongit_ok) · line 243

Quoted 4 lines

setup.c +1 -0

@@ -240,6 +240,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
 			if (chdir(work_tree_env) < 0)
 				die ("Could not chdir to %s", work_tree_env);
 			strcat(buffer, "/");
+			inside_work_tree = 1;

I really have to wonder why this is needed, as it should be deduced (correctly!) when you ask is_inside_work_tree().

Ciao,
Dscho

A message that is not in the archive (sent off-list or not yet received): [email protected]

Johannes Schindelin

2016-06-15 22:43 UTC #11 lore raw

Hi,

Quoted 28 lines · Nguyen Thai Ngoc Duy

Quoted 13 lines · Johannes Schindelin
Quoted 11 lines · Nguyễn Thái Ngọc Duy
Without this, work_tree handling code in setup_git_directory will be activated. If you stay in root work tree (no prefix), it does not harm. It does if you work from a subdirectory though.
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>
---
Turns out my patch on NEED_WORK_TREE is fixing a wrong place.
 setup.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
Quoted 12 lines
setup.c const char *setup_git_directory_gently(int *nongit_ok) · line 243
Quoted 9 lines
setup.c +1 -0
index 145eca5..6f8f769 100644
@@ -240,6 +240,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
                      if (chdir(work_tree_env) < 0)
                              die ("Could not chdir to %s", work_tree_env);
                      strcat(buffer, "/");
+                     inside_work_tree = 1;
I really have to wonder why this is needed, as it should be deduced (correctly!) when you ask is_inside_work_tree().

Quoted 5 lines

Because setup_git_directory() does not? From what I see, setup_git_directory() calls setup_git_directory_gently() then check for inside_work_tree variable almost immediately (check_repository_format() does not seem to ask is_inside_work_tree()).

Ah, I see the problem.

Quoted 1 line

From your commit message I assumed that you fixed the wrong spot... Which

you did not. It would have helped me if the message had read:

When both GIT_DIR and GIT_WORK_TREE are set, and setup_git_directory_gently() changes the current working directory accordingly, it should also set inside_work_tree = 1.

Your fix is the proper one, but it not only fixes setup_git_directory(), but all callers of setup_git_directory_gently().

Thanks,
Dscho