Use showFileAtRefBase64 to read per-commit file contents#3744
Use showFileAtRefBase64 to read per-commit file contents#3744peter-evans merged 7 commits intopeter-evans:mainfrom DeterminateSystems:showFileAtRef
Conversation
|
Thanks @gustavderdrache (note: a colleague of mine) for the code review and fixups! |
This comment was marked as outdated.
This comment was marked as outdated.
| const stdout: string[] = [] | ||
| const stderr: string[] = [] | ||
| const stdout: Buffer[] = [] | ||
| let stdoutLength = 0 | ||
| const stderr: Buffer[] = [] | ||
| let stderrLength = 0 | ||
|
|
||
| const options = { | ||
| cwd: this.workingDirectory, | ||
| env, | ||
| ignoreReturnCode: allowAllExitCodes, | ||
| listeners: { | ||
| stdout: (data: Buffer) => { | ||
| stdout.push(data.toString()) | ||
| stdout.push(data) | ||
| stdoutLength += data.length | ||
| }, | ||
| stderr: (data: Buffer) => { | ||
| stderr.push(data.toString()) | ||
| stderr.push(data) | ||
| stderrLength += data.length | ||
| } | ||
| } | ||
| } | ||
|
|
||
| result.exitCode = await exec.exec(`"${this.gitPath}"`, args, options) | ||
| result.stdout = stdout.join('') | ||
| result.stderr = stderr.join('') | ||
| result.stdout = Buffer.concat(stdout, stdoutLength).toString(encoding) | ||
| result.stderr = Buffer.concat(stderr, stderrLength).toString(encoding) |
There was a problem hiding this comment.
Note these changes are necessary to avoid accidentally incorrectly handling split chunks. In utf-8 this is a problem if the chunk is in the middle of a code point, corrupting the data. In base64 this is a problem because it can end up with padding in the middle of the stream, which effectively truncates the file early.
Reading it all into a single buffer and then converting retains the correct behavior, without letting the executing child process block on a full output stream.
|
@grahamc @gustavderdrache Thank you both for your effort to fix this! ❤️ The fix looks good to me. Not super happy about having to call git commands in the I have a test suite that includes a large file and 1000 small files, so let's see what the performance is like. |
|
/test repository=DeterminateSystems/create-pull-request ref=showFileAtRef sign-commits=true |
|
/test repository=DeterminateSystems/create-pull-request ref=showFileAtRef sign-commits=true perf=true |
|
If you can, it would be helpful if you could you run |
|
All set, thank you @peter-evans! |
|
Thank you again, @peter-evans! |
3 participants
Closes #3743