[BUG] bare repository detection does not work with aliases

[BUG] bare repository detection does not work with aliases

[Mark Lodato]

It seems that the fallback bare repository detection in the absence of
core.bare fails for aliases.

$ git init --bare foo
$ cd foo
$ git config alias.s 'status -sb'
$ git s
fatal: This operation must be run in a work tree
$ sed -i -e '/bare =/d' config
$ git s
## Initial commit on master
?? HEAD
?? config
?? description
?? hooks
?? info/
$ git status -sb
fatal: This operation must be run in a work tree

The reason I am using the fallback is to use a single bare repository
with multiple working directories (via git-new-workdir) as suggested
in 8fa0ee3b [1].

[1] https://github.com/git/git/commit/8fa0ee3b50736eb869a3e13375bb041c1bf5aa12

Re: [BUG] bare repository detection does not work with aliases

[Jeff King]

On Thu, Mar 07, 2013 at 05:47:45PM -0500, Mark Lodato wrote:

> It seems that the fallback bare repository detection in the absence of
> core.bare fails for aliases.

This triggered some deja vu for me, so I went digging. And indeed, this
has been a bug since at least 2008. This patch (which never got applied)
fixed it:

  http://thread.gmane.org/gmane.comp.version-control.git/72792

The issue is that we treat:

  GIT_DIR=/some/path git ...

as if the current directory is the work tree, unless core.bare is
explicitly set, or unless an explicit work tree is given (via
GIT_WORK_TREE, "git --work-tree", or in the config).  This is handy, and
backwards compatible.

Inside setup_git_directory, when we find the directory we put it in
$GIT_DIR for later reference by ourselves or sub-programs (since we are
typically moving to the top of the working tree next, we need to record
the original path, and can't rely on discovery finding the same path
again). But we don't set $GIT_WORK_TREE. So if you don't have core.bare
set, the above rule will kick in for sub-programs, or for aliases (which
will call setup_git_directory again).

The solution is that when we set $GIT_DIR like this, we need to also say
"no, there is no working tree; we are bare". And that's what that patch
does. It's 5 years old now, so not surprisingly, it does not apply
cleanly. The moral equivalent in today's code base would be something
like:

diff --git a/environment.c b/environment.c
index 89d6c70..8edaedd 100644
--- a/environment.c
+++ b/environment.c
@@ -200,7 +200,8 @@ void set_git_work_tree(const char *new_work_tree)
 		return;
 	}
 	git_work_tree_initialized = 1;
-	work_tree = xstrdup(real_path(new_work_tree));
+	if (*new_work_tree)
+		work_tree = xstrdup(real_path(new_work_tree));
 }
 
 const char *get_git_work_tree(void)
diff --git a/setup.c b/setup.c
index e1cfa48..f0e1251 100644
--- a/setup.c
+++ b/setup.c
@@ -544,7 +544,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,
 	worktree = get_git_work_tree();
 
 	/* both get_git_work_tree() and cwd are already normalized */
-	if (!strcmp(cwd, worktree)) { /* cwd == worktree */
+	if (!worktree || !strcmp(cwd, worktree)) { /* cwd == worktree */
 		set_git_dir(gitdirenv);
 		free(gitfile);
 		return NULL;
@@ -636,6 +636,8 @@ static const char *setup_bare_git_dir(char *cwd, int offset, int len, int *nongi
 	}
 	else
 		set_git_dir(".");
+
+	setenv(GIT_WORK_TREE_ENVIRONMENT, "", 1);
 	return NULL;
 }
 

which passes your test. Unfortunately, this patch runs afoul of the same
complaints that prevented the original from being acceptable (weirdness
on Windows with empty environment variables).

Having read the discussion again, I _think_ the more sane thing is to
actually just have a new variable, $GIT_BARE, which overrides any
core.bare config (just as $GIT_WORK_TREE override core.worktree). And
then we set that explicitly when we are in a bare $GIT_DIR, propagating
our auto-detection to sub-processes.

-Peff

Re: [BUG] bare repository detection does not work with aliases

[Junio C Hamano]

The $GIT_BARE idea sounds very sensible to me.



Jeff King <peff@peff.net> wrote:

>On Thu, Mar 07, 2013 at 05:47:45PM -0500, Mark Lodato wrote:
>
>> It seems that the fallback bare repository detection in the absence
>of
>> core.bare fails for aliases.
>
>This triggered some deja vu for me, so I went digging. And indeed, this
>has been a bug since at least 2008. This patch (which never got
>applied)
>fixed it:
>
>  http://thread.gmane.org/gmane.comp.version-control.git/72792
>
>The issue is that we treat:
>
>  GIT_DIR=/some/path git ...
>
>as if the current directory is the work tree, unless core.bare is
>explicitly set, or unless an explicit work tree is given (via
>GIT_WORK_TREE, "git --work-tree", or in the config).  This is handy,
>and
>backwards compatible.
>
>Inside setup_git_directory, when we find the directory we put it in
>$GIT_DIR for later reference by ourselves or sub-programs (since we are
>typically moving to the top of the working tree next, we need to record
>the original path, and can't rely on discovery finding the same path
>again). But we don't set $GIT_WORK_TREE. So if you don't have core.bare
>set, the above rule will kick in for sub-programs, or for aliases
>(which
>will call setup_git_directory again).
>
>The solution is that when we set $GIT_DIR like this, we need to also
>say
>"no, there is no working tree; we are bare". And that's what that patch
>does. It's 5 years old now, so not surprisingly, it does not apply
>cleanly. The moral equivalent in today's code base would be something
>like:
>
>diff --git a/environment.c b/environment.c
>index 89d6c70..8edaedd 100644
>--- a/environment.c
>+++ b/environment.c
>@@ -200,7 +200,8 @@ void set_git_work_tree(const char *new_work_tree)
> 		return;
> 	}
> 	git_work_tree_initialized = 1;
>-	work_tree = xstrdup(real_path(new_work_tree));
>+	if (*new_work_tree)
>+		work_tree = xstrdup(real_path(new_work_tree));
> }
> 
> const char *get_git_work_tree(void)
>diff --git a/setup.c b/setup.c
>index e1cfa48..f0e1251 100644
>--- a/setup.c
>+++ b/setup.c
>@@ -544,7 +544,7 @@ static const char *setup_explicit_git_dir(const
>char *gitdirenv,
> 	worktree = get_git_work_tree();
> 
> 	/* both get_git_work_tree() and cwd are already normalized */
>-	if (!strcmp(cwd, worktree)) { /* cwd == worktree */
>+	if (!worktree || !strcmp(cwd, worktree)) { /* cwd == worktree */
> 		set_git_dir(gitdirenv);
> 		free(gitfile);
> 		return NULL;
>@@ -636,6 +636,8 @@ static const char *setup_bare_git_dir(char *cwd,
>int offset, int len, int *nongi
> 	}
> 	else
> 		set_git_dir(".");
>+
>+	setenv(GIT_WORK_TREE_ENVIRONMENT, "", 1);
> 	return NULL;
> }
> 
>
>which passes your test. Unfortunately, this patch runs afoul of the
>same
>complaints that prevented the original from being acceptable (weirdness
>on Windows with empty environment variables).
>
>Having read the discussion again, I _think_ the more sane thing is to
>actually just have a new variable, $GIT_BARE, which overrides any
>core.bare config (just as $GIT_WORK_TREE override core.worktree). And
>then we set that explicitly when we are in a bare $GIT_DIR, propagating
>our auto-detection to sub-processes.
>
>-Peff
>--
>To unsubscribe from this list: send the line "unsubscribe git" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Pardon terseness, typo and HTML from a tablet.

Re: [BUG] bare repository detection does not work with aliases

[Jeff King]

On Thu, Mar 07, 2013 at 10:27:04PM -0800, Junio C Hamano wrote:

> The $GIT_BARE idea sounds very sensible to me.

Unfortunately, it is not quite as simple as that. I just wrote up the
patch, and it turns out that we are foiled by how core.bare is treated.
If it is true, the repo is definitely bare. If it is false, that is only
a hint for us.

So we cannot just look at is_bare_repository() after setup_git_directory
runs. Because we are not "definitely bare", only "maybe bare", it
returns false. We just happen not to have a work tree. We could do
something like:

  if (is_bare_repository_cfg || !work_tree)
          setenv("GIT_BARE", "1", 1);

which I think would work, but feels kind of wrong. We are bare in this
instance, but somebody setting GIT_WORK_TREE in a sub-process would
want to become unbare, presumably, but our variable would override them.

Just looking through all of the code paths, I am getting a little
nervous that I would not cover all the bases for such a $GIT_BARE to
work (e.g., doing GIT_BARE=0 would not do I would expect as a user,
because of the historical way we treat core.bare=false).

So rather than introduce something like $GIT_BARE which is going to
bring about all new kinds of corner cases, I think I'd rather just pass
along a $GIT_NO_IMPLICIT_WORK_TREE variable, which is much more direct
for solving this problem, and is less likely to end up having bugs of
its own.

-Peff

[PATCH] setup: suppress implicit "." work-tree for bare repos

[Jeff King]

If an explicit GIT_DIR is given without a working tree, we
implicitly assume that the current working directory should
be used as the working tree. E.g.,:

  GIT_DIR=/some/repo.git git status

would compare against the cwd.

Unfortunately, we fool this rule for sub-invocations of git
by setting GIT_DIR internally ourselves. For example:

  git init foo
  cd foo/.git
  git status ;# fails, as we expect
  git config alias.st status
  git status ;# does not fail, but should

What happens is that we run setup_git_directory when doing
alias lookup (since we need to see the config), set GIT_DIR
as a result, and then leave GIT_WORK_TREE blank (because we
do not have one). Then when we actually run the status
command, we do setup_git_directory again, which sees our
explicit GIT_DIR and uses the cwd as an implicit worktree.

It's tempting to argue that we should be suppressing that
second invocation of setup_git_directory, as it could use
the values we already found in memory. However, the problem
still exists for sub-processes (e.g., if "git status" were
an external command).

You can see another example with the "--bare" option, which
sets GIT_DIR explicitly. For example:

  git init foo
  cd foo/.git
  git status ;# fails
  git --bare status ;# does NOT fail

We need some way of telling sub-processes "even though
GIT_DIR is set, do not use cwd as an implicit working tree".
We could do it by putting a special token into
GIT_WORK_TREE, but the obvious choice (an empty string) has
some portability problems, and could potentially be
triggered accidentally by a user.

Instead, we add a new boolean variable, GIT_IMPLICIT_WORK_TREE,
which suppresses the use of cwd as a working tree when
GIT_DIR is set. We trigger the new variable when we know we
are in a bare setting.

The variable is left intentionally undocumented, as this is
an internal detail (for now, anyway). If somebody comes up
with a good alternate use for it, and once we are confident
we have shaken any bugs out of it, we can consider promoting
it further.

Signed-off-by: Jeff King <peff@peff.net>
---
So I think this just ends up being a cleaner and smaller change than
trying to support $GIT_BARE. I think $GIT_BARE could allow more
flexibility, but it's flexibility nobody is particularly asking for, and
there are lots of nasty corner cases around it. I'm pretty sure this is
doing the right thing.

Having written this, I'm still tempted to signal the same thing by
putting /dev/null into GIT_WORK_TREE (Junio's suggestion from the old
thread). This one works OK because we only check GIT_WORK_TREE_IMPLICIT
_after_ exhausting all of the other working tree options, so it is
always subordinate to a later setting of GIT_WORK_TREE. But it seems a
little cleaner for somebody setting GIT_WORK_TREE To clear this
"implicit" flag automatically.

At the same time, I would wonder how other git implementations would
react to GIT_WORK_TREE=/dev/null. Would they try to chdir() there and
barf, when they could happily exist without a working tree? Doing it
this way seems a bit safer from regressions (those other implementations
do not get the _benefit_ of this patch unless they support
GIT_WORK_TREE_IMPLICIT, of course, but at least we are not breaking
them).

 cache.h               |  1 +
 git.c                 |  1 +
 setup.c               |  8 ++++++++
 t/t1510-repo-setup.sh | 19 +++++++++++++++++++
 4 files changed, 29 insertions(+)

diff --git a/cache.h b/cache.h
index e493563..070169a 100644
--- a/cache.h
+++ b/cache.h
@@ -344,6 +344,7 @@ static inline enum object_type object_type(unsigned int mode)
 #define GIT_DIR_ENVIRONMENT "GIT_DIR"
 #define GIT_NAMESPACE_ENVIRONMENT "GIT_NAMESPACE"
 #define GIT_WORK_TREE_ENVIRONMENT "GIT_WORK_TREE"
+#define GIT_IMPLICIT_WORK_TREE_ENVIRONMENT "GIT_IMPLICIT_WORK_TREE"
 #define DEFAULT_GIT_DIR_ENVIRONMENT ".git"
 #define DB_ENVIRONMENT "GIT_OBJECT_DIRECTORY"
 #define INDEX_ENVIRONMENT "GIT_INDEX_FILE"
diff --git a/git.c b/git.c
index b10c18b..24b7984 100644
--- a/git.c
+++ b/git.c
@@ -125,6 +125,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 			static char git_dir[PATH_MAX+1];
 			is_bare_repository_cfg = 1;
 			setenv(GIT_DIR_ENVIRONMENT, getcwd(git_dir, sizeof(git_dir)), 0);
+			setenv(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, "0", 1);
 			if (envchanged)
 				*envchanged = 1;
 		} else if (!strcmp(cmd, "-c")) {
diff --git a/setup.c b/setup.c
index 1dee47e..6c87660 100644
--- a/setup.c
+++ b/setup.c
@@ -523,6 +523,12 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,
 			set_git_work_tree(core_worktree);
 		}
 	}
+	else if (!git_env_bool(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, 1)) {
+		/* #16d */
+		set_git_dir(gitdirenv);
+		free(gitfile);
+		return NULL;
+	}
 	else /* #2, #10 */
 		set_git_work_tree(".");
 
@@ -601,6 +607,8 @@ static const char *setup_bare_git_dir(char *cwd, int offset, int len, int *nongi
 	if (check_repository_format_gently(".", nongit_ok))
 		return NULL;
 
+	setenv(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, "0", 1);
+
 	/* --work-tree is set without --git-dir; use discovered one */
 	if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {
 		const char *gitdir;
diff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh
index 80aedfc..cf2ee78 100755
--- a/t/t1510-repo-setup.sh
+++ b/t/t1510-repo-setup.sh
@@ -517,6 +517,25 @@ test_expect_success '#16c: bare .git has no worktree' '
 		"$here/16c/.git" "(null)" "$here/16c/sub" "(null)"
 '
 
+test_expect_success '#16d: bareness preserved across alias' '
+	setup_repo 16d unset "" unset &&
+	(
+		cd 16d/.git &&
+		test_must_fail git status &&
+		git config alias.st status &&
+		test_must_fail git st
+	)
+'
+
+test_expect_success '#16e: bareness preserved by --bare' '
+	setup_repo 16e unset "" unset &&
+	(
+		cd 16e/.git &&
+		test_must_fail git status &&
+		test_must_fail git --bare status
+	)
+'
+
 test_expect_success '#17: GIT_WORK_TREE without explicit GIT_DIR is accepted (bare case)' '
 	# Just like #16.
 	setup_repo 17a unset "" true &&
-- 
1.8.2.rc2.4.g3e774bb

Re: [PATCH] setup: suppress implicit "." work-tree for bare repos

[Johannes Sixt]

Am 3/8/2013 8:15, schrieb Jeff King:
> --- a/cache.h
> +++ b/cache.h
> @@ -344,6 +344,7 @@ static inline enum object_type object_type(unsigned int mode)
>  #define GIT_DIR_ENVIRONMENT "GIT_DIR"
>  #define GIT_NAMESPACE_ENVIRONMENT "GIT_NAMESPACE"
>  #define GIT_WORK_TREE_ENVIRONMENT "GIT_WORK_TREE"
> +#define GIT_IMPLICIT_WORK_TREE_ENVIRONMENT "GIT_IMPLICIT_WORK_TREE"
>  #define DEFAULT_GIT_DIR_ENVIRONMENT ".git"
>  #define DB_ENVIRONMENT "GIT_OBJECT_DIRECTORY"
>  #define INDEX_ENVIRONMENT "GIT_INDEX_FILE"

This new variable needs to be added to environment.c:local_repo_env, right?

-- Hannes

Re: [PATCH] setup: suppress implicit "." work-tree for bare repos

[Jeff King]

On Fri, Mar 08, 2013 at 08:44:20AM +0100, Johannes Sixt wrote:

> Am 3/8/2013 8:15, schrieb Jeff King:
> > --- a/cache.h
> > +++ b/cache.h
> > @@ -344,6 +344,7 @@ static inline enum object_type object_type(unsigned int mode)
> >  #define GIT_DIR_ENVIRONMENT "GIT_DIR"
> >  #define GIT_NAMESPACE_ENVIRONMENT "GIT_NAMESPACE"
> >  #define GIT_WORK_TREE_ENVIRONMENT "GIT_WORK_TREE"
> > +#define GIT_IMPLICIT_WORK_TREE_ENVIRONMENT "GIT_IMPLICIT_WORK_TREE"
> >  #define DEFAULT_GIT_DIR_ENVIRONMENT ".git"
> >  #define DB_ENVIRONMENT "GIT_OBJECT_DIRECTORY"
> >  #define INDEX_ENVIRONMENT "GIT_INDEX_FILE"
> 
> This new variable needs to be added to environment.c:local_repo_env, right?

It does; I had no idea local_repo_env existed. We should add a comment
to cache.h to that effect, too.

-Peff

[PATCHv2] setup and GIT_IMPLICIT_WORK_TREE

[Jeff King]

Here's a re-roll of the GIT_IMPLICIT_WORK_TREE patch which should
address the comments in the last round. I've added an explanatory
comment near the variable definition, and added it to local_repo_env.

While doing that, I noticed some cleanup opportunities around
local_repo_env, which resulted in the first two patches.

  [1/3]: cache.h: drop LOCAL_REPO_ENV_SIZE
  [2/3]: environment: add GIT_PREFIX to local_repo_env
  [3/3]: setup: suppress implicit "." work-tree for bare repos

-Peff

[PATCH v2 1/3] cache.h: drop LOCAL_REPO_ENV_SIZE

[Jeff King]

We keep a static array of variables that should be cleared
when invoking a sub-process on another repo. We statically
size the array with the LOCAL_REPO_ENV_SIZE macro so that
any readers do not have to count it themselves.

As it turns out, no readers actually use the macro, and it
creates a maintenance headache, as modifications to the
array need to happen in two places (one to add the new
element, and another to bump the size).

Since it's NULL-terminated, we can just drop the size macro
entirely. While we're at it, we'll clean up some comments
around it, and add a new mention of it at the top of the
list of environment variable macros. Even though
local_repo_env is right below that list, it's easy to miss,
and additions to that list should consider local_repo_env.

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h       | 12 ++++++------
 environment.c |  6 ++----
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/cache.h b/cache.h
index e493563..b90044a 100644
--- a/cache.h
+++ b/cache.h
@@ -341,6 +341,7 @@ static inline enum object_type object_type(unsigned int mode)
 		OBJ_BLOB;
 }
 
+/* Double-check local_repo_env below if you add to this list. */
 #define GIT_DIR_ENVIRONMENT "GIT_DIR"
 #define GIT_NAMESPACE_ENVIRONMENT "GIT_NAMESPACE"
 #define GIT_WORK_TREE_ENVIRONMENT "GIT_WORK_TREE"
@@ -365,13 +366,12 @@ static inline enum object_type object_type(unsigned int mode)
 #define GIT_LITERAL_PATHSPECS_ENVIRONMENT "GIT_LITERAL_PATHSPECS"
 
 /*
- * Repository-local GIT_* environment variables
- * The array is NULL-terminated to simplify its usage in contexts such
- * environment creation or simple walk of the list.
- * The number of non-NULL entries is available as a macro.
+ * Repository-local GIT_* environment variables; these will be cleared
+ * when git spawns a sub-process that runs inside another repository.
+ * The array is NULL-terminated, which makes it easy to pass in the "env"
+ * parameter of a run-command invocation, or to do a simple walk.
  */
-#define LOCAL_REPO_ENV_SIZE 9
-extern const char *const local_repo_env[LOCAL_REPO_ENV_SIZE + 1];
+extern const char * const local_repo_env[];
 
 extern int is_bare_repository_cfg;
 extern int is_bare_repository(void);
diff --git a/environment.c b/environment.c
index 89d6c70..dc73927 100644
--- a/environment.c
+++ b/environment.c
@@ -83,11 +83,9 @@ static char *git_object_dir, *git_index_file, *git_graft_file;
 static char *git_object_dir, *git_index_file, *git_graft_file;
 
 /*
- * Repository-local GIT_* environment variables
- * Remember to update local_repo_env_size in cache.h when
- * the size of the list changes
+ * Repository-local GIT_* environment variables; see cache.h for details.
  */
-const char * const local_repo_env[LOCAL_REPO_ENV_SIZE + 1] = {
+const char * const local_repo_env[] = {
 	ALTERNATE_DB_ENVIRONMENT,
 	CONFIG_ENVIRONMENT,
 	CONFIG_DATA_ENVIRONMENT,
-- 
1.8.2.rc2.4.g3e774bb

[PATCH v2 2/3] environment: add GIT_PREFIX to local_repo_env

[Jeff King]

The GIT_PREFIX variable is set based on our location within
the working tree. It should therefore be cleared whenever
GIT_WORK_TREE is cleared.

In practice, this doesn't cause any bugs, because none of
the sub-programs we invoke with local_repo_env cleared
actually care about GIT_PREFIX. But this is the right thing
to do, and future proofs us again that assumption changing.

While we're at it, let's define a GIT_PREFIX_ENVIRONMENT
macro; this avoids repetition of the string literal, which
can help catch any spelling mistakes in the code.

Signed-off-by: Jeff King <peff@peff.net>
---
I noticed this one because it was near code I was touching in an earlier
iteration of patch 3. I gave a quick skim and did not notice any other
variables which would want to receive the same treatment.

 cache.h       | 1 +
 environment.c | 1 +
 setup.c       | 4 ++--
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/cache.h b/cache.h
index b90044a..23e6e62 100644
--- a/cache.h
+++ b/cache.h
@@ -345,6 +345,7 @@ static inline enum object_type object_type(unsigned int mode)
 #define GIT_DIR_ENVIRONMENT "GIT_DIR"
 #define GIT_NAMESPACE_ENVIRONMENT "GIT_NAMESPACE"
 #define GIT_WORK_TREE_ENVIRONMENT "GIT_WORK_TREE"
+#define GIT_PREFIX_ENVIRONMENT "GIT_PREFIX"
 #define DEFAULT_GIT_DIR_ENVIRONMENT ".git"
 #define DB_ENVIRONMENT "GIT_OBJECT_DIRECTORY"
 #define INDEX_ENVIRONMENT "GIT_INDEX_FILE"
diff --git a/environment.c b/environment.c
index dc73927..2bd1c37 100644
--- a/environment.c
+++ b/environment.c
@@ -95,6 +95,7 @@ const char * const local_repo_env[] = {
 	GRAFT_ENVIRONMENT,
 	INDEX_ENVIRONMENT,
 	NO_REPLACE_OBJECTS_ENVIRONMENT,
+	GIT_PREFIX_ENVIRONMENT,
 	NULL
 };
 
diff --git a/setup.c b/setup.c
index 1dee47e..1996295 100644
--- a/setup.c
+++ b/setup.c
@@ -794,9 +794,9 @@ const char *setup_git_directory_gently(int *nongit_ok)
 
 	prefix = setup_git_directory_gently_1(nongit_ok);
 	if (prefix)
-		setenv("GIT_PREFIX", prefix, 1);
+		setenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);
 	else
-		setenv("GIT_PREFIX", "", 1);
+		setenv(GIT_PREFIX_ENVIRONMENT, "", 1);
 
 	if (startup_info) {
 		startup_info->have_repository = !nongit_ok || !*nongit_ok;
-- 
1.8.2.rc2.4.g3e774bb

Re: [PATCH v2 2/3] environment: add GIT_PREFIX to local_repo_env

[Eric Sunshine]

On Fri, Mar 8, 2013 at 4:30 AM, Jeff King <peff@peff.net> wrote:
> The GIT_PREFIX variable is set based on our location within
> the working tree. It should therefore be cleared whenever
> GIT_WORK_TREE is cleared.
>
> In practice, this doesn't cause any bugs, because none of
> the sub-programs we invoke with local_repo_env cleared
> actually care about GIT_PREFIX. But this is the right thing
> to do, and future proofs us again that assumption changing.

s/again/against/

-- ES

Re: [PATCH v2 2/3] environment: add GIT_PREFIX to local_repo_env

[Jeff King]

On Fri, Mar 08, 2013 at 04:39:02PM -0500, Eric Sunshine wrote:

> On Fri, Mar 8, 2013 at 4:30 AM, Jeff King <peff@peff.net> wrote:
> > The GIT_PREFIX variable is set based on our location within
> > the working tree. It should therefore be cleared whenever
> > GIT_WORK_TREE is cleared.
> >
> > In practice, this doesn't cause any bugs, because none of
> > the sub-programs we invoke with local_repo_env cleared
> > actually care about GIT_PREFIX. But this is the right thing
> > to do, and future proofs us again that assumption changing.
> 
> s/again/against/

Yep, thanks.

-Peff

Re: [PATCH v2 2/3] environment: add GIT_PREFIX to local_repo_env

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Mar 08, 2013 at 04:39:02PM -0500, Eric Sunshine wrote:
>
>> On Fri, Mar 8, 2013 at 4:30 AM, Jeff King <peff@peff.net> wrote:
>> > The GIT_PREFIX variable is set based on our location within
>> > the working tree. It should therefore be cleared whenever
>> > GIT_WORK_TREE is cleared.
>> >
>> > In practice, this doesn't cause any bugs, because none of
>> > the sub-programs we invoke with local_repo_env cleared
>> > actually care about GIT_PREFIX. But this is the right thing
>> > to do, and future proofs us again that assumption changing.
>> 
>> s/again/against/
>
> Yep, thanks.

Thanks; squashed-in.

[PATCH v2 3/3] setup: suppress implicit "." work-tree for bare repos

[Jeff King]

If an explicit GIT_DIR is given without a working tree, we
implicitly assume that the current working directory should
be used as the working tree. E.g.,:

  GIT_DIR=/some/repo.git git status

would compare against the cwd.

Unfortunately, we fool this rule for sub-invocations of git
by setting GIT_DIR internally ourselves. For example:

  git init foo
  cd foo/.git
  git status ;# fails, as we expect
  git config alias.st status
  git status ;# does not fail, but should

What happens is that we run setup_git_directory when doing
alias lookup (since we need to see the config), set GIT_DIR
as a result, and then leave GIT_WORK_TREE blank (because we
do not have one). Then when we actually run the status
command, we do setup_git_directory again, which sees our
explicit GIT_DIR and uses the cwd as an implicit worktree.

It's tempting to argue that we should be suppressing that
second invocation of setup_git_directory, as it could use
the values we already found in memory. However, the problem
still exists for sub-processes (e.g., if "git status" were
an external command).

You can see another example with the "--bare" option, which
sets GIT_DIR explicitly. For example:

  git init foo
  cd foo/.git
  git status ;# fails
  git --bare status ;# does NOT fail

We need some way of telling sub-processes "even though
GIT_DIR is set, do not use cwd as an implicit working tree".
We could do it by putting a special token into
GIT_WORK_TREE, but the obvious choice (an empty string) has
some portability problems.

Instead, we add a new boolean variable, GIT_IMPLICIT_WORK_TREE,
which suppresses the use of cwd as a working tree when
GIT_DIR is set. We trigger the new variable when we know we
are in a bare setting.

The variable is left intentionally undocumented, as this is
an internal detail (for now, anyway). If somebody comes up
with a good alternate use for it, and once we are confident
we have shaken any bugs out of it, we can consider promoting
it further.

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h               | 12 ++++++++++++
 environment.c         |  1 +
 git.c                 |  1 +
 setup.c               |  8 ++++++++
 t/t1510-repo-setup.sh | 19 +++++++++++++++++++
 5 files changed, 41 insertions(+)

diff --git a/cache.h b/cache.h
index 23e6e62..635f2e9 100644
--- a/cache.h
+++ b/cache.h
@@ -367,6 +367,18 @@ static inline enum object_type object_type(unsigned int mode)
 #define GIT_LITERAL_PATHSPECS_ENVIRONMENT "GIT_LITERAL_PATHSPECS"
 
 /*
+ * This environment variable is expected to contain a boolean indicating
+ * whether we should or should not treat:
+ *
+ *   GIT_DIR=foo.git git ...
+ *
+ * as if GIT_WORK_TREE=. was given. It's not expected that users will make use
+ * of this, but we use it internally to communicate to sub-processes that we
+ * are in a bare repo. If not set, defaults to true.
+ */
+#define GIT_IMPLICIT_WORK_TREE_ENVIRONMENT "GIT_IMPLICIT_WORK_TREE"
+
+/*
  * Repository-local GIT_* environment variables; these will be cleared
  * when git spawns a sub-process that runs inside another repository.
  * The array is NULL-terminated, which makes it easy to pass in the "env"
diff --git a/environment.c b/environment.c
index 2bd1c37..e2e75c1 100644
--- a/environment.c
+++ b/environment.c
@@ -92,6 +92,7 @@ const char * const local_repo_env[] = {
 	DB_ENVIRONMENT,
 	GIT_DIR_ENVIRONMENT,
 	GIT_WORK_TREE_ENVIRONMENT,
+	GIT_IMPLICIT_WORK_TREE_ENVIRONMENT,
 	GRAFT_ENVIRONMENT,
 	INDEX_ENVIRONMENT,
 	NO_REPLACE_OBJECTS_ENVIRONMENT,
diff --git a/git.c b/git.c
index b10c18b..24b7984 100644
--- a/git.c
+++ b/git.c
@@ -125,6 +125,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 			static char git_dir[PATH_MAX+1];
 			is_bare_repository_cfg = 1;
 			setenv(GIT_DIR_ENVIRONMENT, getcwd(git_dir, sizeof(git_dir)), 0);
+			setenv(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, "0", 1);
 			if (envchanged)
 				*envchanged = 1;
 		} else if (!strcmp(cmd, "-c")) {
diff --git a/setup.c b/setup.c
index 1996295..9107f54 100644
--- a/setup.c
+++ b/setup.c
@@ -523,6 +523,12 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,
 			set_git_work_tree(core_worktree);
 		}
 	}
+	else if (!git_env_bool(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, 1)) {
+		/* #16d */
+		set_git_dir(gitdirenv);
+		free(gitfile);
+		return NULL;
+	}
 	else /* #2, #10 */
 		set_git_work_tree(".");
 
@@ -601,6 +607,8 @@ static const char *setup_bare_git_dir(char *cwd, int offset, int len, int *nongi
 	if (check_repository_format_gently(".", nongit_ok))
 		return NULL;
 
+	setenv(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, "0", 1);
+
 	/* --work-tree is set without --git-dir; use discovered one */
 	if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {
 		const char *gitdir;
diff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh
index 80aedfc..cf2ee78 100755
--- a/t/t1510-repo-setup.sh
+++ b/t/t1510-repo-setup.sh
@@ -517,6 +517,25 @@ test_expect_success '#16c: bare .git has no worktree' '
 		"$here/16c/.git" "(null)" "$here/16c/sub" "(null)"
 '
 
+test_expect_success '#16d: bareness preserved across alias' '
+	setup_repo 16d unset "" unset &&
+	(
+		cd 16d/.git &&
+		test_must_fail git status &&
+		git config alias.st status &&
+		test_must_fail git st
+	)
+'
+
+test_expect_success '#16e: bareness preserved by --bare' '
+	setup_repo 16e unset "" unset &&
+	(
+		cd 16e/.git &&
+		test_must_fail git status &&
+		test_must_fail git --bare status
+	)
+'
+
 test_expect_success '#17: GIT_WORK_TREE without explicit GIT_DIR is accepted (bare case)' '
 	# Just like #16.
 	setup_repo 17a unset "" true &&
-- 
1.8.2.rc2.4.g3e774bb

Re: [PATCH] setup: suppress implicit "." work-tree for bare repos

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> diff --git a/cache.h b/cache.h
> index e493563..070169a 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -344,6 +344,7 @@ static inline enum object_type object_type(unsigned int mode)
>  #define GIT_DIR_ENVIRONMENT "GIT_DIR"
>  #define GIT_NAMESPACE_ENVIRONMENT "GIT_NAMESPACE"
>  #define GIT_WORK_TREE_ENVIRONMENT "GIT_WORK_TREE"
> +#define GIT_IMPLICIT_WORK_TREE_ENVIRONMENT "GIT_IMPLICIT_WORK_TREE"
>  #define DEFAULT_GIT_DIR_ENVIRONMENT ".git"
>  #define DB_ENVIRONMENT "GIT_OBJECT_DIRECTORY"
>  #define INDEX_ENVIRONMENT "GIT_INDEX_FILE"

Not adding any user documentation is fine (you explained why in the
log message), but I would really prefer to have some in-code comment
to clarify its meaning.  Is it "Please do use implicit work tree"
boolean?  Is it "This is the path to the work tree we have already
figured out" string?  Is it something else?  What is it used for,
who sets it, what other codepath that will be invented in the future
need to be careful to set it (or unset it) and how does one who
writes that new codepath decides that he needs to do so (or
shouldn't)?

I would know *today* that it is a bool to affect us, after having
discovered that we are in bare and we have set GIT_DIR (so if the
end user already had GIT_DIR, we shouldn't set it ourselves), and
also our child processes, but I am not confident that I will
remember this thread 6 months down the road.

Re: [PATCH] setup: suppress implicit "." work-tree for bare repos

[Jeff King]

On Thu, Mar 07, 2013 at 11:54:18PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > diff --git a/cache.h b/cache.h
> > index e493563..070169a 100644
> > --- a/cache.h
> > +++ b/cache.h
> > @@ -344,6 +344,7 @@ static inline enum object_type object_type(unsigned int mode)
> >  #define GIT_DIR_ENVIRONMENT "GIT_DIR"
> >  #define GIT_NAMESPACE_ENVIRONMENT "GIT_NAMESPACE"
> >  #define GIT_WORK_TREE_ENVIRONMENT "GIT_WORK_TREE"
> > +#define GIT_IMPLICIT_WORK_TREE_ENVIRONMENT "GIT_IMPLICIT_WORK_TREE"
> >  #define DEFAULT_GIT_DIR_ENVIRONMENT ".git"
> >  #define DB_ENVIRONMENT "GIT_OBJECT_DIRECTORY"
> >  #define INDEX_ENVIRONMENT "GIT_INDEX_FILE"
> 
> Not adding any user documentation is fine (you explained why in the
> log message), but I would really prefer to have some in-code comment
> to clarify its meaning.  Is it "Please do use implicit work tree"
> boolean?  Is it "This is the path to the work tree we have already
> figured out" string?  Is it something else?  What is it used for,
> who sets it, what other codepath that will be invented in the future
> need to be careful to set it (or unset it) and how does one who
> writes that new codepath decides that he needs to do so (or
> shouldn't)?

My intent was that the commit message would be enough to explain it, but
it is a pain for a later reader to have to blame the line back to that
commit to read it. I'll re-roll with a comment.

-Peff