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

From: Taylor Blau <me@ttaylorr.com>
Date: Jun 4, 2021, 20:19 UTC
Message-ID: <YLqK6XTmnaQoFycz@nand.local>
In-Reply-To: <c5307a9f02f3f0f9b30c87bb3e9f8fa3163f839d.1622580781.git.jonathantanmy@google.com>

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

Show 6 quoted lines

> 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 <jonathantanmy@google.com>

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