refactor: fix #722 properly #1250
Conversation
@L11r what's the point of copying? Can't we just pass |
|
@zricethezav It's not ideal, but much better than current implementation in my opinion. |
|
Probably another option is to introduce some client := git.NewClient(opts...)
diffs := client.Log()
defer client.Close()
go func() {
for diff := range diffs { /* handle findings */ }
}()
if client.Err() != nil {} |
|
@zricethezav have pushed second refactor. It should look much better now, I didn't use copy this time. |
|
I will send PR with tests for |
|
@zricethezav any comments? I know you are probably busy, but still think it's good to fix this issue properly. |
|
@L11r these changes look good to me. I'm just running some memory profiles to make sure nothing is exploding |
|
@zricethezav sure! |
|
solid PR, thanks for the contribution @L11r |
Description:
This PR fixes #722 issue properly.
Currently
gitpackage is poorly written in my humble opinion. Probably this is whygit_test.gocommented out entirely, it just hard to test because of it.Main problem:
*exec.Cmdrequires you to runStart()and thenWait()to release resources. But bothgit.GitDiff()andgit.GitLog()return chan with git diff files, later consumer handles chan elements and parser (!) callsWait()and closes stdout pipe. This is why you can't just defercmd.Wait()in these functions, because otherwise you will get blocked, consumer don't consume diff files at this moment yet.In order to fix this we need to merge this: gitleaks/go-gitdiff#6
We shouldn't get parser know about our implementation, it should just read from somethings, that's it.
Then we can use
bytes.Bufferto copy stdout and defer bothcmd.Wait()andstdout.Close()ingitpackage directly. Also for some reason currently we don't even closestderr, this PR also fixes it.To properly handle deferred
stderrErrI use named return variables.I understand that it slightly increases memory footprint, but I am sure it's worth it.
Checklist: