On Thu, Jul 09, 2020 at 07:14:41AM -0400, Derrick Stolee wrote:
> On 7/8/2020 10:29 PM, Jonathan Tan wrote:
> >> +static int run_write_commit_graph(struct repository *r)
> >> +{
> >> + int result;
> >> + struct argv_array cmd = ARGV_ARRAY_INIT;
> >> +
> >> + argv_array_pushl(&cmd, "-C", r->worktree,
> >> + "commit-graph", "write",
> >> + "--split", "--reachable",
> >> + NULL);
> >
> > As mentioned in my reply to an earlier patch (sent a few minutes ago),
> > this won't work if there are environment variables like GIT_DIR present.
>
> Do we not pass GIT_DIR to the subcommand? Or does using "-C" override
> the GIT_DIR?
We do pass GIT_DIR to the subcommand, and "-C" does not override it. I
think this code would work as long as "r" is the_repository, which it
would be in the current code. But then the "-C" would be doing nothing
useful (it might change to the top of the worktree if we weren't there
for some reason, but I don't think "commit-graph write" would care
either way).
But if "r" is some other repository, "commit-graph" would continue to
operate in the parent process repository because of the inherited
GIT_DIR. Using "--git-dir" would solve that, but as a general practice,
if you're spawning a sub-process that might be in another repository,
you should clear any repo-specific environment variables. The list is in
local_repo_env, which you can feed to the "env" or "env_array" parameter
of a child_process (see the use in connect.c for an example).
Even in the current scheme where "r" is always the_repository, I suspect
this might still be buggy. If we're in a bare repository, presumably
r->worktree would be NULL.
-Peff
