Re: [PATCH(amend)] introduce GIT_WORK_TREE environment variable
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:43:03
Matthias Lederhofer [off-list ref] writes:
GIT_WORK_TREE can be used with GIT_DIR to specify the working tree. As for GIT_DIR there is also the option `git --work-tree=GIT_WORK_TREE` which overrides the environment variable and a config setting core.worktree which is used as default value.
I haven't followed the latest code to see if it does what it claims to in the log message (although I looked at the last round briefly). Most of my comments below are not objections but questions.
setup_git_directory_gently() is rewritten and does the following now: find the repository directory ($GIT_DIR, ".git" in parent directories, ".") read the configuration (core.bare and core.worktree are used) if core.bare is not set assume the repository is not bare if a working tree is specified or guess based on the name of the repository directory
Sounds sane so far.
for a bare repository:
set GIT_DIR if it is not set already and stop (this wont change
the directory even if the repository was found as .git in a
parent directory)If you have a normal non-bare layout repository and have core.bare set (perhaps by mistake), currently we chdir(2) up to the working tree root, but the new code doesn't. Is it a problem in practice? If you have a truly bare repository (perhaps a one that is serving the general public over gitweb), and you use GIT_DIR and GIT_WORK_TREE to have a working tree elsewhere so that you can hack on it, running a git command from a subdirectory would not chdir(2) up to the root of the working tree. I suspect that the callers of setup_git_directory() would expect to see the same "cd up to the root and return the prefix" behaviour. Is this a problem in practice?
for a non-bare repository:
if GIT_DIR is specified:
use GIT_WORK_TREE, core.worktree or "." as working tree
if the repository was found as .git in a parent directory:
use the parent directory of the .git directory as working treeSounds sane so far.
if the repository was found in ".":
use "." as working treeI am not sure about this. Is there ever a case that the repository (i.e. the directory that has objects/, refs/, and HEAD) can also be a sane working tree root? Wouldn't it be saner to say this case is without any working tree and fail commands that require a working tree?
set inside_git_dir and inside_working_tree based on getcwd() and
prefixcmp()is_bare_repository() is also changed to return true if the working directory is outside of the working tree.
What does this mean from operational perspective? Suppose you are using GIT_DIR and GIT_WORK_TREE to have a working tree that is separate from your working area, and get interrupted and go somewhere else (say "pushd /var/tmp"). Does this suddenly allow "git fetch" into the repository to update the current branch tip? That does not sound right, but it might not be a good use case to begin with. I dunno, but I think is_bare_repository should mean "I am treating this repository as a bare repository". That means I do not want to have "no-working-tree" semantics applied to the repository operation, regardless of where my $cwd happens to be, once I say GIT_WORK_TREE to name which working tree I am using to work with that repository. What problem are you solving with this is_bare_repository() thing? Could it be that you are working around problems with the current callers that behave inappropriately, based on is_bare_repository(), when they should really be checking inside_work_tree() instead, perhaps?
- call setup_git_env() from setup_git_directory_gently() if needed
Calling setup_git_env() again would leak memory for git_object_dir and friends, but I have a bigger worry about this change. If calling setup_git_env() explicitly after resetting GIT_DIR and stuff in this code makes any difference, doesn't that mean somebody else already called a function in environment.c (say, get_refs_directory()) and also have already _acted_ on it (e.g. calling get_packed_refs() which populates the list of refs based on the old value of "$GIT_DIR/packed-refs")? Calling the function again would not undo/redo that, so maybe the calling sequence into setup_git_env() needs to be made safer. I think the only thing you care about in your "where is the repo and where is the worktree" codepath are get_git_dir() and is_bare_repository(). As a side effect of calling these functions for your own purpose, you later have to call setup_git_env() again to clean up, which is fine. I would feel better if there is an assert in setup_git_env that catches the case where it is called for the second time even though the first caller was something other than this repository/worktree discovery code.
quoted hunk
diff --git a/environment.c b/environment.c index 713a011..769d409 100644 --- a/environment.c +++ b/environment.c@@ -60,8 +60,15 @@ void setup_git_env(void) int is_bare_repository(void) { const char *dir, *s; - if (0 <= is_bare_repository_cfg) - return is_bare_repository_cfg; + /* definitely bare */ + if (is_bare_repository_cfg == 1) + return 1; + /* bare if cwd is outside of the working tree */ + if (inside_working_tree >= 0) + return !inside_working_tree; + /* configuration says it is not bare */ + if (is_bare_repository_cfg == 0) + return 0; dir = get_git_dir(); if (!strcmp(dir, DEFAULT_GIT_DIR_ENVIRONMENT))
For example, calling this function from programs before calling the repository/worktree discovery is an error, isn't it? Can we have a safety here to catch such a programming error?
quoted hunk
diff --git a/setup.c b/setup.c index a45ea83..794edcf 100644 --- a/setup.c +++ b/setup.c@@ -192,67 +192,168 @@ int is_inside_git_dir(void) return inside_git_dir; } +static char *git_work_tree; + +static int git_setup_config(const char *var, const char *value) +{ + if (git_work_tree && !strcmp(var, "core.worktree")) { + strlcpy(git_work_tree, value, PATH_MAX); + } + return git_default_config(var, value); +} +
Other config functions do not pass already handled variables to git_default_config(). You probably would care about core.bare in your own code, so falling back to git_default_config() is fine, although it may look wasteful.