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 <jonathantanmy@xxxxxxxxxx>
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
