Re: [PATCH(amend)] introduce GIT_WORK_TREE environment variable

[Junio C Hamano <junkio@cox.net>]

Matthias Lederhofer <matled@gmx.net> 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.