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

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

[Jeff King]

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

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

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

[Johannes Sixt]

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.

-- Hannes

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

[Jeff King]

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.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> 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.

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

[Jeff King]

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

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

[Jay Soffian]

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?

j.

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

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

[Johannes Schindelin]

Hi,

On Thu, 7 Feb 2008, Jay Soffian wrote:

> 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

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

[Jay Soffian]

On Feb 7, 2008 7:33 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>
> 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.

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

[Johannes Schindelin]

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():

-- 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

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

[Johannes Sixt]

Johannes Schindelin schrieb:
> 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.

> -- 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

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

[Johannes Schindelin]

Hi,

On Fri, 8 Feb 2008, Johannes Sixt wrote:

> 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