diff: release all handles before running external diff
Get series with b4
b4 am [email protected]Versions newest first · linked by author and title
- v2 Latest1 patch 1 replyIn review
- v11 patch 3 repliesSuperseded
Changes in v2 from the cover letter
* Clarified in the commit message that even mapped regions block writes/deletes.
* The diff file pair is now released unconditionally, not only when it is mapped, for consistency (the CI build did not fail, and a cursory inspection of the code paths indicates that this should be safe, as from this point on only the external command accesses the file pair's contents, and they had to be written out to disk to that end).
Patches in v2 No applies recorded yet
| # | Subject and files | Size | Replies | Applied |
|---|---|---|---|---|
| 1/1 | diff: munmap() file contents before running external diffdiff.c | +2 | 1 reply |
Reviews trailers per patch
No Reviewed-by, Acked-by or Tested-by trailers for v2 yet.
Cover letter [PATCH v2 0/1] diff: release all handles before running external diff
On Windows, it is not possible to overwrite a file as long as any process holds a read handle to it. Even keeping regions memory-mapped prevents that.
When git difftool calls git diff, it might be the user's intention to write the file(s) via the diff tool, so let's make sure that they are not memory-mapped at that stage.
Changes since v1:
* Clarified in the commit message that even mapped regions block writes/deletes.
* The diff file pair is now released unconditionally, not only when it is mapped, for consistency (the CI build did not fail, and a cursory inspection of the code paths indicates that this should be safe, as from this point on only the external command accesses the file pair's contents, and they had to be written out to disk to that end).
Johannes Schindelin (1):
diff: munmap() file contents before running external diff
diff.c | 2 ++ 1 file changed, 2 insertions(+)
base-commit: aa25c82427ae70aebf3b8f970f2afd54e9a2a8c6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-213%2Fdscho%2Fmunmap-before-ext-diff-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-213/dscho/munmap-before-ext-diff-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/213
Range-diff vs v1:
1: bef83fc20b ! 1: 8a0213291b diff: munmap() file contents before running external diff
@@ -5,11 +5,15 @@
When running an external diff from, say, a diff tool, it is safe to assume that we want to write the files in question. On Windows, that means that there cannot be any other process holding an open handle to
- said files.
+ said files, or even just a mapped region.
So let's make sure that git diff itself is not holding any open handle to the files in question.
+ In fact, we will just release the file pair right away, as the external
+ diff uses the files we just wrote, so we do not need to hold the file
+ contents in memory anymore.
+
This fixes https://github.com/git-for-windows/git/issues/1315
@@ -21,10 +25,8 @@
argv_array_pushf(&env, "GIT_DIFF_PATH_COUNTER=%d", ++o->diff_path_counter); argv_array_pushf(&env, "GIT_DIFF_PATH_TOTAL=%d", q->nr);
-+ if (one && one->should_munmap)
-+ diff_free_filespec_data(one);
-+ if (two && two->should_munmap)
-+ diff_free_filespec_data(two);
++ diff_free_filespec_data(one);
++ diff_free_filespec_data(two);
if (run_command_v_opt_cd_env(argv.argv, RUN_USING_SHELL, NULL, env.argv))
die(_("external diff died, stopping at %s"), name);
Signature · 1 line
gitgitgadget