Code review: pass the whole process.env to git child process?

[josecelano]

Imported from: https://github.com/josecelano/library-consumer/issues/15

We are using the SimpleGit package to create git commits.

The git commit command accepts some env vars like:

GIT_COMMITTER_NAME
GIT_COMMITTER_EMAIL
GIT_COMMITTER_DATE

SimpleGit also uses a child process to run git and you can pass that child process env vars with:

git.env(process.env);

We are passing the whole process.env.

Wouldn't be better to only pass the variables we know it's going to consume? If we do that we need to modify the code every time we detect a missing env var, and anyway when you use git directly in a workflow git is going to have access to all the env vars.

On the other hand, some things could work just because we have a needed env var and we do not know it. Maybe we can pass the whole env on PRODUCTION and only the consumed vars in TESTS.

Notice that I've removed the commit author env vars because we are planning to overwrite the author with the action name.

[josecelano]

Passing all env vars:

Pros

• Easier to maintain. We don't need to add more env vars if we need them.
• The user of the action can use any Git feature that depend on env vars.

Cons

• We could pass critical env vars to a third-party package. We are only using some specific Git commands, and very stable commands, so they are not likely to change.

Passing only env vars for the commands we use and for behaviour we explicitly want to allow the user to change:

Pros

• Safer. We only grant access to the process to what we know for sure it needs.

Cons

• A user could ask us to add a env var for commit in order to overwrite the command behaviour. For example, overwrite the commit author. In fact, we are always overwriting the commit author to set our bot. So in that case we won't be consistent if we allow only some env vars.
• We could be already using some env vars without even knowing it, for example the language. If the user has configured the runner to use a different language, that configuration could be omitted by Git. It could show messages in a different language.

I think in general we can get more problems than advantages. We have other production node dependencies that also have access to env vars. I guess, this case in only different because the code it's executed in a different thread.

I would suggest to keep it as it is, for now.

@yeraydavidrodriguez @da2ce7