Re: [BUG] bare repository detection does not work with aliases
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] bare repository detection does not work with aliases
The $GIT_BARE idea sounds very sensible to me.
Jeff King 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 majord...@vger.kernel.org
>More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Pardon terseness, typo and HTML from a tablet.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] bare repository detection does not work with aliases
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
