Matthias Lederhofer <matled@xxxxxxx> 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 tree
Sounds sane so far.
> if the repository was found in ".":
> use "." as working tree
I 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.
> 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?
> 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.
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html