Skip to content

diff: release all handles before running external diff #213

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

dscho
Copy link
Member

@dscho dscho commented May 20, 2019

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).

@dscho dscho added the ready to submit Has commits that have not been submitted yet label May 20, 2019
@dscho
Copy link
Member Author

dscho commented Jul 4, 2019

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Jul 4, 2019

Submitted as [email protected]

@dscho dscho removed the ready to submit Has commits that have not been submitted yet label Jul 4, 2019
@gitgitgadget

This comment has been minimized.

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, 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 git-for-windows#1315

Signed-off-by: Johannes Schindelin <[email protected]>
@dscho dscho force-pushed the munmap-before-ext-diff branch from bef83fc to 8a02132 Compare July 10, 2019 12:44
@dscho
Copy link
Member Author

dscho commented Jul 11, 2019

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Jul 11, 2019

Submitted as [email protected]

@gitgitgadget
Copy link

gitgitgadget bot commented Jul 12, 2019

This branch is now known as js/unmap-before-ext-diff.

@gitgitgadget
Copy link

gitgitgadget bot commented Jul 12, 2019

This patch series was integrated into pu via git@2de7515.

@gitgitgadget gitgitgadget bot added the pu label Jul 12, 2019
@gitgitgadget
Copy link

gitgitgadget bot commented Jul 12, 2019

This patch series was integrated into pu via git@3779829.

@gitgitgadget
Copy link

gitgitgadget bot commented Jul 15, 2019

This patch series was integrated into pu via git@6116512.

@gitgitgadget
Copy link

gitgitgadget bot commented Jul 15, 2019

This patch series was integrated into pu via git@28c319b.

@gitgitgadget
Copy link

gitgitgadget bot commented Jul 15, 2019

This patch series was integrated into next via git@7aa292c.

@gitgitgadget gitgitgadget bot added the next label Jul 15, 2019
@gitgitgadget
Copy link

gitgitgadget bot commented Jul 19, 2019

This patch series was integrated into pu via git@b54ebd0.

@gitgitgadget
Copy link

gitgitgadget bot commented Jul 23, 2019

This patch series was integrated into pu via git@44d6127.

@gitgitgadget
Copy link

gitgitgadget bot commented Jul 25, 2019

This patch series was integrated into pu via git@d9beb46.

@gitgitgadget
Copy link

gitgitgadget bot commented Jul 25, 2019

This patch series was integrated into next via git@d9beb46.

@gitgitgadget
Copy link

gitgitgadget bot commented Jul 25, 2019

This patch series was integrated into master via git@d9beb46.

@gitgitgadget gitgitgadget bot added the master label Jul 25, 2019
@gitgitgadget gitgitgadget bot closed this Jul 25, 2019
@gitgitgadget
Copy link

gitgitgadget bot commented Jul 25, 2019

Closed via d9beb46.

@dscho dscho deleted the munmap-before-ext-diff branch July 30, 2019 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant