Re: [PATCH 6/8] environment.c: adjust env containing relpath when $CWD is moved

[Jeff King <peff@peff.net>]

On Wed, Mar 28, 2018 at 07:55:35PM +0200, Nguyễn Thái Ngọc Duy wrote:

> From: Duy Nguyen <pclouds@gmail.com>
> 
> As noted in the previous patch, when $CWD is moved, we recognize the
> problem with relative paths and update $GIT_WORK_TREE and $GIT_DIR
> with new ones.
> 
> We have plenty more environment variables that can contain paths
> though. If they are read and cached before setup_work_tree() is
> called, nobody will update them and they become bad paths.

Hmm, yeah, I missed these. It would be interesting to know if there are
easy-to-run test cases that show off these bugs, or if they're
hypothetical. (Even if they are hypothetical, I'm not opposed to fixing
them in the name of maintainability).

> Hook into setup_work_tree() and update all those env variables. The
> code to update $GIT_WORK_TREE is no longer needed and removed.

That's a nice cleanup.

> Technically we should remove the setenv() in set_git_dir() as well,
> but that is also called _not_ by setup_work_tree() and we should keep
> the behavior the same in that case for safety. set_git_dir() will be
> removed from setup_work_tree() soon, which achieves the same goal.

Makes sense.

> diff --git a/environment.c b/environment.c
> index 39b3d906c8..f9dcc1b99e 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -128,6 +128,20 @@ const char * const local_repo_env[] = {
>  	NULL
>  };
>  
> +/* A subset of local_repo_env[] that contains path */
> +const char * const local_repo_path_env[] = {
> +	ALTERNATE_DB_ENVIRONMENT,
> +	CONFIG_ENVIRONMENT,
> +	DB_ENVIRONMENT,
> +	GIT_COMMON_DIR_ENVIRONMENT,
> +	GIT_DIR_ENVIRONMENT,
> +	GIT_SHALLOW_FILE_ENVIRONMENT,
> +	GIT_WORK_TREE_ENVIRONMENT,
> +	GRAFT_ENVIRONMENT,
> +	INDEX_ENVIRONMENT,
> +	NULL
> +};

It might be nice to fold this list into local_repo_env automatically. I
think you'd have to do it with a macro.

OTOH, it's possible that there could be a path-related environment
variable that _isn't_ actually part of local_repo_env. E.g., I think
GIT_CONFIG might classify there (though I don't know if it's worth
worrying about).

> +static void update_path_envs(const char *old_cwd, const char *new_cwd,
> +			     void *cb)
> +{
> +	int i;
> +
> +	/*
> +	 * FIXME: special treatment needed for
> +	 * GIT_ALTERNATE_OBJECT_DIRECTORIES because it can contain
> +	 * multiple paths.
> +	 */

Yuck. It just keeps getting more complicated. :(

I do wonder if relative paths in variables like this are worth worrying
about. AFAIK, it's always been a "well, it kind of works" situation, but
not something we've tried to actively support. I think with the current
code you'd potentially get inconsistent results between a command which
sets up the work tree and one which doesn't. So this would be fixing
that, but at the same time, I'm not sure how much we want to promise
here.

-Peff