On Mon, Nov 21, 2016 at 04:44:21PM -0800, Jonathan Nieder wrote:
> > 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.
> 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