> 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. I mentioned "reset" because the effect of adding the name without any value makes the environment variable of that name unset in the subprocess. I'll word it as you say, and add "When used as the env_array of a subprocess, these entries cause the corresponding environment variables to be unset in the subprocess." after the first sentence. > (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_". OK, that makes sense. > Otherwise the movement looks fine to me. > > Thanks, > Taylor Thanks.
