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

From: Junio C Hamano <junkio@cox.net>
Date: Apr 4, 2007, 22:34 UTC
Message-ID: <7vslbfydiv.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <20070404201313.GB22782@moooo.ath.cx>

Matthias Lederhofer <matled@gmx.net> writes:

Show 5 quoted lines

> 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.

Show 11 quoted lines

> 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?

Show 5 quoted lines

>   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.

Show 22 quoted lines

> 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?

Show 18 quoted lines

> 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.