avoid os.Setenv calls in runServ, add env vars to gitcmd.Env instead #3772
Conversation
Codecov Report
@@ Coverage Diff @@ ## master #3772 +/- ## ========================================= Coverage ? 37.61% ========================================= Files ? 310 Lines ? 46040 Branches ? 0 ========================================= Hits ? 17319 Misses ? 26242 Partials ? 2479Continue to review full report at Codecov.
|
bkcsoft added the lgtm/need 2 
lafriks added the type/refactoring 
cmd/serv.go Outdated
| } | ||
| | ||
| os.Setenv(models.EnvRepoUsername, username) | ||
| env := map[string]string{} |
There was a problem hiding this comment.
I would prefer to avoid using a map and then joning all of its keys (maps are quite expensive to allocate and env vars will always be in random order because of how range works on maps) - can't you use a bytes.Buffer instead?
There was a problem hiding this comment.
I changed it to []string, the same thing that we need to give to the command (gitcmd.Env)
cmd/serv.go Outdated
| os.Setenv(models.EnvRepoUsername, username) | ||
| env := map[string]string{} | ||
| | ||
| env[models.EnvRepoUsername] = username |
There was a problem hiding this comment.
... and make these buf.WriteString(models.EnvRepoUsername + "=" + username + "\n") etc. instead?
There was a problem hiding this comment.
We will need to convert it to []string at the end (strings.Split). I don't think using buffer is worth it (we don't set too many env vars)
| This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions. |
| This pull request has been automatically closed because of inactivity. You can re-open it if needed. |
6 participants
Using
os.Setenv()is the middle of a running program is very unsafe, specially if some vars are set conditionally (their effect will remain until the program is running)I think the only reason we set these env vars is because of running the git command
gitcmd.Run().Please correct me if I'm wrong.
For the
gitcmd.Run(), we just need to add vars togitcmd.Env, without effecting the global env vars.EDIT: I realized this part of program does not stay running (and there is no concurrency) because it's compiled to a
servbinary and runs for each operation.But it's still cleaner this way and a better practice in general.