Save patches to temporary files #9296
Conversation
os.Rename cannot rename across disk boundaries
Codecov Report
@@ Coverage Diff @@
## master #9296 +/- ##
==========================================
- Coverage 41.19% 41.18% -0.01%
==========================================
Files 559 559
Lines 73073 73126 +53
==========================================
+ Hits 30099 30114 +15
- Misses 39205 39240 +35
- Partials 3769 3772 +3
Continue to review full report at Codecov.
|
| _ = os.Remove(tmpPatchFile.Name()) | ||
| }() | ||
|
|
||
| if err := headGitRepo.GetPatch(pr.MergeBase, pr.HeadBranch, tmpPatchFile); err != nil { |
There was a problem hiding this comment.
| if err := headGitRepo.GetPatch(pr.MergeBase, pr.HeadBranch, tmpPatchFile); err != nil { | |
| err = headGitRepo.GetPatch(pr.MergeBase, pr.HeadBranch, tmpPatchFile) | |
| tmpPatchFile.Close() | |
| if err != nil { |
| }() | ||
|
|
||
| if err := headGitRepo.GetPatch(pr.MergeBase, pr.HeadBranch, tmpPatchFile); err != nil { | ||
| tmpPatchFile.Close() |
There was a problem hiding this comment.
| tmpPatchFile.Close() |
| }() | ||
|
|
||
| if err := headGitRepo.GetPatch(prInfo.MergeBase, headBranch, tmpPatchFile); err != nil { | ||
| tmpPatchFile.Close() |
|
So is this obsolete or complementary regarding #9302 ? |
|
This one is simpler, the other is more of a refactor but also likely fixes the issue that spurred me to look at this. We could probably backport this one - if we think the efficiency is worth it. |
| if len(patch) > 0 { | ||
| if err = repo.savePatch(sess, pr.Index, patch); err != nil { | ||
| if patchFileSize > 0 { | ||
| if err = repo.savePatch(sess, pr.Index, patchFileName); err != nil { |
There was a problem hiding this comment.
I think the patch should be saved even if it's got 0 bytes. The reason is that a previous PR creation attempt (from a different PR, with patch data) could have failed between this step and the commit, so a left-over patch file (named after this same index number) might still be lingering in the directory. I hope that makes sense.
In fact, there are scenarios where that patch could be broken anyway with this retry mechanism. Mmm.... we should be locking that path, at the very least? Sorry, I wasn't aware that there were destinations other than the database when I wrote the retry code.
There was a problem hiding this comment.
It would make it simpler if we dropped that file size test.
Tbh I'm not certain that the patch files are ever actually cleaned up. I don't remember seeing any code looking at deleting them. I actually forgot to look explicitly - that's not like me!!
You're also right about the lack of file locking. This might be because we have generally migrated from the repoworkingcopy lock mechanism to rely on git locking but obviously this doesn't use that mechanism. #9302 obviously isn't affected by this.
I guess the question is how important is it provide a backport?
If we're not going to backport - then I'm happy to close this and move to the more radical refactor.
(It's not like this is the only place we still read in potentially huge amounts of data - diff and blame are both still doing that, and a change to streaming there will require huge refactoring.)
There was a problem hiding this comment.
Well, backporting is not my call, but I'm OK with not backporting this as long as #9302 makes it in time for 1.11.
|
I'm gonna close this. We can re-examine this technique if 1.10 has lots of problems with its current structure. |
This PR saves patches directly to a temporary file rather than read these directly into memory - this fixes a number of issues but not the issue that spurred me to look at this.
This could be backported to 1.10 as it doesn't make any significant changes to the code structure.