git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: Junio C Hamano <gitster@pobox.com>
Subject: [PATCH v2 0/1] diff: release all handles before running external diff
Date: Thu, 11 Jul 2019 01:23:40 -0700 (PDT)	[thread overview]
Message-ID: <pull.213.v2.git.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.213.git.gitgitgadget@gmail.com>

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
      
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
     @@ -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);
       

-- 
gitgitgadget

  parent reply	other threads:[~2019-07-11  8:23 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-04  9:16 [PATCH 0/1] diff: release all handles before running external diff Johannes Schindelin via GitGitGadget
2019-07-04  9:16 ` [PATCH 1/1] diff: munmap() file contents " Johannes Schindelin via GitGitGadget
2019-07-08 21:54   ` Junio C Hamano
2019-07-10 12:43     ` Johannes Schindelin
2019-07-08 19:24 ` [PATCH 0/1] diff: release all handles " Junio C Hamano
2019-07-11  8:23 ` Johannes Schindelin via GitGitGadget [this message]
2019-07-11  8:23   ` [PATCH v2 1/1] diff: munmap() file contents " Johannes Schindelin via GitGitGadget
2019-07-11 19:03     ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=pull.213.v2.git.gitgitgadget@gmail.com \
    --to=gitgitgadget@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).