threads / patch / 10609

patchAdd missing inside_work_tree setting in setup_git_directory_gently

Subject: [PATCH] Add missing inside_work_tree setting in setup_git_directory_gently

## tl;dr

11 messages between Nov 3, 2007 and Nov 9, 2007. Diffs are folded; open one to read it.

replies: 10people: 3as markdown or json

Nguyễn Thái Ngọc Duy· Nov 3, 2007, 10:03 UTC · lore

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 <pclouds@gmail.com>
---
 Turns out my patch on NEED_WORK_TREE is fixing a wrong place.
 setup.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
Show changes to setup.c +1 −0
diff --git a/setup.c b/setup.c
index 145eca5..6f8f769 100644
--- a/setup.c
+++ b/setup.c
@@ -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) {

-- 
1.5.3.rc4.3.gab089
Johannes Schindelin· Nov 3, 2007, 11:38 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH] Add missing inside_work_tree setting in setup_git_directory_gently

Hi,
On Sat, 3 Nov 2007, Nguyễn Thái Ngọc Duy wrote:
Show 20 quoted 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.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  Turns out my patch on NEED_WORK_TREE is fixing a wrong place.
> 
>  setup.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/setup.c b/setup.c
> index 145eca5..6f8f769 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -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

Nguyễn Thái Ngọc Duy· Nov 3, 2007, 13:18 UTC · re: Nguyễn Thái Ngọc Duy · lore

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 <pclouds@gmail.com>
---
 Updated commit message accordingly
 setup.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
Show changes to setup.c +1 −0
diff --git a/setup.c b/setup.c
index 145eca5..6f8f769 100644
--- a/setup.c
+++ b/setup.c
@@ -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) {

-- 
1.5.3.rc4.3.gab089
Johannes Schindelin· Nov 3, 2007, 13:25 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH] Add missing inside_work_tree setting in setup_git_directory_gently

Hi,
On Sat, 3 Nov 2007, Nguyễn Thái Ngọc Duy wrote:
Show 11 quoted lines
> 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 <pclouds@gmail.com>
> ---
>  Updated commit message accordingly

Thanks, Dscho

Junio C Hamano· Nov 4, 2007, 04:33 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH] Add missing inside_work_tree setting in setup_git_directory_gently

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
Show 7 quoted lines
> 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· Nov 4, 2007, 07:03 UTC · re: Junio C Hamano · lore

Re: [PATCH] Add missing inside_work_tree setting in setup_git_directory_gently

On Sat, Nov 03, 2007 at 09:33:40PM -0700, Junio C Hamano wrote:
Show 12 quoted lines
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> 
> > 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.
>

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:

Show changes to t/t1501-worktree.sh +5 −0
diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh
index 7ee3820..bdb7720 100755
--- a/t/t1501-worktree.sh
+++ b/t/t1501-worktree.sh
@@ -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= \

-- 
Duy
Junio C Hamano· Nov 7, 2007, 20:42 UTC · re: Nguyen Thai Ngoc Duy · lore

Re: [PATCH] Add missing inside_work_tree setting in setup_git_directory_gently

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
Show 36 quoted lines
> On Sat, Nov 03, 2007 at 09:33:40PM -0700, Junio C Hamano wrote:
>> 
>> 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:
>
> diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh
> index 7ee3820..bdb7720 100755
> --- a/t/t1501-worktree.sh
> +++ b/t/t1501-worktree.sh
> @@ -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= \
I am wondering what happened to this thread...
Johannes Schindelin· Nov 7, 2007, 20:54 UTC · re: Junio C Hamano · lore

Re: [PATCH] Add missing inside_work_tree setting in setup_git_directory_gently

Hi,
On Wed, 7 Nov 2007, Junio C Hamano wrote:
Show 40 quoted lines
> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
> 
> > On Sat, Nov 03, 2007 at 09:33:40PM -0700, Junio C Hamano wrote:
> >> 
> >> 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:
> >
> > diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh
> > index 7ee3820..bdb7720 100755
> > --- a/t/t1501-worktree.sh
> > +++ b/t/t1501-worktree.sh
> > @@ -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= \
> 
> 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· Nov 9, 2007, 11:32 UTC · re: Nguyen Thai Ngoc Duy · lore

Re: [PATCH] Add missing inside_work_tree setting in setup_git_directory_gently

Hi,
On Sun, 4 Nov 2007, Nguyen Thai Ngoc Duy wrote:
Show 46 quoted lines
> On Sat, Nov 03, 2007 at 09:33:40PM -0700, Junio C Hamano wrote:
> > Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> > 
> > > 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.
> > 
> 
> 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:
> 
> diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh
> index 7ee3820..bdb7720 100755
> --- a/t/t1501-worktree.sh
> +++ b/t/t1501-worktree.sh
> @@ -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= \

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· Nov 9, 2007, 11:34 UTC · re: Nguyen Thai Ngoc Duy · lore

[PATCH] builtin-blame: set up the work_tree before the first file access

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 <johannes.schindelin@gmx.de>
---
	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(-)
Show changes to 2 files +9 −2

builtin-blame.c, setup.c

diff --git a/builtin-blame.c b/builtin-blame.c
index 55a3c0b..ba80bf8 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -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));
diff --git a/setup.c b/setup.c
index 084d722..1421a2c 100644
--- a/setup.c
+++ b/setup.c
@@ -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;
 }
 
 /*

-- 
1.5.3.5.1645.g1f4df
Johannes Schindelin· Nov 3, 2007, 12:16 UTC · lore

Re: [PATCH] Add missing inside_work_tree setting in setup_git_directory_gently

Hi,
On Sat, 3 Nov 2007, Nguyen Thai Ngoc Duy wrote:
Show 33 quoted lines
> On 11/3/07, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>
> > On Sat, 3 Nov 2007, Nguyễn Thái Ngọc Duy wrote:
> >
> > > 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 <pclouds@gmail.com>
> > > ---
> > >  Turns out my patch on NEED_WORK_TREE is fixing a wrong place.
> > >
> > >  setup.c |    1 +
> > >  1 files changed, 1 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/setup.c b/setup.c
> > > index 145eca5..6f8f769 100644
> > > --- a/setup.c
> > > +++ b/setup.c
> > > @@ -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().
> 
> 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.
>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

← back to recent threads