threads / patch / 11912

patchallow setting GIT_WORK_TREE to "no work tree"

Subject: [PATCH] allow setting GIT_WORK_TREE to "no work tree"

## tl;dr

13 messages between Feb 6, 2008 and Feb 8, 2008. Diffs are folded; open one to read it.

replies: 12people: 5as markdown or json

Jeff King· Feb 6, 2008, 10:26 UTC · lore

In setup_git_directory_gently, we have a special rule that says "if GIT_DIR is set but GIT_WORK_TREE is not, then use the current working directory as the work tree." This is the intended behavior for the user perspective.

However, setup_git_directory_gently sets GIT_DIR itself, meaning that further setups (either because we are executing a command via alias, or in a subprocess) will see the non-existent GIT_WORK_TREE and assume we fall into the "current working directory is the working tree" codepath.

Instead, we now use a special value of GIT_WORK_TREE to indicate that we have already checked for a worktree and that there isn't one, setting it when we set GIT_DIR and checking for it in the special case path.

The special value is a blank GIT_WORK_TREE; it could be any value, but this should not conflict with any user values (and as a bonus, you can now tell git "I don't have a work tree" with "GIT_WORK_TREE= git", though I suspect the use case for that is limited).

Signed-off-by: Jeff King <peff@peff.net>
---
On Sun, Feb 03, 2008 at 02:59:13AM -0800, Junio C Hamano wrote:
> * "[alias] st = status" and "cd .git && git st" (Jeff King)

This turned out to be easier than expected. Note that this patch causes t1500 to fail; however, the test is at fault, and is already fixed in the series I just sent out.

 environment.c                 |    9 +++++++--
 setup.c                       |    1 +
 t/t1502-worktree-semantics.sh |   20 ++++++++++++++++++++
 3 files changed, 28 insertions(+), 2 deletions(-)
 create mode 100755 t/t1502-worktree-semantics.sh
Show changes to 3 files +28 −2

environment.c, setup.c, t/t1502-worktree-semantics.sh

diff --git a/environment.c b/environment.c
index 18a1c4e..38e8dff 100644
--- a/environment.c
+++ b/environment.c
@@ -90,8 +90,13 @@ const char *get_git_work_tree(void)
 			/* make_absolute_path also normalizes the path */
 			if (work_tree && !is_absolute_path(work_tree))
 				work_tree = xstrdup(make_absolute_path(git_path(work_tree)));
-		} else if (work_tree)
-			work_tree = xstrdup(make_absolute_path(work_tree));
+		} else if (work_tree) {
+			if (*work_tree)
+				work_tree = xstrdup(
+						make_absolute_path(work_tree));
+			else
+				work_tree = NULL;
+		}
 		initialized = 1;
 		if (work_tree)
 			is_bare_repository_cfg = 0;
diff --git a/setup.c b/setup.c
index adede16..85c0b48 100644
--- a/setup.c
+++ b/setup.c
@@ -309,6 +309,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
 			if (!work_tree_env)
 				inside_work_tree = 0;
 			setenv(GIT_DIR_ENVIRONMENT, ".", 1);
+			setenv(GIT_WORK_TREE_ENVIRONMENT, "", 1);
 			check_repository_format_gently(nongit_ok);
 			return NULL;
 		}
diff --git a/t/t1502-worktree-semantics.sh b/t/t1502-worktree-semantics.sh
new file mode 100755
index 0000000..fbc355b
--- /dev/null
+++ b/t/t1502-worktree-semantics.sh
@@ -0,0 +1,20 @@
+#!/bin/sh
+
+test_description='semantics of GIT_WORK_TREE variable'
+. ./test-lib.sh
+
+test_expect_success 'setup working tree' 'touch file'
+
+test_expect_success 'setup alias' 'git config alias.a add'
+
+test_expect_success 'blank GIT_WORK_TREE disallows work-tree commands' '
+	! GIT_WORK_TREE= git add .
+'
+
+test_expect_success 'alias inside working tree works' 'git a .'
+
+test_expect_success 'alias inside .git complains about working tree' '
+	cd .git && ! GIT_CONFIG=config git a .
+'
+
+test_done

-- 
1.5.4.26.g301fb
Johannes Sixt· Feb 6, 2008, 10:42 UTC · re: Jeff King · lore

Re: [PATCH] allow setting GIT_WORK_TREE to "no work tree"

Jeff King schrieb:
Show 21 quoted lines
> In setup_git_directory_gently, we have a special rule
> that says "if GIT_DIR is set but GIT_WORK_TREE is not, then
> use the current working directory as the work tree." This is
> the intended behavior for the user perspective.
> 
> However, setup_git_directory_gently sets GIT_DIR itself,
> meaning that further setups (either because we are executing
> a command via alias, or in a subprocess) will see the
> non-existent GIT_WORK_TREE and assume we fall into the
> "current working directory is the working tree" codepath.
> 
> Instead, we now use a special value of GIT_WORK_TREE to
> indicate that we have already checked for a worktree and
> that there isn't one, setting it when we set GIT_DIR and
> checking for it in the special case path.
> 
> The special value is a blank GIT_WORK_TREE; it could be any
> value, but this should not conflict with any user values
> (and as a bonus, you can now tell git "I don't have a work
> tree" with "GIT_WORK_TREE= git", though I suspect the use
> case for that is limited).

Hrm. Unfortunately, on Windows there is no such thing as an empty environment string. setenv(x, "") *removes* the environment variable.

-- Hannes
Jeff King· Feb 6, 2008, 11:01 UTC · re: Johannes Sixt · lore

Re: [PATCH] allow setting GIT_WORK_TREE to "no work tree"

On Wed, Feb 06, 2008 at 11:42:15AM +0100, Johannes Sixt wrote:
Show 8 quoted lines
> > The special value is a blank GIT_WORK_TREE; it could be any
> > value, but this should not conflict with any user values
> > (and as a bonus, you can now tell git "I don't have a work
> > tree" with "GIT_WORK_TREE= git", though I suspect the use
> > case for that is limited).
> 
> Hrm. Unfortunately, on Windows there is no such thing as an empty
> environment string. setenv(x, "") *removes* the environment variable.

Bleh. Maybe "GIT_WORK_TREE=:"? It doesn't make sense by itself since we don't try to execute the contents of GIT_WORK_TREE, but it's unlikely to be used by a user, and I believe there was recent talk of making "GIT_EDITOR=:" work.

The other option is setting
  GIT_MAGICALLY_SET_GIT_DIR_SO_DONT_ACT_LIKE_THE_USER_DID=1
but I was hoping to avoid that.
-Peff
Junio C Hamano· Feb 6, 2008, 20:54 UTC · re: Jeff King · lore

Re: [PATCH] allow setting GIT_WORK_TREE to "no work tree"

Jeff King <peff@peff.net> writes:
Show 21 quoted lines
> On Wed, Feb 06, 2008 at 11:42:15AM +0100, Johannes Sixt wrote:
>
>> > The special value is a blank GIT_WORK_TREE; it could be any
>> > value, but this should not conflict with any user values
>> > (and as a bonus, you can now tell git "I don't have a work
>> > tree" with "GIT_WORK_TREE= git", though I suspect the use
>> > case for that is limited).
>> 
>> Hrm. Unfortunately, on Windows there is no such thing as an empty
>> environment string. setenv(x, "") *removes* the environment variable.
>
> Bleh. Maybe "GIT_WORK_TREE=:"? It doesn't make sense by itself since we
> don't try to execute the contents of GIT_WORK_TREE, but it's unlikely to
> be used by a user, and I believe there was recent talk of making
> "GIT_EDITOR=:" work.
>
> The other option is setting
>
>   GIT_MAGICALLY_SET_GIT_DIR_SO_DONT_ACT_LIKE_THE_USER_DID=1
>
> but I was hoping to avoid that.
Yuck.  Let's then try your original (slightly redundant) one.
Junio C Hamano· Feb 6, 2008, 20:59 UTC · re: Junio C Hamano · lore

Re: [PATCH] allow setting GIT_WORK_TREE to "no work tree"

Junio C Hamano <gitster@pobox.com> writes:
Show 7 quoted lines
>> The other option is setting
>>
>>   GIT_MAGICALLY_SET_GIT_DIR_SO_DONT_ACT_LIKE_THE_USER_DID=1
>>
>> but I was hoping to avoid that.
>
> Yuck.  Let's then try your original (slightly redundant) one.
Oops, scratch that.  I was confused.

I do not like the magic GIT_WORK_TREE=: which is simply illogical. GIT_EDITOR=: made perfect sense (":" is actually a command that succeeds without doing anything), but ":" does not have anything to do with "there is no such path".

I was tempted to suggest GIT_WORK_TREE=/dev/null because that is what "diff" uses to mark "this does not even exist", but that feels dirty.

Jeff King· Feb 7, 2008, 05:13 UTC · re: Junio C Hamano · lore

Re: [PATCH] allow setting GIT_WORK_TREE to "no work tree"

On Wed, Feb 06, 2008 at 12:59:32PM -0800, Junio C Hamano wrote:
> I do not like the magic GIT_WORK_TREE=: which is simply
> illogical.  GIT_EDITOR=: made perfect sense (":" is actually a
> command that succeeds without doing anything), but ":" does not
> have anything to do with "there is no such path".

No, it doesn't, but they are both no-ops. Sort of. I agree the reasoning is stretched, but I think we are just going to have to choose _some_ arbitrary value.

Unless you want to switch it to a "git set GIT_DIR automatically, so don't pretend the user set it" flag variable.

> I was tempted to suggest GIT_WORK_TREE=/dev/null because that is
> what "diff" uses to mark "this does not even exist", but that
> feels dirty.

It feels dirty to me, too. I would rather choose something that doesn't even look like a path. I think GIT_WORK_TREE=" " is too ugly, too.

Also, having just looked at the '.git-file' code, it does the same thing (setting GIT_DIR even though the user didn't explicitly ask for it), so it will need a similar fix. Which makes me lean towards just setting a "we set this behind the user's back" flag, since that code won't even have anything to do with the worktree.

In fact, the more I think about it, that makes sense. WORK_TREE munging just happens to be the only special behavior right now that depends on whether the user manually set GIT_DIR. But what we really want to communicate to later code is not "I have corrected this particular munge" but "don't run any special behavior as a result of this variable being set."

-Peff
Jay Soffian· Feb 7, 2008, 07:13 UTC · re: Jeff King · lore

Re: [PATCH] allow setting GIT_WORK_TREE to "no work tree"

On Feb 7, 2008 12:13 AM, Jeff King <peff@peff.net> wrote:
Show 9 quoted lines
>
> On Wed, Feb 06, 2008 at 12:59:32PM -0800, Junio C Hamano wrote:
> >
> > I was tempted to suggest GIT_WORK_TREE=/dev/null because that is
> > what "diff" uses to mark "this does not even exist", but that
> > feels dirty.
>
> It feels dirty to me, too. I would rather choose something that doesn't
> even look like a path. I think GIT_WORK_TREE=" " is too ugly, too.
Is using something like "__GIT_WORK_TREE_NOT_SET__" that terrible?
j.
Johannes Schindelin· Feb 7, 2008, 12:33 UTC · re: Jay Soffian · lore

Re: [PATCH] allow setting GIT_WORK_TREE to "no work tree"

Hi,
On Thu, 7 Feb 2008, Jay Soffian wrote:
Show 12 quoted lines
> On Feb 7, 2008 12:13 AM, Jeff King <peff@peff.net> wrote:
> >
> > On Wed, Feb 06, 2008 at 12:59:32PM -0800, Junio C Hamano wrote:
> > >
> > > I was tempted to suggest GIT_WORK_TREE=/dev/null because that is
> > > what "diff" uses to mark "this does not even exist", but that
> > > feels dirty.
> >
> > It feels dirty to me, too. I would rather choose something that doesn't
> > even look like a path. I think GIT_WORK_TREE=" " is too ugly, too.
> 
> Is using something like "__GIT_WORK_TREE_NOT_SET__" that terrible?

Yes. First: it looks more like a C constant than a proper environment variable. Second: what to do _sanely_, when both GIT_WORK_TREE and GIT_WORK_TREE_NOT_SET are true?

Of course, you could argue that one should override the other. But the consequence will be user errors that could be prevented, thus it is a design error.

Ciao, Dscho

Jay Soffian· Feb 7, 2008, 18:06 UTC · re: Johannes Schindelin · lore

Re: [PATCH] allow setting GIT_WORK_TREE to "no work tree"

On Feb 7, 2008 7:33 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
Show 8 quoted lines
>
> On Thu, 7 Feb 2008, Jay Soffian wrote:
>
> > Is using something like "__GIT_WORK_TREE_NOT_SET__" that terrible?
>
> Yes.  First: it looks more like a C constant than a proper environment
> variable.  Second: what to do _sanely_, when both GIT_WORK_TREE and
> GIT_WORK_TREE_NOT_SET are true?

Sorry I was unclear. The discussion was about using a special value to denote "this is not set." So I meant something like:

GIT_WORK_TREE="__GIT_WORK_TREE_NOT_SET"

There may not be precedent in git, but it is not unusual to use a double-underbar prefix to denote private names and/or values. While in theory a user could have a directory named as such, it would seem highly unlikely. This looks a little cleaner to me than using ":", " ", or "/dev/null".

j.
Junio C Hamano· Feb 7, 2008, 09:10 UTC · re: Jeff King · lore

Re: [PATCH] allow setting GIT_WORK_TREE to "no work tree"

Jeff King <peff@peff.net> writes:
Show 6 quoted lines
> In fact, the more I think about it, that makes sense. WORK_TREE munging
> just happens to be the only special behavior right now that depends on
> whether the user manually set GIT_DIR. But what we really want to
> communicate to later code is not "I have corrected this particular
> munge" but "don't run any special behavior as a result of this variable
> being set."
Yeah, that makes sense.
Johannes Schindelin· Feb 7, 2008, 19:02 UTC · re: Johannes Sixt · lore

Re: [PATCH] allow setting GIT_WORK_TREE to "no work tree"

Hi,
On Wed, 6 Feb 2008, Johannes Sixt wrote:
Show 24 quoted lines
> Jeff King schrieb:
> > In setup_git_directory_gently, we have a special rule that says "if 
> > GIT_DIR is set but GIT_WORK_TREE is not, then use the current working 
> > directory as the work tree." This is the intended behavior for the 
> > user perspective.
> > 
> > However, setup_git_directory_gently sets GIT_DIR itself, meaning that 
> > further setups (either because we are executing a command via alias, 
> > or in a subprocess) will see the non-existent GIT_WORK_TREE and assume 
> > we fall into the "current working directory is the working tree" 
> > codepath.
> > 
> > Instead, we now use a special value of GIT_WORK_TREE to indicate that 
> > we have already checked for a worktree and that there isn't one, 
> > setting it when we set GIT_DIR and checking for it in the special case 
> > path.
> > 
> > The special value is a blank GIT_WORK_TREE; it could be any value, but 
> > this should not conflict with any user values (and as a bonus, you can 
> > now tell git "I don't have a work tree" with "GIT_WORK_TREE= git", 
> > though I suspect the use case for that is limited).
> 
> Hrm. Unfortunately, on Windows there is no such thing as an empty 
> environment string. setenv(x, "") *removes* the environment variable.
That might be a shortcoming of our implementation of setenv():

-- snip -- cd /git

cat > a1.c << EOF #include <stdio.h> #include "compat/setenv.c" #include "compat/unsetenv.c"

static void p()
{
	const char *abc = getenv("ABC");
	printf("env ABC: %s\n", abc ? abc : "(null)");
}
int main()
{
	p();
	gitsetenv("ABC", "Hello", 1);
	p();
	gitsetenv("ABC", "", 1);
	p();
	gitunsetenv("ABC");
	p();
	return 0;
}
EOF
gcc -DNO_MMAP=1 -I. -Icompat -o a1.exe a1.c

ABC="" ./a1.exe -- snap --

This will show

env ABC: env ABC: Hello env ABC: (null) env ABC: (null)

So it seems that environment variables _can_ be empty. Just our relatively stupid implementation of setenv() does not do it.

Maybe something like compat/unsetenv.c is needed in setenv(), too.

Ciao, Dscho

Johannes Sixt· Feb 8, 2008, 07:21 UTC · re: Johannes Schindelin · lore

Re: [PATCH] allow setting GIT_WORK_TREE to "no work tree"

Johannes Schindelin schrieb:
Show 29 quoted lines
> Hi,
> 
> On Wed, 6 Feb 2008, Johannes Sixt wrote:
> 
>> Jeff King schrieb:
>>> In setup_git_directory_gently, we have a special rule that says "if 
>>> GIT_DIR is set but GIT_WORK_TREE is not, then use the current working 
>>> directory as the work tree." This is the intended behavior for the 
>>> user perspective.
>>>
>>> However, setup_git_directory_gently sets GIT_DIR itself, meaning that 
>>> further setups (either because we are executing a command via alias, 
>>> or in a subprocess) will see the non-existent GIT_WORK_TREE and assume 
>>> we fall into the "current working directory is the working tree" 
>>> codepath.
>>>
>>> Instead, we now use a special value of GIT_WORK_TREE to indicate that 
>>> we have already checked for a worktree and that there isn't one, 
>>> setting it when we set GIT_DIR and checking for it in the special case 
>>> path.
>>>
>>> The special value is a blank GIT_WORK_TREE; it could be any value, but 
>>> this should not conflict with any user values (and as a bonus, you can 
>>> now tell git "I don't have a work tree" with "GIT_WORK_TREE= git", 
>>> though I suspect the use case for that is limited).
>> Hrm. Unfortunately, on Windows there is no such thing as an empty 
>> environment string. setenv(x, "") *removes* the environment variable.
> 
> That might be a shortcoming of our implementation of setenv():
No, it is not. It's Windows's putenv(), and it's even documented.
Show 43 quoted lines
> -- snip --
> cd /git
> 
> cat > a1.c << EOF
> #include <stdio.h>
> #include "compat/setenv.c"
> #include "compat/unsetenv.c"
> 
> static void p()
> {
> 	const char *abc = getenv("ABC");
> 	printf("env ABC: %s\n", abc ? abc : "(null)");
> }
> 
> int main()
> {
> 	p();
> 	gitsetenv("ABC", "Hello", 1);
> 	p();
> 	gitsetenv("ABC", "", 1);
> 	p();
> 	gitunsetenv("ABC");
> 	p();
> 	return 0;
> }
> EOF
> 
> gcc -DNO_MMAP=1 -I. -Icompat -o a1.exe a1.c
> 
> ABC="" ./a1.exe 
> -- snap --
> 
> This will show
> 
> env ABC: 
> env ABC: Hello
> env ABC: (null)
> env ABC: (null)
> 
> So it seems that environment variables _can_ be empty.  Just our 
> relatively stupid implementation of setenv() does not do it.
> 
> Maybe something like compat/unsetenv.c is needed in setenv(), too.

This only shows that, yes, variables _can_ be empty - if the setenv/putenv implementation is "sane", like MSYS/bash's.

That said, we probably should modify environ directly in gitsetenv().
-- Hannes
Johannes Schindelin· Feb 8, 2008, 12:05 UTC · re: Johannes Sixt · lore

Re: [PATCH] allow setting GIT_WORK_TREE to "no work tree"

Hi,
On Fri, 8 Feb 2008, Johannes Sixt wrote:
Show 32 quoted lines
> Johannes Schindelin schrieb:
> 
> > On Wed, 6 Feb 2008, Johannes Sixt wrote:
> > 
> >> Jeff King schrieb:
> >>> In setup_git_directory_gently, we have a special rule that says "if 
> >>> GIT_DIR is set but GIT_WORK_TREE is not, then use the current 
> >>> working directory as the work tree." This is the intended behavior 
> >>> for the user perspective.
> >>>
> >>> However, setup_git_directory_gently sets GIT_DIR itself, meaning 
> >>> that further setups (either because we are executing a command via 
> >>> alias, or in a subprocess) will see the non-existent GIT_WORK_TREE 
> >>> and assume we fall into the "current working directory is the 
> >>> working tree" codepath.
> >>>
> >>> Instead, we now use a special value of GIT_WORK_TREE to indicate 
> >>> that we have already checked for a worktree and that there isn't 
> >>> one, setting it when we set GIT_DIR and checking for it in the 
> >>> special case path.
> >>>
> >>> The special value is a blank GIT_WORK_TREE; it could be any value, 
> >>> but this should not conflict with any user values (and as a bonus, 
> >>> you can now tell git "I don't have a work tree" with "GIT_WORK_TREE= 
> >>> git", though I suspect the use case for that is limited).
> >>
> >> Hrm. Unfortunately, on Windows there is no such thing as an empty 
> >> environment string. setenv(x, "") *removes* the environment variable.
> > 
> > That might be a shortcoming of our implementation of setenv():
> 
> No, it is not. It's Windows's putenv(), and it's even documented.

Yes, that's what I said: our setenv() implementation uses putenv(), which introduces that bug (setenv() is not supposed to unset variables, unsetenv() is).

> That said, we probably should modify environ directly in gitsetenv().
Exactly.

Ciao, Dscho

← back to recent threads