{"thread":{"id":"51428","subject":"[PATCH 0/1] diff: release all handles before running external diff","startedAt":"2019-07-04T09:16:25Z","lastAt":"2019-07-11T19:03:16Z","messageCount":8,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"378582","messageId":"pull.213.git.gitgitgadget@gmail.com","threadId":"51428","inReplyTo":null,"subject":"[PATCH 0/1] diff: release all handles before running external diff","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-07-04T09:16:22Z","receivedAt":"2019-07-04T09:16:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"On Windows, it is not possible to overwrite a file as long as any process\nholds a read handle to it. Even keeping regions memory-mapped prevents that.\n\nWhen git difftool calls git diff, it might be the user's intention to write\nthe file(s) via the diff tool, so let's make sure that they are not\nmemory-mapped at that stage.\n\nJohannes Schindelin (1):\n diff: munmap() file contents before running external diff\n\n diff.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\n\nbase-commit: aa25c82427ae70aebf3b8f970f2afd54e9a2a8c6\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-213%2Fdscho%2Fmunmap-before-ext-diff-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-213/dscho/munmap-before-ext-diff-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/213\n-- \ngitgitgadget\n"},{"id":"378583","messageId":"bef83fc20b9e2048872316e3f396da0cb6775086.1562231781.git.gitgitgadget@gmail.com","threadId":"51428","inReplyTo":"pull.213.git.gitgitgadget@gmail.com","subject":"[PATCH 1/1] diff: munmap() file contents before running external diff","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-07-04T09:16:22Z","receivedAt":"2019-07-04T09:16:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen running an external diff from, say, a diff tool, it is safe to\nassume that we want to write the files in question. On Windows, that\nmeans that there cannot be any other process holding an open handle to\nsaid files.\n\nSo let's make sure that `git diff` itself is not holding any open handle\nto the files in question.\n\nThis fixes https://github.com/git-for-windows/git/issues/1315\n\nSigned-off-by: Johannes Schindelin \n---\n diff.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/diff.c b/diff.c\nindex 4d3cf83a27..0afb76bbca 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4206,6 +4206,10 @@ static void run_external_diff(const char *pgm,\n \targv_array_pushf(&env, \"GIT_DIFF_PATH_COUNTER=%d\", ++o->diff_path_counter);\n \targv_array_pushf(&env, \"GIT_DIFF_PATH_TOTAL=%d\", q->nr);\n \n+\tif (one && one->should_munmap)\n+\t\tdiff_free_filespec_data(one);\n+\tif (two && two->should_munmap)\n+\t\tdiff_free_filespec_data(two);\n \tif (run_command_v_opt_cd_env(argv.argv, RUN_USING_SHELL, NULL, env.argv))\n \t\tdie(_(\"external diff died, stopping at %s\"), name);\n \n-- \ngitgitgadget\n"},{"id":"378701","messageId":"xmqqr270xrc5.fsf@gitster-ct.c.googlers.com","threadId":"51428","inReplyTo":"pull.213.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/1] diff: release all handles before running external diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-08T19:24:10Z","receivedAt":"2019-07-08T19:24:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> On Windows, it is not possible to overwrite a file as long as any process\n> holds a read handle to it. Even keeping regions memory-mapped prevents that.\n\nThat second sentence was quite helpful, as I do recall us closing\nafter mmapping. Without it, the justification becomes quite weak.\n\n\n"},{"id":"378709","messageId":"xmqq8st8xkdg.fsf@gitster-ct.c.googlers.com","threadId":"51428","inReplyTo":"bef83fc20b9e2048872316e3f396da0cb6775086.1562231781.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] diff: munmap() file contents before running external diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-08T21:54:35Z","receivedAt":"2019-07-08T21:54:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> When running an external diff from, say, a diff tool, it is safe to\n> assume that we want to write the files in question. On Windows, that\n> means that there cannot be any other process holding an open handle to\n> said files.\n\nPlease add \"It is not enough to close the file descriptor; having a\nregion that is still mmapped keeps the file busy\" or something like\nthat at the end.\n\n> So let's make sure that `git diff` itself is not holding any open handle\n> to the files in question.\n>\n> This fixes https://github.com/git-for-windows/git/issues/1315\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> diff.c | 4 ++++\n> 1 file changed, 4 insertions(+)\n>\n> diff --git a/diff.c b/diff.c\n> index 4d3cf83a27..0afb76bbca 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -4206,6 +4206,10 @@ static void run_external_diff(const char *pgm,\n> \targv_array_pushf(&env, \"GIT_DIFF_PATH_COUNTER=%d\", ++o->diff_path_counter);\n> \targv_array_pushf(&env, \"GIT_DIFF_PATH_TOTAL=%d\", q->nr);\n> \n> +\tif (one && one->should_munmap)\n> +\t\tdiff_free_filespec_data(one);\n> +\tif (two && two->should_munmap)\n> +\t\tdiff_free_filespec_data(two);\n\nI wondered if a single diff_filespec instance can be used in two\ndiff_filepair instances (e.g. file A is in-place modified and also\nused to create file C), and if so after showing the diff for file A,\nwe have problems with showing file C. But I do not think it should\npose a problem, as \"free data after comparing a pair\" is what we do\nfor the in-core codepath in builtin_diff().\n\nWe can lose the NULL-ness test for one and two if these \"free the\nresource once we no longer need it\" is done inside \"if (one && two)\".\nAfter all, once add_external_diff_name()[*1*] does its thing, we do\nnot need the data for these diff_filespec instances, right?\n\nAlso, just like builtin_diff() unconditionally frees the resources\nheld by diff_filespec instances, shouldn't this function do so, even\nthe ones that are not marked with should_munmap?\n\n\n> \tif (run_command_v_opt_cd_env(argv.argv, RUN_USING_SHELL, NULL, env.argv))\n> \t\tdie(_(\"external diff died, stopping at %s\"), name);\n"},{"id":"378761","messageId":"nycvar.QRO.7.76.6.1907101424300.46@tvgsbejvaqbjf.bet","threadId":"51428","inReplyTo":"xmqq8st8xkdg.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] diff: munmap() file contents before running external diff","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-07-10T12:43:21Z","receivedAt":"2019-07-10T12:43:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 8 Jul 2019, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > From: Johannes Schindelin \n> >\n> > When running an external diff from, say, a diff tool, it is safe to\n> > assume that we want to write the files in question. On Windows, that\n> > means that there cannot be any other process holding an open handle to\n> > said files.\n>\n> Please add \"It is not enough to close the file descriptor; having a\n> region that is still mmapped keeps the file busy\" or something like\n> that at the end.\n\nGood call.\n\n> > So let's make sure that `git diff` itself is not holding any open handle\n> > to the files in question.\n> >\n> > This fixes https://github.com/git-for-windows/git/issues/1315\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > diff.c | 4 ++++\n> > 1 file changed, 4 insertions(+)\n> >\n> > diff --git a/diff.c b/diff.c\n> > index 4d3cf83a27..0afb76bbca 100644\n> > --- a/diff.c\n> > +++ b/diff.c\n> > @@ -4206,6 +4206,10 @@ static void run_external_diff(const char *pgm,\n> > \targv_array_pushf(&env, \"GIT_DIFF_PATH_COUNTER=%d\", ++o->diff_path_counter);\n> > \targv_array_pushf(&env, \"GIT_DIFF_PATH_TOTAL=%d\", q->nr);\n> >\n> > +\tif (one && one->should_munmap)\n> > +\t\tdiff_free_filespec_data(one);\n> > +\tif (two && two->should_munmap)\n> > +\t\tdiff_free_filespec_data(two);\n>\n> I wondered if a single diff_filespec instance can be used in two\n> diff_filepair instances (e.g. file A is in-place modified and also\n> used to create file C), and if so after showing the diff for file A,\n> we have problems with showing file C. But I do not think it should\n> pose a problem, as \"free data after comparing a pair\" is what we do\n> for the in-core codepath in builtin_diff().\n\nPrecisely.\n\n> We can lose the NULL-ness test for one and two if these \"free the\n> resource once we no longer need it\" is done inside \"if (one && two)\".\n> After all, once add_external_diff_name()[*1*] does its thing, we do\n> not need the data for these diff_filespec instances, right?\n\nYes, but we still need the `should_munmap` test, I believe. So we have\nthat `if` anyway.\n\n> Also, just like builtin_diff() unconditionally frees the resources\n> held by diff_filespec instances, shouldn't this function do so, even\n> the ones that are not marked with should_munmap?\n\nI have not inspected the code path vigorously, nor am I confident that\nthis wouldn't be broken easily. At least when regions are mapped, I am\nfairly certain that my patch does not break anything.\n\nBut if you are confident that this won't break anything, I'll certainly\nbe happy with that assessment.\n\nI'll make that change and trust the CI build to fail if your assumption\nwas incorrect.\n\nCiao,\nDscho\n\n>\n>\n> > \tif (run_command_v_opt_cd_env(argv.argv, RUN_USING_SHELL, NULL, env.argv))\n> > \t\tdie(_(\"external diff died, stopping at %s\"), name);\n>\n"},{"id":"378798","messageId":"pull.213.v2.git.gitgitgadget@gmail.com","threadId":"51428","inReplyTo":"pull.213.git.gitgitgadget@gmail.com","subject":"[PATCH v2 0/1] diff: release all handles before running external diff","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-07-11T08:23:40Z","receivedAt":"2019-07-11T08:23:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"On Windows, it is not possible to overwrite a file as long as any process\nholds a read handle to it. Even keeping regions memory-mapped prevents that.\n\nWhen git difftool calls git diff, it might be the user's intention to write\nthe file(s) via the diff tool, so let's make sure that they are not\nmemory-mapped at that stage.\n\nChanges since v1:\n\n * Clarified in the commit message that even mapped regions block\n writes/deletes.\n * The diff file pair is now released unconditionally, not only when it is\n mapped, for consistency (the CI build did not fail, and a cursory\n inspection of the code paths indicates that this should be safe, as from\n this point on only the external command accesses the file pair's\n contents, and they had to be written out to disk to that end).\n\nJohannes Schindelin (1):\n diff: munmap() file contents before running external diff\n\n diff.c | 2 ++\n 1 file changed, 2 insertions(+)\n\n\nbase-commit: aa25c82427ae70aebf3b8f970f2afd54e9a2a8c6\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-213%2Fdscho%2Fmunmap-before-ext-diff-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-213/dscho/munmap-before-ext-diff-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/213\n\nRange-diff vs v1:\n\n 1: bef83fc20b ! 1: 8a0213291b diff: munmap() file contents before running external diff\n @@ -5,11 +5,15 @@\n When running an external diff from, say, a diff tool, it is safe to\n assume that we want to write the files in question. On Windows, that\n means that there cannot be any other process holding an open handle to\n - said files.\n + said files, or even just a mapped region.\n \n So let's make sure that `git diff` itself is not holding any open handle\n to the files in question.\n \n + In fact, we will just release the file pair right away, as the external\n + diff uses the files we just wrote, so we do not need to hold the file\n + contents in memory anymore.\n +\n This fixes https://github.com/git-for-windows/git/issues/1315\n \n Signed-off-by: Johannes Schindelin \n @@ -21,10 +25,8 @@\n \targv_array_pushf(&env, \"GIT_DIFF_PATH_COUNTER=%d\", ++o->diff_path_counter);\n \targv_array_pushf(&env, \"GIT_DIFF_PATH_TOTAL=%d\", q->nr);\n \n -+\tif (one && one->should_munmap)\n -+\t\tdiff_free_filespec_data(one);\n -+\tif (two && two->should_munmap)\n -+\t\tdiff_free_filespec_data(two);\n ++\tdiff_free_filespec_data(one);\n ++\tdiff_free_filespec_data(two);\n \tif (run_command_v_opt_cd_env(argv.argv, RUN_USING_SHELL, NULL, env.argv))\n \t\tdie(_(\"external diff died, stopping at %s\"), name);\n \n\n-- \ngitgitgadget\n"},{"id":"378799","messageId":"8a0213291b2afc8db9243808319dc86b736122c8.1562833419.git.gitgitgadget@gmail.com","threadId":"51428","inReplyTo":"pull.213.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 1/1] diff: munmap() file contents before running external diff","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-07-11T08:23:41Z","receivedAt":"2019-07-11T08:23:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen running an external diff from, say, a diff tool, it is safe to\nassume that we want to write the files in question. On Windows, that\nmeans that there cannot be any other process holding an open handle to\nsaid files, or even just a mapped region.\n\nSo let's make sure that `git diff` itself is not holding any open handle\nto the files in question.\n\nIn fact, we will just release the file pair right away, as the external\ndiff uses the files we just wrote, so we do not need to hold the file\ncontents in memory anymore.\n\nThis fixes https://github.com/git-for-windows/git/issues/1315\n\nSigned-off-by: Johannes Schindelin \n---\n diff.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/diff.c b/diff.c\nindex 4d3cf83a27..42affb6dcf 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4206,6 +4206,8 @@ static void run_external_diff(const char *pgm,\n \targv_array_pushf(&env, \"GIT_DIFF_PATH_COUNTER=%d\", ++o->diff_path_counter);\n \targv_array_pushf(&env, \"GIT_DIFF_PATH_TOTAL=%d\", q->nr);\n \n+\tdiff_free_filespec_data(one);\n+\tdiff_free_filespec_data(two);\n \tif (run_command_v_opt_cd_env(argv.argv, RUN_USING_SHELL, NULL, env.argv))\n \t\tdie(_(\"external diff died, stopping at %s\"), name);\n \n-- \ngitgitgadget\n"},{"id":"378856","messageId":"xmqq36jcs8b8.fsf@gitster-ct.c.googlers.com","threadId":"51428","inReplyTo":"8a0213291b2afc8db9243808319dc86b736122c8.1562833419.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/1] diff: munmap() file contents before running external diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-11T19:03:07Z","receivedAt":"2019-07-11T19:03:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> When running an external diff from, say, a diff tool, it is safe to\n> assume that we want to write the files in question. On Windows, that\n> means that there cannot be any other process holding an open handle to\n> said files, or even just a mapped region.\n>\n> So let's make sure that `git diff` itself is not holding any open handle\n> to the files in question.\n>\n> In fact, we will just release the file pair right away, as the external\n> diff uses the files we just wrote, so we do not need to hold the file\n> contents in memory anymore.\n>\n> This fixes https://github.com/git-for-windows/git/issues/1315\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> diff.c | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/diff.c b/diff.c\n> index 4d3cf83a27..42affb6dcf 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -4206,6 +4206,8 @@ static void run_external_diff(const char *pgm,\n> \targv_array_pushf(&env, \"GIT_DIFF_PATH_COUNTER=%d\", ++o->diff_path_counter);\n> \targv_array_pushf(&env, \"GIT_DIFF_PATH_TOTAL=%d\", q->nr);\n> \n> +\tdiff_free_filespec_data(one);\n> +\tdiff_free_filespec_data(two);\n> \tif (run_command_v_opt_cd_env(argv.argv, RUN_USING_SHELL, NULL, env.argv))\n> \t\tdie(_(\"external diff died, stopping at %s\"), name);\n\nLooks sensible; will queue. Thanks.\n"}]}