Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to ".git"

From: Jeff King <peff@peff.net>
Date: Nov 22, 2016, 02:41 UTC
Message-ID: <20161122024102.otlnl6jcrb3pejux@sigill.intra.peff.net>
In-Reply-To: <20161122004421.GA12263@google.com>

On Mon, Nov 21, 2016 at 04:44:21PM -0800, Jonathan Nieder wrote:

Show 30 quoted lines

> >  	git_dir = getenv(GIT_DIR_ENVIRONMENT);
> > -	if (!git_dir)
> > +	if (!git_dir) {
> > +		if (!startup_info->have_repository)
> > +			die("BUG: setup_git_env called without repository");
> >  		git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
> > +	}
> 
> This trips reproducibly for
> 
>   git ls-remote https://kernel.googlesource.com/pub/scm/git/git
> 
> when run outside of a git repository.
> 
> Backtrace:
> 
>   #0  setup_git_env () at environment.c:172
>   #1  get_git_dir () at environment.c:214
>   #2  get_helper at transport-helper.c:127
>   #3  get_refs_list (for_push=0) at transport-helper.c:1038
>   #4  transport_get_remote_refs at transport.c:1076
>   #5  cmd_ls_remote at builtin/ls-remote.c:97
> 
> transport-helper.c:127 is
> 
> 	argv_array_pushf(&helper->env_array, "%s=%s", GIT_DIR_ENVIRONMENT,
> 			 get_git_dir());
> 
> That code is pretty clearly wrong.  Should it be made conditional
> on have_git_dir(), like this?

Yeah, I think making it conditional makes the most sense. Just trying to think of cases that might not be covered by your patch:

  - if we are not in a repository, we shouldn't ever need to override an
    existing $GIT_DIR from the environment. Because if $GIT_DIR is
    present, then we _would_ be in a repo if it's valid, and die if it
    isn't.

  - not setting $GIT_DIR may cause a helper to do the usual discovery
    walk to find the repository. But we know we're not in one, or we
    would have found it ourselves. So worst case it may expend a little
    effort to try to find a repository and fail (I think remote-curl
    would do this to try to find repo-level config).

Both of those points assume that we've already called setup_git_directory_gently(), but I think that's a reasonable assumption. And certainly is true for ls-remote, and should be for any git command that invokes the transport code.

Show 13 quoted lines

> diff --git a/transport-helper.c b/transport-helper.c
> index 91aed35..e4fd982 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -124,8 +124,9 @@ static struct child_process *get_helper(struct transport *transport)
>  	helper->git_cmd = 0;
>  	helper->silent_exec_failure = 1;
>  
> -	argv_array_pushf(&helper->env_array, "%s=%s", GIT_DIR_ENVIRONMENT,
> -			 get_git_dir());
> +	if (have_git_dir())
> +		argv_array_pushf(&helper->env_array, "%s=%s",
> +				 GIT_DIR_ENVIRONMENT, get_git_dir());

So yeah, I think this is the extent of the change needed.

Thanks.

-Peff