{"thread":{"id":"59897","subject":"Design issue in git merge driver interface","startedAt":"2023-06-22T16:50:13Z","lastAt":"2023-06-27T21:32:21Z","messageCount":15,"participants":["Joshua Hudson","Junio C Hamano","Elijah Newren","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"478692","messageId":"6e1b9ce4-e86d-fe30-e5de-27a3be57eefd@cedaron.com","threadId":"59897","inReplyTo":null,"subject":"Design issue in git merge driver interface","fromName":"Joshua Hudson","fromEmail":"jhudson@cedaron.com","sentAt":"2023-06-22T16:50:01Z","receivedAt":"2023-06-22T16:50:13Z","isPatch":false,"sender":{"key":"jhudson@cedaron.com","avatar":null},"body":"We had a major incident here involving bad merges. It was traced to a \nproblem with a custom merge driver.\n\nOn studying why the merge driver failed, we found it was in an \nunrunnable state (the logical equivalent of calling abort()) and could \nnot mark files as conflicted. Since the users were using a graphical \nfrontend the error messages disappeared into the bitbucket.\n\nI've come to the definite conclusion that treating the file as \nconflicted was wrong and the merge needed to bail out in the middle as \nthough git merge itself bombed. But there doesn't seem to be any way in \nthe protocol to actually do it.\n\nLooking at the merge driver found that some things cannot be handled, \nsuch as OOM condition. The fault has to propagate upwards, unwinding as \nit goes.\n\n"},{"id":"478694","messageId":"xmqqttuze2fh.fsf@gitster.g","threadId":"59897","inReplyTo":"6e1b9ce4-e86d-fe30-e5de-27a3be57eefd@cedaron.com","subject":"Re: Design issue in git merge driver interface","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-22T19:12:50Z","receivedAt":"2023-06-22T19:12:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joshua Hudson writes:\n\n> Looking at the merge driver found that some things cannot be handled,\n> such as OOM condition. The fault has to propagate upwards, unwinding\n> as it goes.\n\nEven though the end-user facing documentation says:\n\n The merge driver is expected to leave the result of the merge in\n the file named with `%A` by overwriting it, and exit with zero\n status if it managed to merge them cleanly, or non-zero if there\n were conflicts.\n\nthe ll-merge.c:ll_ext_merge() function that calls an external merge\ndriver does this:\n\n static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n ...\n status = run_command(&child);\n ...\n ret = (status > 0) ? LL_MERGE_CONFLICT : status;\n return ret;\n }\n\nso a true \"failure\" from run_command() to run the external merge\ndriver will be noticed as a failure by the upper layer of the\ncallchain. merge-ort.c:merge_3way() relays the return value of\nll-merge.c:ll_merge() and merge-ort.c:handle_content_merge() reacts\nto a negative return as an _(\"Failed to execute internal merge\")\nerror, for example. merge-recursive uses the same logic.\n\nUnfortunately, I see no provision for the merge driver to actively\nsignal such a condition. The return value of run_command() is a\nreturn value from run-command.c:wait_or_whine() and exit status of\nthe process is cleansed with WEXITSTATUS() so we cannot make it\nnegative X-<.\n\nIn the worst case, we may retroactively have to reserve one exit\nstatus so that the external merge driver can actively say \"I give\nup\" to cause LL_MERGE_ERROR to be returned from the codepath, but I\nwonder if it is safe to abuse \"exit due to signal\" (which shows up\nas a return value greater than 128) as such a \"merge driver went\naway without leaving a useful result\"? Elijah, what do you think?\n\nStepping back a bit and even disregarding such a merge driver that\nOOMs, if a long-running merge driver is killed, by definition we\ncannot trust what the driver left on the filesystem, so handling\n\"exit due to signal\" case differently does sound like a sensible\nthing to do, at least to me, offhand.\n\nAnd once we have such an enhancement to the ll-ext-merge interface,\na merge driver that voluntarily \"gives up\" can send a signal to kill\nitself (or call abort(3)).\n\nWith a tentative commit log message (which would need to be updated\nto mention what the triggering topic was that led to this\nenhancement) but without associated documentation update and test,\nhere is to summarize and illustrate the above idea.\n\n----- >8 ---------- >8 ---------- >8 -----\nll-merge: external merge driver died with a signal causes an error\n\nWhen an external merge driver dies with a signal, we should not\nexpect that the result left on the filesystem is in any useful\nstate. However, because the current code uses the return value from\nrun_command() and declares any positive value as a sign that the\ndriver successfully left conflicts in the result, and because the\nreturn value from run_command() for a subprocess that died upon a\nsignal is positive, we end up treating whatever garbage left on the\nfilesystem as the result the merge driver wanted to leave us.\n\nrun_command() returns larger than 128 (WTERMSIG(status) + 128, to be\nexact) when it notices that the subprocess died with a signal, so\ndetect such a case and return LL_MERGE_ERROR from ll_ext_merge().\n\nSigned-off-by: Junio C Hamano \n---\n ll-merge.c | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git c/ll-merge.c w/ll-merge.c\nindex 07ec16e8e5..5599f55ffc 100644\n--- c/ll-merge.c\n+++ w/ll-merge.c\n@@ -243,7 +243,14 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \t\tunlink_or_warn(temp[i]);\n \tstrbuf_release(&cmd);\n \tstrbuf_release(&path_sq);\n-\tret = (status > 0) ? LL_MERGE_CONFLICT : status;\n+\n+\tif (!status)\n+\t\tret = LL_MERGE_OK;\n+\telse if (status <= 128)\n+\t\tret = LL_MERGE_CONFLICT;\n+\telse\n+\t\t/* died due to a signal: WTERMSIG(status) + 128 */\n+\t\tret = LL_MERGE_ERROR;\n \treturn ret;\n }\n \n"},{"id":"478719","messageId":"xmqq4jmzc91e.fsf_-_@gitster.g","threadId":"59897","inReplyTo":"xmqqttuze2fh.fsf@gitster.g","subject":"[PATCH] ll-merge: killing the external merge driver aborts the merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-23T00:33:01Z","receivedAt":"2023-06-23T00:33:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When an external merge driver dies with a signal, we should not\nexpect that the result left on the filesystem is in any useful\nstate. However, because the current code uses the return value from\nrun_command() and declares any positive value as a sign that the\ndriver successfully left conflicts in the result, and because the\nreturn value from run_command() for a subprocess that died upon a\nsignal is positive, we end up treating whatever garbage left on the\nfilesystem as the result the merge driver wanted to leave us.\n\nrun_command() returns larger than 128 (WTERMSIG(status) + 128, to be\nexact) when it notices that the subprocess died with a signal, so\ndetect such a case and return LL_MERGE_ERROR from ll_ext_merge().\n\nSigned-off-by: Junio C Hamano \n---\n\n * This time with an updated title, a minimal documentation, and an\n additional test.\n\n Documentation/gitattributes.txt | 5 ++++-\n ll-merge.c | 9 ++++++++-\n t/t6406-merge-attr.sh | 23 +++++++++++++++++++++++\n 3 files changed, 35 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 02a3ec83e4..6deb89a296 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -1132,7 +1132,10 @@ size (see below).\n The merge driver is expected to leave the result of the merge in\n the file named with `%A` by overwriting it, and exit with zero\n status if it managed to merge them cleanly, or non-zero if there\n-were conflicts.\n+were conflicts. When the driver crashes (e.g. killed by SEGV),\n+it is expected to exit with non-zero status that are higher than\n+128, and in such a case, the merge results in a failure (which is\n+different from producing a conflict).\n \n The `merge.*.recursive` variable specifies what other merge\n driver to use when the merge driver is called for an internal\ndiff --git a/ll-merge.c b/ll-merge.c\nindex 07ec16e8e5..ba45aa2f79 100644\n--- a/ll-merge.c\n+++ b/ll-merge.c\n@@ -243,7 +243,14 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \t\tunlink_or_warn(temp[i]);\n \tstrbuf_release(&cmd);\n \tstrbuf_release(&path_sq);\n-\tret = (status > 0) ? LL_MERGE_CONFLICT : status;\n+\n+\tif (!status)\n+\t\tret = LL_MERGE_OK;\n+\telse if (status <= 128)\n+\t\tret = LL_MERGE_CONFLICT;\n+\telse\n+\t\t/* died due to a signal: WTERMSIG(status) + 128 */\n+\t\tret = LL_MERGE_ERROR;\n \treturn ret;\n }\n \ndiff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh\nindex 5e4e4dd6d9..b50aedbc00 100755\n--- a/t/t6406-merge-attr.sh\n+++ b/t/t6406-merge-attr.sh\n@@ -56,6 +56,12 @@ test_expect_success setup '\n \t) >\"$ours+\"\n \tcat \"$ours+\" >\"$ours\"\n \trm -f \"$ours+\"\n+\n+\tif test -f ./please-abort\n+\tthen\n+\t\techo >>./please-abort killing myself\n+\t\tkill -9 $$\n+\tfi\n \texit \"$exit\"\n \tEOF\n \tchmod +x ./custom-merge\n@@ -162,6 +168,23 @@ test_expect_success 'custom merge backend' '\n \trm -f $o $a $b\n '\n \n+test_expect_success 'custom merge driver that is killed with a signal' '\n+\ttest_when_finished \"rm -f output please-abort\" &&\n+\n+\tgit reset --hard anchor &&\n+\tgit config --replace-all \\\n+\tmerge.custom.driver \"./custom-merge %O %A %B 0 %P\" &&\n+\tgit config --replace-all \\\n+\tmerge.custom.name \"custom merge driver for testing\" &&\n+\n+\t>./please-abort &&\n+\techo \"* merge=custom\" >.gitattributes &&\n+\ttest_must_fail git merge main &&\n+\tgit ls-files -u >output &&\n+\tgit diff --name-only HEAD >>output &&\n+\ttest_must_be_empty output\n+'\n+\n test_expect_success 'up-to-date merge without common ancestor' '\n \tgit init repo1 &&\n \tgit init repo2 &&\n-- \n2.41.0-159-g0bfa463d37\n\n"},{"id":"478720","messageId":"CABPp-BG-KDu0fAC=bydz9A56xguSmgwO6SFDdxZ8h=90qR1PUA@mail.gmail.com","threadId":"59897","inReplyTo":"xmqq4jmzc91e.fsf_-_@gitster.g","subject":"Re: [PATCH] ll-merge: killing the external merge driver aborts the merge","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-06-23T06:25:40Z","receivedAt":"2023-06-23T06:25:59Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Jun 22, 2023 at 5:33 PM Junio C Hamano wrote:\n>\n> When an external merge driver dies with a signal, we should not\n> expect that the result left on the filesystem is in any useful\n> state. However, because the current code uses the return value from\n> run_command() and declares any positive value as a sign that the\n> driver successfully left conflicts in the result, and because the\n> return value from run_command() for a subprocess that died upon a\n> signal is positive, we end up treating whatever garbage left on the\n> filesystem as the result the merge driver wanted to leave us.\n\nYeah, I think the tradition was exit code == number of conflicts for\nsome merge processes. Not particularly useful when the driver died\nfrom some signal.\n\n> run_command() returns larger than 128 (WTERMSIG(status) + 128, to be\n> exact) when it notices that the subprocess died with a signal, so\n> detect such a case and return LL_MERGE_ERROR from ll_ext_merge().\n\nMakes sense.\n\n>\n> Signed-off-by: Junio C Hamano \n> ---\n>\n> * This time with an updated title, a minimal documentation, and an\n> additional test.\n>\n> Documentation/gitattributes.txt | 5 ++++-\n> ll-merge.c | 9 ++++++++-\n> t/t6406-merge-attr.sh | 23 +++++++++++++++++++++++\n> 3 files changed, 35 insertions(+), 2 deletions(-)\n>\n> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\n> index 02a3ec83e4..6deb89a296 100644\n> --- a/Documentation/gitattributes.txt\n> +++ b/Documentation/gitattributes.txt\n> @@ -1132,7 +1132,10 @@ size (see below).\n> The merge driver is expected to leave the result of the merge in\n> the file named with `%A` by overwriting it, and exit with zero\n> status if it managed to merge them cleanly, or non-zero if there\n> -were conflicts.\n> +were conflicts. When the driver crashes (e.g. killed by SEGV),\n> +it is expected to exit with non-zero status that are higher than\n> +128, and in such a case, the merge results in a failure (which is\n> +different from producing a conflict).\n\nLooks good.\n\n> The `merge.*.recursive` variable specifies what other merge\n> driver to use when the merge driver is called for an internal\n> diff --git a/ll-merge.c b/ll-merge.c\n> index 07ec16e8e5..ba45aa2f79 100644\n> --- a/ll-merge.c\n> +++ b/ll-merge.c\n> @@ -243,7 +243,14 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n> unlink_or_warn(temp[i]);\n> strbuf_release(&cmd);\n> strbuf_release(&path_sq);\n> - ret = (status > 0) ? LL_MERGE_CONFLICT : status;\n> +\n> + if (!status)\n> + ret = LL_MERGE_OK;\n> + else if (status <= 128)\n> + ret = LL_MERGE_CONFLICT;\n> + else\n> + /* died due to a signal: WTERMSIG(status) + 128 */\n> + ret = LL_MERGE_ERROR;\n> return ret;\n> }\n\nLikewise.\n\n> diff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh\n> index 5e4e4dd6d9..b50aedbc00 100755\n> --- a/t/t6406-merge-attr.sh\n> +++ b/t/t6406-merge-attr.sh\n> @@ -56,6 +56,12 @@ test_expect_success setup '\n> ) >\"$ours+\"\n> cat \"$ours+\" >\"$ours\"\n> rm -f \"$ours+\"\n> +\n> + if test -f ./please-abort\n> + then\n> + echo >>./please-abort killing myself\n> + kill -9 $$\n> + fi\n> exit \"$exit\"\n> EOF\n> chmod +x ./custom-merge\n> @@ -162,6 +168,23 @@ test_expect_success 'custom merge backend' '\n> rm -f $o $a $b\n> '\n>\n> +test_expect_success 'custom merge driver that is killed with a signal' '\n> + test_when_finished \"rm -f output please-abort\" &&\n> +\n> + git reset --hard anchor &&\n> + git config --replace-all \\\n> + merge.custom.driver \"./custom-merge %O %A %B 0 %P\" &&\n> + git config --replace-all \\\n> + merge.custom.name \"custom merge driver for testing\" &&\n> +\n> + >./please-abort &&\n> + echo \"* merge=custom\" >.gitattributes &&\n> + test_must_fail git merge main &&\n> + git ls-files -u >output &&\n> + git diff --name-only HEAD >>output &&\n> + test_must_be_empty output\n> +'\n> +\n\nI was about to comment that we needed to clean up the please-abort\nfile, then realized I just missed it in my first reading of the\ntest_when_finished line. So, patch looks good.\n\nReviewed-by: Elijah Newren \n"},{"id":"478734","messageId":"xmqqv8feb0vo.fsf@gitster.g","threadId":"59897","inReplyTo":"CABPp-BG-KDu0fAC=bydz9A56xguSmgwO6SFDdxZ8h=90qR1PUA@mail.gmail.com","subject":"Re: [PATCH] ll-merge: killing the external merge driver aborts the merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-23T16:26:51Z","receivedAt":"2023-06-23T16:26:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> Reviewed-by: Elijah Newren \n\n\nThanks for a quick review.\n"},{"id":"478757","messageId":"xmqqjzvt92nw.fsf@gitster.g","threadId":"59897","inReplyTo":"xmqqv8feb0vo.fsf@gitster.g","subject":"Re: [PATCH] ll-merge: killing the external merge driver aborts the merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-23T23:31:15Z","receivedAt":"2023-06-23T23:31:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Elijah Newren writes:\n>\n>> Reviewed-by: Elijah Newren \n>\n>\n> Thanks for a quick review.\n\nUnfortunately Windows does not seem to correctly detect the aborting\nmerge driver. Does run_command() there report process death due to\nsignals differently, I wonder?\n\nhttps://github.com/git/git/actions/runs/5360400800/jobs/9725341775#step:6:285\n\nshows that on Windows, aborted external merge driver is not noticed\nand we happily take the auto-merged result, ouch.\n\nI am tempted to protect this step of the test with a prerequisite to\nskip it on Windows for now. Anybody with better idea?\n\nThanks.\n"},{"id":"478784","messageId":"ac9b1ccb-82ba-4046-dd2a-e10b38a528a3@cedaron.com","threadId":"59897","inReplyTo":"xmqqjzvt92nw.fsf@gitster.g","subject":"Re: [PATCH] ll-merge: killing the external merge driver aborts the merge","fromName":"Joshua Hudson","fromEmail":"jhudson@cedaron.com","sentAt":"2023-06-25T13:40:57Z","receivedAt":"2023-06-25T13:41:13Z","isPatch":true,"sender":{"key":"jhudson@cedaron.com","avatar":null},"body":"On 6/23/2023 4:31 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> Elijah Newren writes:\n>>\n>>> Reviewed-by: Elijah Newren \n>>\n>> Thanks for a quick review.\n> Unfortunately Windows does not seem to correctly detect the aborting\n> merge driver. Does run_command() there report process death due to\n> signals differently, I wonder?\n>\n> https://github.com/git/git/actions/runs/5360400800/jobs/9725341775#step:6:285\n>\n> shows that on Windows, aborted external merge driver is not noticed\n> and we happily take the auto-merged result, ouch.\n>\n> I am tempted to protect this step of the test with a prerequisite to\n> skip it on Windows for now. Anybody with better idea?\n>\n> Thanks.\n\nI would suggest putting in the correct test harness on Windows. abort() \ndoesn't work very well.\n\n(Sample code only--read description below)\n\n#ifdef WIN32\n     TerminateProcess(GetCurrentProcess(), 131); /* something that looks \nlike it passes the test */\n     TerminateProcess(GetCurrentProcess(), 0x80070485); /* actual exit \ncode for process that cannot start because its missing a shared library */\n#else\n     abort();\n#endif\n\nI only want to fight so hard with the Unix to Windows translation layer \nyou use. Strictly speaking, the second TerminateProcess() line is what \nshould be in the test harness, but if it doesn't work go with the first \none. Then I at least have something to work with.\n\nI'm not going to lie to you. We are doing development on Windows, and \nthe merge driver is written using a different portability layer. I am \nprepared to build a native shim to make it work if I have to. I am not \nin a position where I can build git and test any development code; \ngetting the fix to us will be a long journey.\n\n"},{"id":"478879","messageId":"59b7a582-be68-3f7b-a06f-3bd662582a1d@gmx.de","threadId":"59897","inReplyTo":"xmqqjzvt92nw.fsf@gitster.g","subject":"Re: [PATCH] ll-merge: killing the external merge driver aborts the merge","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2023-06-27T12:02:02Z","receivedAt":"2023-06-27T12:02:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\n\nOn Fri, 23 Jun 2023, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n>\n> > Elijah Newren writes:\n> >\n> >> Reviewed-by: Elijah Newren \n> >\n> >\n> > Thanks for a quick review.\n>\n> Unfortunately Windows does not seem to correctly detect the aborting\n> merge driver. Does run_command() there report process death due to\n> signals differently, I wonder?\n>\n> https://github.com/git/git/actions/runs/5360400800/jobs/9725341775#step:6:285\n>\n> shows that on Windows, aborted external merge driver is not noticed\n> and we happily take the auto-merged result, ouch.\n\nHmm. I tried to verify this, but failed. With this patch:\n\n```diff\ndiff --git a/git.c b/git.c\nindex 2f42da20f4e0..3c513e3f2cb1 100644\n--- a/git.c\n+++ b/git.c\n@@ -330,6 +330,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\tsetenv(GIT_ATTR_SOURCE_ENVIRONMENT, cmd, 1);\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n+\t\t} else if (!strcmp(cmd, \"--abort\")) {\n+\t\t\tabort();\n \t\t} else {\n \t\t\tfprintf(stderr, _(\"unknown option: %s\\n\"), cmd);\n \t\t\tusage(git_usage_string);\n```\n\nI get this:\n\n\n```console\n$ ./git.exe --abort\n\n$ echo $?\n3\n```\n\nFor that reason, I am somehow doubtful that the `abort()` is actually\ncalled?!?\n\nCiao,\nJohannes\n"},{"id":"478882","messageId":"45ca0983-a0b6-1372-b548-2e0351988587@cedaron.com","threadId":"59897","inReplyTo":"59b7a582-be68-3f7b-a06f-3bd662582a1d@gmx.de","subject":"Re: [PATCH] ll-merge: killing the external merge driver aborts the merge","fromName":"Joshua Hudson","fromEmail":"jhudson@cedaron.com","sentAt":"2023-06-27T13:43:38Z","receivedAt":"2023-06-27T13:43:49Z","isPatch":true,"sender":{"key":"jhudson@cedaron.com","avatar":null},"body":"\nOn 6/27/2023 5:02 AM, Johannes Schindelin wrote:\n> Hi,\n>\n>\n> On Fri, 23 Jun 2023, Junio C Hamano wrote:\n>\n>> Junio C Hamano writes:\n>>\n>>> Elijah Newren writes:\n>>>\n>>>> Reviewed-by: Elijah Newren \n>>>\n>>> Thanks for a quick review.\n>> Unfortunately Windows does not seem to correctly detect the aborting\n>> merge driver. Does run_command() there report process death due to\n>> signals differently, I wonder?\n>>\n>> https://github.com/git/git/actions/runs/5360400800/jobs/9725341775#step:6:285\n>>\n>> shows that on Windows, aborted external merge driver is not noticed\n>> and we happily take the auto-merged result, ouch.\n> Hmm. I tried to verify this, but failed. With this patch:\n>\n> ```diff\n> diff --git a/git.c b/git.c\n> index 2f42da20f4e0..3c513e3f2cb1 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -330,6 +330,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n> \t\t\tsetenv(GIT_ATTR_SOURCE_ENVIRONMENT, cmd, 1);\n> \t\t\tif (envchanged)\n> \t\t\t\t*envchanged = 1;\n> +\t\t} else if (!strcmp(cmd, \"--abort\")) {\n> +\t\t\tabort();\n> \t\t} else {\n> \t\t\tfprintf(stderr, _(\"unknown option: %s\\n\"), cmd);\n> \t\t\tusage(git_usage_string);\n> ```\n>\n> I get this:\n>\n>\n> ```console\n> $ ./git.exe --abort\n>\n> $ echo $?\n> 3\n> ```\n>\n> For that reason, I am somehow doubtful that the `abort()` is actually\n> called?!?\n>\n> Ciao,\n> Johannes\n\nabort(); does _exit(3); on Windows because there are no signals. This is \neasily changed by providing abort like so:\n\nvoid abort() { _exit(131 /* or whatever else you think goes here */); }\n\n"},{"id":"478916","messageId":"xmqqedlwhgf9.fsf@gitster.g","threadId":"59897","inReplyTo":"59b7a582-be68-3f7b-a06f-3bd662582a1d@gmx.de","subject":"Re: [PATCH] ll-merge: killing the external merge driver aborts the merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-27T19:08:10Z","receivedAt":"2023-06-27T19:08:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Fri, 23 Jun 2023, Junio C Hamano wrote:\n>\n>> Junio C Hamano writes:\n>>\n>> > Elijah Newren writes:\n>> >\n>> >> Reviewed-by: Elijah Newren \n>> >\n>> >\n>> > Thanks for a quick review.\n>>\n>> Unfortunately Windows does not seem to correctly detect the aborting\n\nSorry, I did not mean \"abort(3)\" literally. What I meant was that\nan external merge driver that gets spawned via the run_command()\ninterface may not die by calling exit()---like \"killed by signal\"\n(including \"segfaulting\"). The new test script piece added in the\npatch did \"kill -9 $$\" to kill the external merge driver itself,\nwhich gets reported as \"killed by signal\" from run_command() by\nreturning the signal number + 128, but that did not pass Windows CI.\n\n"},{"id":"478917","messageId":"4f28a9c4-b422-69b7-ccc1-2661d756d876@cedaron.com","threadId":"59897","inReplyTo":"xmqqedlwhgf9.fsf@gitster.g","subject":"Re: [PATCH] ll-merge: killing the external merge driver aborts the merge","fromName":"Joshua Hudson","fromEmail":"jhudson@cedaron.com","sentAt":"2023-06-27T19:10:36Z","receivedAt":"2023-06-27T19:10:50Z","isPatch":true,"sender":{"key":"jhudson@cedaron.com","avatar":null},"body":"\nOn 6/27/2023 12:08 PM, Junio C Hamano wrote:\n> Johannes Schindelin writes:\n>\n>> On Fri, 23 Jun 2023, Junio C Hamano wrote:\n>>\n>>> Junio C Hamano writes:\n>>>\n>>>> Elijah Newren writes:\n>>>>\n>>>>> Reviewed-by: Elijah Newren \n>>>>\n>>>> Thanks for a quick review.\n>>> Unfortunately Windows does not seem to correctly detect the aborting\n> Sorry, I did not mean \"abort(3)\" literally. What I meant was that\n> an external merge driver that gets spawned via the run_command()\n> interface may not die by calling exit()---like \"killed by signal\"\n> (including \"segfaulting\"). The new test script piece added in the\n> patch did \"kill -9 $$\" to kill the external merge driver itself,\n> which gets reported as \"killed by signal\" from run_command() by\n> returning the signal number + 128, but that did not pass Windows CI.\n>\nDo you need me to provide a windows test harness?\n"},{"id":"478929","messageId":"xmqqttusfz9b.fsf@gitster.g","threadId":"59897","inReplyTo":"4f28a9c4-b422-69b7-ccc1-2661d756d876@cedaron.com","subject":"Re: [PATCH] ll-merge: killing the external merge driver aborts the merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-27T20:04:16Z","receivedAt":"2023-06-27T20:04:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joshua Hudson writes:\n\n> On 6/27/2023 12:08 PM, Junio C Hamano wrote:\n>> Johannes Schindelin writes:\n>>\n>>> On Fri, 23 Jun 2023, Junio C Hamano wrote:\n>>>\n>>>> Junio C Hamano writes:\n>>>>\n>>>>> Elijah Newren writes:\n>>>>>\n>>>>>> Reviewed-by: Elijah Newren \n>>>>>\n>>>>> Thanks for a quick review.\n>>>> Unfortunately Windows does not seem to correctly detect the aborting\n>> Sorry, I did not mean \"abort(3)\" literally. What I meant was that\n>> an external merge driver that gets spawned via the run_command()\n>> interface may not die by calling exit()---like \"killed by signal\"\n>> (including \"segfaulting\"). The new test script piece added in the\n>> patch did \"kill -9 $$\" to kill the external merge driver itself,\n>> which gets reported as \"killed by signal\" from run_command() by\n>> returning the signal number + 128, but that did not pass Windows CI.\n>>\n> Do you need me to provide a windows test harness?\n\nSorry, I do not understand the question.\n\nFWIW how \"external merge driver that kills itself by sending a\nsignal to itself does not get noticed on Windows\" appears in our\ntests can be seen at\n\nhttps://github.com/git/git/actions/runs/5360824580/jobs/9727137272\n\nThe job is \"win test(0)\", part of our standard Windows test harness\nimplemented as part of our GitHub Actions CI test.\n\nThanks.\n"},{"id":"478933","messageId":"cd5046d4-d66c-9dac-87e7-cdb638124170@cedaron.com","threadId":"59897","inReplyTo":"xmqqttusfz9b.fsf@gitster.g","subject":"Re: [PATCH] ll-merge: killing the external merge driver aborts the merge","fromName":"Joshua Hudson","fromEmail":"jhudson@cedaron.com","sentAt":"2023-06-27T21:14:31Z","receivedAt":"2023-06-27T21:14:45Z","isPatch":true,"sender":{"key":"jhudson@cedaron.com","avatar":null},"body":"\nOn 6/27/2023 1:04 PM, Junio C Hamano wrote:\n> Joshua Hudson writes:\n>\n>> On 6/27/2023 12:08 PM, Junio C Hamano wrote:\n>>> Johannes Schindelin writes:\n>>>\n>>>> On Fri, 23 Jun 2023, Junio C Hamano wrote:\n>>>>\n>>>>> Junio C Hamano writes:\n>>>>>\n>>>>>> Elijah Newren writes:\n>>>>>>\n>>>>>>> Reviewed-by: Elijah Newren \n>>>>>> Thanks for a quick review.\n>>>>> Unfortunately Windows does not seem to correctly detect the aborting\n>>> Sorry, I did not mean \"abort(3)\" literally. What I meant was that\n>>> an external merge driver that gets spawned via the run_command()\n>>> interface may not die by calling exit()---like \"killed by signal\"\n>>> (including \"segfaulting\"). The new test script piece added in the\n>>> patch did \"kill -9 $$\" to kill the external merge driver itself,\n>>> which gets reported as \"killed by signal\" from run_command() by\n>>> returning the signal number + 128, but that did not pass Windows CI.\n>>>\n>> Do you need me to provide a windows test harness?\n> Sorry, I do not understand the question.\n>\n> FWIW how \"external merge driver that kills itself by sending a\n> signal to itself does not get noticed on Windows\" appears in our\n> tests can be seen at\n>\n> https://github.com/git/git/actions/runs/5360824580/jobs/9727137272\n>\n> The job is \"win test(0)\", part of our standard Windows test harness\n> implemented as part of our GitHub Actions CI test.\n>\n> Thanks.\nTry changing kill -9 $$ to exit 137 # 128 + 9\n"},{"id":"478934","messageId":"xmqqilb8fvgu.fsf@gitster.g","threadId":"59897","inReplyTo":"cd5046d4-d66c-9dac-87e7-cdb638124170@cedaron.com","subject":"Re: [PATCH] ll-merge: killing the external merge driver aborts the merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-27T21:26:09Z","receivedAt":"2023-06-27T21:26:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joshua Hudson writes:\n\n> Try changing kill -9 $$ to exit 137 # 128 + 9\n\nYeah, but then (1) we are not simulating a case where the external\nmerge driver hits a segfault or receives a signal from outside and\ndies involuntarily, and (2) we are codifying that even on Windows,\nprogram that was killed by signal N must exit with 128 + N, and\nthese are the reasons why I did not go that route.\n\nStepping back a bit, how does one typically diagnose programatically\non Windows, after \"spawning\" a separate program, if the program died\ninvoluntarily and/or got killed? It does not have to be \"exit with\n128 + signal number\"---as long as it can be done programatically and\nreliably, we would be happy. The code to diagnose how the spawned\nprogram exited in run_command(), which is in finish_command() and\nthen in wait_or_whine(), may have to be updated with such a piece of\nWindows specific knowledge.\n"},{"id":"478935","messageId":"e7e5809d-5405-1e0b-f4e7-2d97e6748a09@cedaron.com","threadId":"59897","inReplyTo":"xmqqilb8fvgu.fsf@gitster.g","subject":"Re: [PATCH] ll-merge: killing the external merge driver aborts the merge","fromName":"Joshua Hudson","fromEmail":"jhudson@cedaron.com","sentAt":"2023-06-27T21:32:13Z","receivedAt":"2023-06-27T21:32:21Z","isPatch":true,"sender":{"key":"jhudson@cedaron.com","avatar":null},"body":"On 6/27/2023 2:26 PM, Junio C Hamano wrote:\n> Joshua Hudson writes:\n>\n>> Try changing kill -9 $$ to exit 137 # 128 + 9\n> Yeah, but then (1) we are not simulating a case where the external\n> merge driver hits a segfault or receives a signal from outside and\n> dies involuntarily, and (2) we are codifying that even on Windows,\n> program that was killed by signal N must exit with 128 + N, and\n> these are the reasons why I did not go that route.\n>\n> Stepping back a bit, how does one typically diagnose programatically\n> on Windows, after \"spawning\" a separate program, if the program died\n> involuntarily and/or got killed? It does not have to be \"exit with\n> 128 + signal number\"---as long as it can be done programatically and\n> reliably, we would be happy. The code to diagnose how the spawned\n> program exited in run_command(), which is in finish_command() and\n> then in wait_or_whine(), may have to be updated with such a piece of\n> Windows specific knowledge.\n\nabort() => 3\n\nKilled => no you can't detect it\n\nFaulted => exit code has the high bit set ( >= 0x8000000 )\n\nMy starting off with \"the logical equivalent of calling abort()\" has \nproven to be an unfortunate word choice. I need to harden up the exit \npathway on my side _anyway_. An OOM at least does turn into Faulted.\n\n"}]}