Re: [PATCH v2 1/2] Clean stale environment pointer in finish_command()

From: Jeff King <peff@peff.net>
Date: Nov 11, 2014, 03:16 UTC
Message-ID: <20141111031636.GE21328@peff.net>
In-Reply-To: <alpine.DEB.1.00.1411101539091.13845@s15462909.onlinehome-server.info>

On Mon, Nov 10, 2014 at 03:41:09PM +0100, Johannes Schindelin wrote:

Show 12 quoted lines

> > However, start_command() will set "env" to env_array.argv only if "env"
> > was unset to begin with, and if it was already set, the caller will need
> > the original value. Therefore, we need to be very careful only to reset
> > "env" in finish_command() when it has been initialized in start_command().
> 
> In case it was unclear: this is needed for the the suggested switch from the
> previous method to construct the environment to the new env_array method
> to work.
> 
> (The env_array method unfortunately requires the code to initialize the
> environment twice because finish_command() insists on always releasing the
> env_array, even if the caller may want to reuse the generated array).

I don't think this is "unfortunately"; freeing the memory was the entire purpose in adding env_array. If you want to easily reuse the same environment in multiple commands, it is still perfectly fine to use "env" directly, like:

  struct argv_array env = ARGV_ARRAY_INIT;
  struct child_process one = CHILD_PROCESS_INIT;
  struct child_process two = CHILD_PROCESS_INIT;

  ... setup env with argv_array_push ...

  one.argv = foo;
  one.env = env.argv;
  run_command(&one);

  two.argv = bar;
  two.env = env.argv;
  run_command(&two);

  argv_array_clear(&env);

You do not get the benefit of the auto-cleanup (you have to call argv_array_clear yourself), but that is less bad than repeating the setup of "env" twice.

It may be that Documentation/technical/api-run-command.txt needs to make this more explicit (I just read over it and it looks OK to me. But since I am the one who designed the feature, I am not the best test of whether it is sufficiently clear).

-Peff