Re: [PATCH 3/4] run-command: move envvar-resetting function

On Tue, Jun 01, 2021 at 02:34:18PM -0700, Jonathan Tan wrote:

There is a function that resets environment variables, used when
invoking a sub-process in a submodule. The lazy-fetching code (used in
partial clones) will need this function in a subsequent commit, so move
it to a more central location.

Signed-off-by: Jonathan Tan <redacted>

All seems pretty normal to me. I did have one question, though:

+/**
+ * Convenience function that adds entries to env_array that resets all

Hmm. Why "resets"? IIUC local_repo_env is the array of environment
variables that change behavior. With that understanding in mind, I
probably would have written something more like:

    Convenience function which adds all GIT_* environment variables to
    env_array with the exception of GIT_CONFIG_PARAMETERS. See
    local_repo_env in cache.h for more information.

(Confusingly, cache.h calls this variable CONFIG_DATA_ENVIRONMENT, but
binds it to GIT_CONFIG_PARAMETERS. I think it probably makes more sense
to use the environment variable's name rather than our #define, since
we're saying "all GIT_* variables, except this one", so it would be
weird for "this one" not to start with "GIT_".

Otherwise the movement looks fine to me.

Thanks,
Taylor