Re: [PATCH] setup: suppress implicit . work-tree for bare repos
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 -- 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: [PATCH] setup: suppress implicit . work-tree for bare repos
On Thu, Mar 07, 2013 at 11:54:18PM -0800, Junio C Hamano wrote: Jeff King p...@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 -- 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
[PATCH] setup: suppress implicit . work-tree for bare repos
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 p...@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,
Re: [PATCH] setup: suppress implicit . work-tree for bare repos
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 -- 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: [PATCH] setup: suppress implicit . work-tree for bare repos
Jeff King p...@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. -- 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
