On Thu, Jun 22, 2023 at 5:33 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > When an external merge driver dies with a signal, we should not > expect that the result left on the filesystem is in any useful > state. However, because the current code uses the return value from > run_command() and declares any positive value as a sign that the > driver successfully left conflicts in the result, and because the > return value from run_command() for a subprocess that died upon a > signal is positive, we end up treating whatever garbage left on the > filesystem as the result the merge driver wanted to leave us. Yeah, I think the tradition was exit code == number of conflicts for some merge processes. Not particularly useful when the driver died from some signal. > run_command() returns larger than 128 (WTERMSIG(status) + 128, to be > exact) when it notices that the subprocess died with a signal, so > detect such a case and return LL_MERGE_ERROR from ll_ext_merge(). Makes sense. > > Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx> > --- > > * This time with an updated title, a minimal documentation, and an > additional test. > > Documentation/gitattributes.txt | 5 ++++- > ll-merge.c | 9 ++++++++- > t/t6406-merge-attr.sh | 23 +++++++++++++++++++++++ > 3 files changed, 35 insertions(+), 2 deletions(-) > > diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt > index 02a3ec83e4..6deb89a296 100644 > --- a/Documentation/gitattributes.txt > +++ b/Documentation/gitattributes.txt > @@ -1132,7 +1132,10 @@ size (see below). > The merge driver is expected to leave the result of the merge in > the file named with `%A` by overwriting it, and exit with zero > status if it managed to merge them cleanly, or non-zero if there > -were conflicts. > +were conflicts. When the driver crashes (e.g. killed by SEGV), > +it is expected to exit with non-zero status that are higher than > +128, and in such a case, the merge results in a failure (which is > +different from producing a conflict). Looks good. > The `merge.*.recursive` variable specifies what other merge > driver to use when the merge driver is called for an internal > diff --git a/ll-merge.c b/ll-merge.c > index 07ec16e8e5..ba45aa2f79 100644 > --- a/ll-merge.c > +++ b/ll-merge.c > @@ -243,7 +243,14 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn, > unlink_or_warn(temp[i]); > strbuf_release(&cmd); > strbuf_release(&path_sq); > - ret = (status > 0) ? LL_MERGE_CONFLICT : status; > + > + if (!status) > + ret = LL_MERGE_OK; > + else if (status <= 128) > + ret = LL_MERGE_CONFLICT; > + else > + /* died due to a signal: WTERMSIG(status) + 128 */ > + ret = LL_MERGE_ERROR; > return ret; > } Likewise. > diff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh > index 5e4e4dd6d9..b50aedbc00 100755 > --- a/t/t6406-merge-attr.sh > +++ b/t/t6406-merge-attr.sh > @@ -56,6 +56,12 @@ test_expect_success setup ' > ) >"$ours+" > cat "$ours+" >"$ours" > rm -f "$ours+" > + > + if test -f ./please-abort > + then > + echo >>./please-abort killing myself > + kill -9 $$ > + fi > exit "$exit" > EOF > chmod +x ./custom-merge > @@ -162,6 +168,23 @@ test_expect_success 'custom merge backend' ' > rm -f $o $a $b > ' > > +test_expect_success 'custom merge driver that is killed with a signal' ' > + test_when_finished "rm -f output please-abort" && > + > + git reset --hard anchor && > + git config --replace-all \ > + merge.custom.driver "./custom-merge %O %A %B 0 %P" && > + git config --replace-all \ > + merge.custom.name "custom merge driver for testing" && > + > + >./please-abort && > + echo "* merge=custom" >.gitattributes && > + test_must_fail git merge main && > + git ls-files -u >output && > + git diff --name-only HEAD >>output && > + test_must_be_empty output > +' > + I was about to comment that we needed to clean up the please-abort file, then realized I just missed it in my first reading of the test_when_finished line. So, patch looks good. Reviewed-by: Elijah Newren <newren@xxxxxxxxx>
