[PATCH 0/7] Fix and improve some error codepaths in merge-ort

[PATCH 0/7] Fix and improve some error codepaths in merge-ort

[Elijah Newren via GitGitGadget]

This series started as a just a fix for the abort hit in merge-ort when
custom merge drivers error out (see
https://lore.kernel.org/git/75F8BD12-7743-4863-B4C5-049FDEC4645E@gearset.com/).
However, while working on that, I found a few other issues around error
codepaths in merge-ort. So this series:

 * Patches 1-2: fix the reported abort problem
 * Patches 3-4: make code in handle_content_merges() easier to handle when
   we hit errors
 * Patch 5: fix a misleading comment
 * Patches 6-7: make error handling (immediate print vs. letting callers get
   the error information) more consistent

The last two patches change the behavior slightly for error codepaths, and
there's a question about whether we should show only the error messages that
caused an early termination of the merge, or if we should also show any
conflict messages for other paths that were handled before we hit the early
termination. These patches made a decision but feel free to take those last
two patches as more of an RFC.

Elijah Newren (7):
  merge-ort: extract handling of priv member into reusable function
  merge-ort: maintain expected invariant for priv member
  merge-ort: fix type of local 'clean' var in handle_content_merge()
  merge-ort: clearer propagation of failure-to-function from
    merge_submodule
  merge-ort: loosen commented requirements
  merge-ort: upon merge abort, only show messages causing the abort
  merge-ort: convert more error() cases to path_msg()

 merge-ort.c           | 167 +++++++++++++++++++++++++++++++-----------
 t/t6406-merge-attr.sh |  42 ++++++++++-
 2 files changed, 164 insertions(+), 45 deletions(-)


base-commit: 8d94cfb54504f2ec9edc7ca3eb5c29a3dd3675ae
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1748%2Fnewren%2Ffix-error-cases-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1748/newren/fix-error-cases-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1748
-- 
gitgitgadget

[PATCH 1/7] merge-ort: extract handling of priv member into reusable function

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

In preparation for a subsequent commit which will ensure we do not
forget to maintain our invariants for the priv member in error
codepaths, extract the necessary functionality out into a separate
function.  This change is cosmetic at this point, and introduces no
changes beyond an extra assertion sanity check.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-ort.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/merge-ort.c b/merge-ort.c
index eaede6cead9..10f5a655f29 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -5000,6 +5000,28 @@ static void merge_check_renames_reusable(struct merge_result *result,
 
 /*** Function Grouping: merge_incore_*() and their internal variants ***/
 
+static void move_opt_priv_to_result_priv(struct merge_options *opt,
+					 struct merge_result *result)
+{
+	/*
+	 * opt->priv and result->priv are a bit weird.  opt->priv contains
+	 * information that we can re-use in subsequent merge operations to
+	 * enable our cached renames optimization.  The best way to provide
+	 * that to subsequent merges is putting it in result->priv.
+	 * However, putting it directly there would mean retrofitting lots
+	 * of functions in this file to also take a merge_result pointer,
+	 * which is ugly and annoying.  So, we just make sure at the end of
+	 * the merge (the outer merge if there are internal recursive ones)
+	 * to move it.
+	 */
+	assert(opt->priv && !result->priv);
+	if (!opt->priv->call_depth) {
+		result->priv = opt->priv;
+		result->_properly_initialized = RESULT_INITIALIZED;
+		opt->priv = NULL;
+	}
+}
+
 /*
  * Originally from merge_trees_internal(); heavily adapted, though.
  */
@@ -5060,11 +5082,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,
 		/* existence of conflicted entries implies unclean */
 		result->clean &= strmap_empty(&opt->priv->conflicted);
 	}
-	if (!opt->priv->call_depth) {
-		result->priv = opt->priv;
-		result->_properly_initialized = RESULT_INITIALIZED;
-		opt->priv = NULL;
-	}
+	move_opt_priv_to_result_priv(opt, result);
 }
 
 /*
-- 
gitgitgadget

Re: [PATCH 1/7] merge-ort: extract handling of priv member into reusable function

[Junio C Hamano]

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +static void move_opt_priv_to_result_priv(struct merge_options *opt,
> +					 struct merge_result *result)
> +{
> +	/*
> +	 * opt->priv and result->priv are a bit weird.  opt->priv contains
> +	 * information that we can re-use in subsequent merge operations to
> +	 * enable our cached renames optimization.  The best way to provide
> +	 * that to subsequent merges is putting it in result->priv.
> +	 * However, putting it directly there would mean retrofitting lots
> +	 * of functions in this file to also take a merge_result pointer,
> +	 * which is ugly and annoying.  So, we just make sure at the end of
> +	 * the merge (the outer merge if there are internal recursive ones)
> +	 * to move it.
> +	 */
> +	assert(opt->priv && !result->priv);
> +	if (!opt->priv->call_depth) {
> +		result->priv = opt->priv;
> +		result->_properly_initialized = RESULT_INITIALIZED;
> +		opt->priv = NULL;
> +	}
> +}
> +
>  /*
>   * Originally from merge_trees_internal(); heavily adapted, though.
>   */
> @@ -5060,11 +5082,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,
>  		/* existence of conflicted entries implies unclean */
>  		result->clean &= strmap_empty(&opt->priv->conflicted);
>  	}
> -	if (!opt->priv->call_depth) {
> -		result->priv = opt->priv;
> -		result->_properly_initialized = RESULT_INITIALIZED;
> -		opt->priv = NULL;
> -	}
> +	move_opt_priv_to_result_priv(opt, result);
>  }

I have a feeling that making it the caller's responsibility to check
"are we doing the outermost merge?"  and not the callee's problem
would result in a better code organization.  If we write

	if (!opt->priv->call_depth)
		move_opt_priv_to_result_priv(opt, result);

then for this call site, it is still crystal clear that this will
happen only at the outermost level.  The new caller you add in the
next step would also be simpler to reason about.

You have the assert() to make sure callers do not call the "move"
helper at a wrong place already, and if the organization in this
patch somehow comes from a desire that the "move" is done only at
the outermost level (and "or immediately before an error causes the
whole thing to abort" after the next patch), it does not have to be
a silent "if call_depth is not zero we return silently", but another
assert() that insists that the callers are allowed to call it under
these two specific conditions.

Thanks.

Re: [PATCH 1/7] merge-ort: extract handling of priv member into reusable function

[Taylor Blau]

On Thu, Jun 13, 2024 at 02:00:00PM -0700, Junio C Hamano wrote:
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > +static void move_opt_priv_to_result_priv(struct merge_options *opt,
> > +					 struct merge_result *result)
> > +{
> > +	/*
> > +	 * opt->priv and result->priv are a bit weird.  opt->priv contains
> > +	 * information that we can re-use in subsequent merge operations to
> > +	 * enable our cached renames optimization.  The best way to provide
> > +	 * that to subsequent merges is putting it in result->priv.
> > +	 * However, putting it directly there would mean retrofitting lots
> > +	 * of functions in this file to also take a merge_result pointer,
> > +	 * which is ugly and annoying.  So, we just make sure at the end of
> > +	 * the merge (the outer merge if there are internal recursive ones)
> > +	 * to move it.
> > +	 */
> > +	assert(opt->priv && !result->priv);
> > +	if (!opt->priv->call_depth) {
> > +		result->priv = opt->priv;
> > +		result->_properly_initialized = RESULT_INITIALIZED;
> > +		opt->priv = NULL;
> > +	}
> > +}
> > +
> >  /*
> >   * Originally from merge_trees_internal(); heavily adapted, though.
> >   */
> > @@ -5060,11 +5082,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,
> >  		/* existence of conflicted entries implies unclean */
> >  		result->clean &= strmap_empty(&opt->priv->conflicted);
> >  	}
> > -	if (!opt->priv->call_depth) {
> > -		result->priv = opt->priv;
> > -		result->_properly_initialized = RESULT_INITIALIZED;
> > -		opt->priv = NULL;
> > -	}
> > +	move_opt_priv_to_result_priv(opt, result);
> >  }
>
> I have a feeling that making it the caller's responsibility to check
> "are we doing the outermost merge?"  and not the callee's problem
> would result in a better code organization.  If we write
>
> 	if (!opt->priv->call_depth)
> 		move_opt_priv_to_result_priv(opt, result);
>
> then for this call site, it is still crystal clear that this will
> happen only at the outermost level.  The new caller you add in the
> next step would also be simpler to reason about.

I had the same thought. Calling the function
move_opt_priv_to_result_priv() seems to indicate that reuslt->priv will
definitely be updated to opt->priv.

Another approach would be to rename the function
maybe_move_opt_priv_to_result_priv() and have it be a noop if
opt->priv->call_depth is non-zero.

But this is all fairly philosophical ;-). I do not really mind or feel
strongly either way whatsoever.

Thanks,
Taylor

[PATCH 2/7] merge-ort: maintain expected invariant for priv member

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

The calling convention for the merge machinery is
   One call to          init_merge_options()
   One or more calls to merge_incore_[non]recursive()
   One call to          merge_finalize()
      (possibly indirectly via merge_switch_to_result())
Both merge_switch_to_result() and merge_finalize() expect
   opt->priv == NULL && result->priv != NULL
which is supposed to be set up by our move_opt_priv_to_result_priv()
function.  However, two codepath dealing with error cases did not
execute this necessary logic, which could result in assertion failures
(or, if assertions were compiled out, could result in segfaults).  Fix
the oversight and add a test that would have caught one of these
problems.

While at it, also tighten an existing test for a non-recursive merge
to verify that it fails correctly, i.e. with the expected exit code
rather than with an assertion failure.

Reported-by: Matt Cree <matt.cree@gearset.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-ort.c           |  3 ++-
 t/t6406-merge-attr.sh | 42 +++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/merge-ort.c b/merge-ort.c
index 10f5a655f29..6ca7b0f9be4 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -5015,7 +5015,7 @@ static void move_opt_priv_to_result_priv(struct merge_options *opt,
 	 * to move it.
 	 */
 	assert(opt->priv && !result->priv);
-	if (!opt->priv->call_depth) {
+	if (!opt->priv->call_depth || result->clean < 0) {
 		result->priv = opt->priv;
 		result->_properly_initialized = RESULT_INITIALIZED;
 		opt->priv = NULL;
@@ -5052,6 +5052,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,
 		    oid_to_hex(&side1->object.oid),
 		    oid_to_hex(&side2->object.oid));
 		result->clean = -1;
+		move_opt_priv_to_result_priv(opt, result);
 		return;
 	}
 	trace2_region_leave("merge", "collect_merge_info", opt->repo);
diff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh
index 156a1efacfe..b6db5c2cc36 100755
--- a/t/t6406-merge-attr.sh
+++ b/t/t6406-merge-attr.sh
@@ -185,7 +185,7 @@ test_expect_success !WINDOWS 'custom merge driver that is killed with a signal'
 
 	>./please-abort &&
 	echo "* merge=custom" >.gitattributes &&
-	test_must_fail git merge main 2>err &&
+	test_expect_code 2 git merge main 2>err &&
 	grep "^error: failed to execute internal merge" err &&
 	git ls-files -u >output &&
 	git diff --name-only HEAD >>output &&
@@ -261,4 +261,44 @@ test_expect_success 'binary files with union attribute' '
 	grep -i "warning.*cannot merge.*HEAD vs. bin-main" output
 '
 
+test_expect_success !WINDOWS 'custom merge driver that is killed with a signal on recursive merge' '
+	test_when_finished "rm -f output please-abort" &&
+	test_when_finished "git checkout side" &&
+
+	git reset --hard anchor &&
+
+	git checkout -b base-a main^ &&
+	echo base-a >text &&
+	git commit -m base-a text &&
+
+	git checkout -b base-b main^ &&
+	echo base-b >text &&
+	git commit -m base-b text &&
+
+	git checkout -b recursive-a base-a &&
+	test_must_fail git merge base-b &&
+	echo recursive-a >text &&
+	git add text &&
+	git commit -m recursive-a &&
+
+	git checkout -b recursive-b base-b &&
+	test_must_fail git merge base-a &&
+	echo recursive-b >text &&
+	git add text &&
+	git commit -m recursive-b &&
+
+	git config --replace-all \
+	merge.custom.driver "./custom-merge %O %A %B 0 %P %S %X %Y" &&
+	git config --replace-all \
+	merge.custom.name "custom merge driver for testing" &&
+
+	>./please-abort &&
+	echo "* merge=custom" >.gitattributes &&
+	test_expect_code 2 git merge recursive-a 2>err &&
+	grep "^error: failed to execute internal merge" err &&
+	git ls-files -u >output &&
+	git diff --name-only HEAD >>output &&
+	test_must_be_empty output
+'
+
 test_done
-- 
gitgitgadget

Re: [PATCH 2/7] merge-ort: maintain expected invariant for priv member

[Taylor Blau]

On Thu, Jun 13, 2024 at 08:25:02PM +0000, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
>
> The calling convention for the merge machinery is
>    One call to          init_merge_options()
>    One or more calls to merge_incore_[non]recursive()
>    One call to          merge_finalize()
>       (possibly indirectly via merge_switch_to_result())
> Both merge_switch_to_result() and merge_finalize() expect
>    opt->priv == NULL && result->priv != NULL
> which is supposed to be set up by our move_opt_priv_to_result_priv()
> function.  However, two codepath dealing with error cases did not

s/codepath/&s/ ?

> execute this necessary logic, which could result in assertion failures
> (or, if assertions were compiled out, could result in segfaults).  Fix
> the oversight and add a test that would have caught one of these
> problems.
>
> While at it, also tighten an existing test for a non-recursive merge
> to verify that it fails correctly, i.e. with the expected exit code
> rather than with an assertion failure.

I suspect that this test was flaky, too, since if the assertion errors
were compiled out and it died via a segfault, the test would have failed
outright as test_must_fail does not allow for segfaults typically.

So I'm glad to see us tightening up that area of the test suite.

> Reported-by: Matt Cree <matt.cree@gearset.com>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  merge-ort.c           |  3 ++-
>  t/t6406-merge-attr.sh | 42 +++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 43 insertions(+), 2 deletions(-)
>
> diff --git a/merge-ort.c b/merge-ort.c
> index 10f5a655f29..6ca7b0f9be4 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -5015,7 +5015,7 @@ static void move_opt_priv_to_result_priv(struct merge_options *opt,
>  	 * to move it.
>  	 */
>  	assert(opt->priv && !result->priv);
> -	if (!opt->priv->call_depth) {
> +	if (!opt->priv->call_depth || result->clean < 0) {
>  		result->priv = opt->priv;
>  		result->_properly_initialized = RESULT_INITIALIZED;
>  		opt->priv = NULL;
> @@ -5052,6 +5052,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,
>  		    oid_to_hex(&side1->object.oid),
>  		    oid_to_hex(&side2->object.oid));
>  		result->clean = -1;
> +		move_opt_priv_to_result_priv(opt, result);
>  		return;
>  	}
>  	trace2_region_leave("merge", "collect_merge_info", opt->repo);
> diff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh
> index 156a1efacfe..b6db5c2cc36 100755
> --- a/t/t6406-merge-attr.sh
> +++ b/t/t6406-merge-attr.sh
> @@ -185,7 +185,7 @@ test_expect_success !WINDOWS 'custom merge driver that is killed with a signal'
>
>  	>./please-abort &&
>  	echo "* merge=custom" >.gitattributes &&
> -	test_must_fail git merge main 2>err &&
> +	test_expect_code 2 git merge main 2>err &&
>  	grep "^error: failed to execute internal merge" err &&
>  	git ls-files -u >output &&
>  	git diff --name-only HEAD >>output &&
> @@ -261,4 +261,44 @@ test_expect_success 'binary files with union attribute' '
>  	grep -i "warning.*cannot merge.*HEAD vs. bin-main" output
>  '
>
> +test_expect_success !WINDOWS 'custom merge driver that is killed with a signal on recursive merge' '
> +	test_when_finished "rm -f output please-abort" &&
> +	test_when_finished "git checkout side" &&
> +
> +	git reset --hard anchor &&
> +
> +	git checkout -b base-a main^ &&
> +	echo base-a >text &&
> +	git commit -m base-a text &&
> +
> +	git checkout -b base-b main^ &&
> +	echo base-b >text &&
> +	git commit -m base-b text &&
> +
> +	git checkout -b recursive-a base-a &&
> +	test_must_fail git merge base-b &&

Here and below, do you care about the particular exit code of merging as
above? IOW, should these be `test_expect_code`'s as well?

Thanks,
Taylor

Re: [PATCH 2/7] merge-ort: maintain expected invariant for priv member

[Elijah Newren]

On Thu, Jun 13, 2024 at 10:59 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Thu, Jun 13, 2024 at 08:25:02PM +0000, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@gmail.com>
> >
> > The calling convention for the merge machinery is
> >    One call to          init_merge_options()
> >    One or more calls to merge_incore_[non]recursive()
> >    One call to          merge_finalize()
> >       (possibly indirectly via merge_switch_to_result())
> > Both merge_switch_to_result() and merge_finalize() expect
> >    opt->priv == NULL && result->priv != NULL
> > which is supposed to be set up by our move_opt_priv_to_result_priv()
> > function.  However, two codepath dealing with error cases did not
>
> s/codepath/&s/ ?

Indeed.

> > execute this necessary logic, which could result in assertion failures
> > (or, if assertions were compiled out, could result in segfaults).  Fix
> > the oversight and add a test that would have caught one of these
> > problems.
> >
> > While at it, also tighten an existing test for a non-recursive merge
> > to verify that it fails correctly, i.e. with the expected exit code
> > rather than with an assertion failure.

It turns out my logic here was faulty; if a
   test_must_fail git ARGS...
command is run and git hits an assertion failure, the test_must_fail
will fail with
   test_must_fail: died by signal 6: git ARGS...
similar to how it would fail if git were to segfault.

However, I still like the idea of testing the exit status here because
of this comment in builtin/merge.c:
    /*
     * The backend exits with 1 when conflicts are
     * left to be resolved, with 2 when it does not
     * handle the given merge at all.
     */
and this is one of the few tests in the testsuite where we are
explicitly testing a case where the merge is neither success nor
conflicts, but failed-to-handle.

> I suspect that this test was flaky, too, since if the assertion errors
> were compiled out and it died via a segfault, the test would have failed
> outright as test_must_fail does not allow for segfaults typically.
>
> So I'm glad to see us tightening up that area of the test suite.
[...]
> > +     test_must_fail git merge base-b &&
>
> Here and below, do you care about the particular exit code of merging as
> above? IOW, should these be `test_expect_code`'s as well?

Both of these comments led me on the path to check something out and
discover my error above, which will lead to slightly different changes
to the patch.

[PATCH 3/7] merge-ort: fix type of local 'clean' var in handle_content_merge()

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

handle_content_merge() returns an int.  Every caller of
handle_content_merge() expects an int.  However, we declare a local
variable 'clean' that we use for the return value to be unsigned.  To
make matters worse, we also assign 'clean' the return value of
merge_submodule() in one codepath, which is defined to return an int.
It seems that the only reason to have 'clean' be unsigned was to allow a
cutesy bit manipulation operation to be well-defined.  Fix the type of
the 'clean' local in handle_content_merge().

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-ort.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/merge-ort.c b/merge-ort.c
index 6ca7b0f9be4..13b47d352fc 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -2109,7 +2109,7 @@ static int handle_content_merge(struct merge_options *opt,
 	 * merges, which happens for example with rename/rename(2to1) and
 	 * rename/add conflicts.
 	 */
-	unsigned clean = 1;
+	int clean = 1;
 
 	/*
 	 * handle_content_merge() needs both files to be of the same type, i.e.
@@ -2184,7 +2184,8 @@ static int handle_content_merge(struct merge_options *opt,
 		free(result_buf.ptr);
 		if (ret)
 			return -1;
-		clean &= (merge_status == 0);
+		if (merge_status > 0)
+			clean = 0;
 		path_msg(opt, INFO_AUTO_MERGING, 1, path, NULL, NULL, NULL,
 			 _("Auto-merging %s"), path);
 	} else if (S_ISGITLINK(a->mode)) {
-- 
gitgitgadget

[PATCH 4/7] merge-ort: clearer propagation of failure-to-function from merge_submodule

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

The 'clean' member variable is somewhat of a tri-state (1 = clean, 0 =
conflicted, -1 = failure-to-determine), but we often like to think of
it as binary (ignoring the possibility of a negative value) and use
constructs like '!clean' to reflect this.  However, these constructs
can make codepaths more difficult to understand, unless we handle the
negative case early and return pre-emptively; do that in
handle_content_merge() to make the code a bit easier to read.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-ort.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/merge-ort.c b/merge-ort.c
index 13b47d352fc..39799e65a36 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -2193,6 +2193,8 @@ static int handle_content_merge(struct merge_options *opt,
 		clean = merge_submodule(opt, pathnames[0],
 					two_way ? null_oid() : &o->oid,
 					&a->oid, &b->oid, &result->oid);
+		if (clean < 0)
+			return -1;
 		if (opt->priv->call_depth && two_way && !clean) {
 			result->mode = o->mode;
 			oidcpy(&result->oid, &o->oid);
-- 
gitgitgadget

[PATCH 5/7] merge-ort: loosen commented requirements

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

The comment above type_short_descriptions claimed that the order had to
match what was found in the conflict_info_and_types enum.  Since
type_short_descriptions uses designated initializers, the order should
not actually matter; I am guessing that positional initializers may have
been under consideration when that comment was added, but the comment
was not updated when designated initializers were chosen.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-ort.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/merge-ort.c b/merge-ort.c
index 39799e65a36..462bc6fb6e1 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -553,7 +553,7 @@ enum conflict_and_info_types {
  * Short description of conflict type, relied upon by external tools.
  *
  * We can add more entries, but DO NOT change any of these strings.  Also,
- * Order MUST match conflict_info_and_types.
+ * please ensure the order matches what is used in conflict_info_and_types.
  */
 static const char *type_short_descriptions[] = {
 	/*** "Simple" conflicts and informational messages ***/
-- 
gitgitgadget

[PATCH 6/7] merge-ort: upon merge abort, only show messages causing the abort

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

When something goes wrong enough that we need to abort early and not
even attempt merging the remaining files, it probably does not make
sense to report conflicts messages for the subset of files we processed
before hitting the fatal error.  Instead, only show the messages
associated with paths where we hit the fatal error.  Also, print these
messages to stderr rather than stdout.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-ort.c | 76 ++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 52 insertions(+), 24 deletions(-)

diff --git a/merge-ort.c b/merge-ort.c
index 462bc6fb6e1..5410dec2b4f 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -543,10 +543,24 @@ enum conflict_and_info_types {
 	CONFLICT_SUBMODULE_HISTORY_NOT_AVAILABLE,
 	CONFLICT_SUBMODULE_MAY_HAVE_REWINDS,
 	CONFLICT_SUBMODULE_NULL_MERGE_BASE,
-	CONFLICT_SUBMODULE_CORRUPT,
+
+	/* INSERT NEW ENTRIES HERE */
+
+	/*
+	 * Keep this entry after all regular conflict and info types; only
+	 * errors (failures causing immediate abort of the merge) should
+	 * come after this.
+	 */
+	NB_REGULAR_CONFLICT_TYPES,
+
+	/*
+	 * Something is seriously wrong; cannot even perform merge;
+	 * Keep this group _last_ other than NB_CONFLICT_TYPES
+	 */
+	ERROR_SUBMODULE_CORRUPT,
 
 	/* Keep this entry _last_ in the list */
-	NB_CONFLICT_TYPES,
+	NB_TOTAL_TYPES,
 };
 
 /*
@@ -597,8 +611,10 @@ static const char *type_short_descriptions[] = {
 		"CONFLICT (submodule may have rewinds)",
 	[CONFLICT_SUBMODULE_NULL_MERGE_BASE] =
 		"CONFLICT (submodule lacks merge base)",
-	[CONFLICT_SUBMODULE_CORRUPT] =
-		"CONFLICT (submodule corrupt)"
+
+	/* Something is seriously wrong; cannot even perform merge */
+	[ERROR_SUBMODULE_CORRUPT] =
+		"ERROR (submodule corrupt)",
 };
 
 struct logical_conflict_info {
@@ -762,7 +778,8 @@ static void path_msg(struct merge_options *opt,
 
 	/* Sanity checks */
 	assert(omittable_hint ==
-	       !starts_with(type_short_descriptions[type], "CONFLICT") ||
+	       (!starts_with(type_short_descriptions[type], "CONFLICT") &&
+		!starts_with(type_short_descriptions[type], "ERROR")) ||
 	       type == CONFLICT_DIR_RENAME_SUGGESTED);
 	if (opt->record_conflict_msgs_as_headers && omittable_hint)
 		return; /* Do not record mere hints in headers */
@@ -1817,9 +1834,9 @@ static int merge_submodule(struct merge_options *opt,
 	/* check whether both changes are forward */
 	ret2 = repo_in_merge_bases(&subrepo, commit_o, commit_a);
 	if (ret2 < 0) {
-		path_msg(opt, CONFLICT_SUBMODULE_CORRUPT, 0,
+		path_msg(opt, ERROR_SUBMODULE_CORRUPT, 0,
 			 path, NULL, NULL, NULL,
-			 _("Failed to merge submodule %s "
+			 _("error: failed to merge submodule %s "
 			   "(repository corrupt)"),
 			 path);
 		ret = -1;
@@ -1828,9 +1845,9 @@ static int merge_submodule(struct merge_options *opt,
 	if (ret2 > 0)
 		ret2 = repo_in_merge_bases(&subrepo, commit_o, commit_b);
 	if (ret2 < 0) {
-		path_msg(opt, CONFLICT_SUBMODULE_CORRUPT, 0,
+		path_msg(opt, ERROR_SUBMODULE_CORRUPT, 0,
 			 path, NULL, NULL, NULL,
-			 _("Failed to merge submodule %s "
+			 _("error: failed to merge submodule %s "
 			   "(repository corrupt)"),
 			 path);
 		ret = -1;
@@ -1848,7 +1865,7 @@ static int merge_submodule(struct merge_options *opt,
 	/* Case #1: a is contained in b or vice versa */
 	ret2 = repo_in_merge_bases(&subrepo, commit_a, commit_b);
 	if (ret2 < 0) {
-		path_msg(opt, CONFLICT_SUBMODULE_CORRUPT, 0,
+		path_msg(opt, ERROR_SUBMODULE_CORRUPT, 0,
 			 path, NULL, NULL, NULL,
 			 _("Failed to merge submodule %s "
 			   "(repository corrupt)"),
@@ -1867,9 +1884,9 @@ static int merge_submodule(struct merge_options *opt,
 	}
 	ret2 = repo_in_merge_bases(&subrepo, commit_b, commit_a);
 	if (ret2 < 0) {
-		path_msg(opt, CONFLICT_SUBMODULE_CORRUPT, 0,
+		path_msg(opt, ERROR_SUBMODULE_CORRUPT, 0,
 			 path, NULL, NULL, NULL,
-			 _("Failed to merge submodule %s "
+			 _("error: failed to merge submodule %s "
 			   "(repository corrupt)"),
 			 path);
 		ret = -1;
@@ -1901,9 +1918,9 @@ static int merge_submodule(struct merge_options *opt,
 					 &merges);
 	switch (parent_count) {
 	case -1:
-		path_msg(opt, CONFLICT_SUBMODULE_CORRUPT, 0,
+		path_msg(opt, ERROR_SUBMODULE_CORRUPT, 0,
 			 path, NULL, NULL, NULL,
-			 _("Failed to merge submodule %s "
+			 _("error: failed to merge submodule %s "
 			   "(repository corrupt)"),
 			 path);
 		ret = -1;
@@ -4646,6 +4663,7 @@ void merge_display_update_messages(struct merge_options *opt,
 	struct hashmap_iter iter;
 	struct strmap_entry *e;
 	struct string_list olist = STRING_LIST_INIT_NODUP;
+	FILE *o = stdout;
 
 	if (opt->record_conflict_msgs_as_headers)
 		BUG("Either display conflict messages or record them as headers, not both");
@@ -4662,6 +4680,10 @@ void merge_display_update_messages(struct merge_options *opt,
 	}
 	string_list_sort(&olist);
 
+	/* Print to stderr if we hit errors rather than just conflicts */
+	if (result->clean < 0)
+		o = stderr;
+
 	/* Iterate over the items, printing them */
 	for (int path_nr = 0; path_nr < olist.nr; ++path_nr) {
 		struct string_list *conflicts = olist.items[path_nr].util;
@@ -4669,25 +4691,31 @@ void merge_display_update_messages(struct merge_options *opt,
 			struct logical_conflict_info *info =
 				conflicts->items[i].util;
 
+			/* On failure, ignore regular conflict types */
+			if (result->clean < 0 &&
+			    info->type < NB_REGULAR_CONFLICT_TYPES)
+				continue;
+
 			if (detailed) {
-				printf("%lu", (unsigned long)info->paths.nr);
-				putchar('\0');
+				fprintf(o, "%lu", (unsigned long)info->paths.nr);
+				fputc('\0', o);
 				for (int n = 0; n < info->paths.nr; n++) {
-					fputs(info->paths.v[n], stdout);
-					putchar('\0');
+					fputs(info->paths.v[n], o);
+					fputc('\0', o);
 				}
-				fputs(type_short_descriptions[info->type],
-				      stdout);
-				putchar('\0');
+				fputs(type_short_descriptions[info->type], o);
+				fputc('\0', o);
 			}
-			puts(conflicts->items[i].string);
+			fputs(conflicts->items[i].string, o);
+			fputc('\n', o);
 			if (detailed)
-				putchar('\0');
+				fputc('\0', o);
 		}
 	}
 	string_list_clear(&olist, 0);
 
-	print_submodule_conflict_suggestion(&opti->conflicted_submodules);
+	if (result->clean >= 0)
+		print_submodule_conflict_suggestion(&opti->conflicted_submodules);
 
 	/* Also include needed rename limit adjustment now */
 	diff_warn_rename_limit("merge.renamelimit",
-- 
gitgitgadget

Re: [PATCH 6/7] merge-ort: upon merge abort, only show messages causing the abort

[Eric Sunshine]

On Thu, Jun 13, 2024 at 4:25 PM Elijah Newren via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> When something goes wrong enough that we need to abort early and not
> even attempt merging the remaining files, it probably does not make
> sense to report conflicts messages for the subset of files we processed
> before hitting the fatal error.  Instead, only show the messages
> associated with paths where we hit the fatal error.  Also, print these
> messages to stderr rather than stdout.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
> diff --git a/merge-ort.c b/merge-ort.c
> @@ -543,10 +543,24 @@ enum conflict_and_info_types {
> -       CONFLICT_SUBMODULE_CORRUPT,
> +
> +       /* INSERT NEW ENTRIES HERE */
> +       /*
> +        * Something is seriously wrong; cannot even perform merge;
> +        * Keep this group _last_ other than NB_CONFLICT_TYPES
> +        */

I'm probably missing something obvious, but here the new comment talks
about NB_CONFLICT_TYPES...

> +       ERROR_SUBMODULE_CORRUPT,
>
>         /* Keep this entry _last_ in the list */
> -       NB_CONFLICT_TYPES,
> +       NB_TOTAL_TYPES,

... but NB_CONFLICT_TYPES gets removed here.

> @@ -1828,9 +1845,9 @@ static int merge_submodule(struct merge_options *opt,
> -                        _("Failed to merge submodule %s "
> +                        _("error: failed to merge submodule %s "
> @@ -1848,7 +1865,7 @@ static int merge_submodule(struct merge_options *opt,
>                          _("Failed to merge submodule %s "
>                            "(repository corrupt)"),

Do you also want to apply the same "error: failed..." transformation
to this error message as you did to other error messages?

Re: [PATCH 6/7] merge-ort: upon merge abort, only show messages causing the abort

[Elijah Newren]

On Fri, Jun 14, 2024 at 4:19 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Thu, Jun 13, 2024 at 4:25 PM Elijah Newren via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > When something goes wrong enough that we need to abort early and not
> > even attempt merging the remaining files, it probably does not make
> > sense to report conflicts messages for the subset of files we processed
> > before hitting the fatal error.  Instead, only show the messages
> > associated with paths where we hit the fatal error.  Also, print these
> > messages to stderr rather than stdout.
> >
> > Signed-off-by: Elijah Newren <newren@gmail.com>
> > ---
> > diff --git a/merge-ort.c b/merge-ort.c
> > @@ -543,10 +543,24 @@ enum conflict_and_info_types {
> > -       CONFLICT_SUBMODULE_CORRUPT,
> > +
> > +       /* INSERT NEW ENTRIES HERE */
> > +       /*
> > +        * Something is seriously wrong; cannot even perform merge;
> > +        * Keep this group _last_ other than NB_CONFLICT_TYPES
> > +        */
>
> I'm probably missing something obvious, but here the new comment talks
> about NB_CONFLICT_TYPES...
>
> > +       ERROR_SUBMODULE_CORRUPT,
> >
> >         /* Keep this entry _last_ in the list */
> > -       NB_CONFLICT_TYPES,
> > +       NB_TOTAL_TYPES,
>
> ... but NB_CONFLICT_TYPES gets removed here.
>
> > @@ -1828,9 +1845,9 @@ static int merge_submodule(struct merge_options *opt,
> > -                        _("Failed to merge submodule %s "
> > +                        _("error: failed to merge submodule %s "
> > @@ -1848,7 +1865,7 @@ static int merge_submodule(struct merge_options *opt,
> >                          _("Failed to merge submodule %s "
> >                            "(repository corrupt)"),
>
> Do you also want to apply the same "error: failed..." transformation
> to this error message as you did to other error messages?

Doh, I tried to re-read through my patches after making numerous
additional tweaks to try to catch stuff like this, but I clearly
missed a few cases here.  Thanks for catching both of these issues;
I'll fix them up.

[PATCH 7/7] merge-ort: convert more error() cases to path_msg()

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

merge_submodule() stores errors using path_msg(), whereas other call
sites make use of the error() function.  This is inconsistent, and
moving towards path_msg() seems more friendly for libification efforts
since it will allow the caller to determine whether the error messages
need to be printed.

Note that this deferred handling of error messages changes the error
message in a recursive merge from
  error: failed to execute internal merge
to
  From inner merge:  error: failed to execute internal merge
which provides a little more information about the error which may be
useful.  Since the recursive merge strategy still only shows the older
error, we had to adjust the new testcase introduced a few commits ago to
just search for the older message somewhere in the output.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-ort.c           | 53 +++++++++++++++++++++++++++++++++----------
 t/t6406-merge-attr.sh |  2 +-
 2 files changed, 42 insertions(+), 13 deletions(-)

diff --git a/merge-ort.c b/merge-ort.c
index 5410dec2b4f..493aa55aa80 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -558,6 +558,10 @@ enum conflict_and_info_types {
 	 * Keep this group _last_ other than NB_CONFLICT_TYPES
 	 */
 	ERROR_SUBMODULE_CORRUPT,
+	ERROR_THREEWAY_CONTENT_MERGE_FAILED,
+	ERROR_OBJECT_WRITE_FAILED,
+	ERROR_OBJECT_READ_FAILED,
+	ERROR_OBJECT_NOT_A_BLOB,
 
 	/* Keep this entry _last_ in the list */
 	NB_TOTAL_TYPES,
@@ -615,6 +619,14 @@ static const char *type_short_descriptions[] = {
 	/* Something is seriously wrong; cannot even perform merge */
 	[ERROR_SUBMODULE_CORRUPT] =
 		"ERROR (submodule corrupt)",
+	[ERROR_THREEWAY_CONTENT_MERGE_FAILED] =
+		"ERROR (three-way content merge failed)",
+	[ERROR_OBJECT_WRITE_FAILED] =
+		"ERROR (object write failed)",
+	[ERROR_OBJECT_READ_FAILED] =
+		"ERROR (object read failed)",
+	[ERROR_OBJECT_NOT_A_BLOB] =
+		"ERROR (object is not a blob)",
 };
 
 struct logical_conflict_info {
@@ -2190,15 +2202,24 @@ static int handle_content_merge(struct merge_options *opt,
 					  pathnames, extra_marker_size,
 					  &result_buf);
 
-		if ((merge_status < 0) || !result_buf.ptr)
-			ret = error(_("failed to execute internal merge"));
+		if ((merge_status < 0) || !result_buf.ptr) {
+			path_msg(opt, ERROR_THREEWAY_CONTENT_MERGE_FAILED, 0,
+				 pathnames[0], pathnames[1], pathnames[2], NULL,
+				 _("error: failed to execute internal merge for %s"),
+				 path);
+			ret = -1;
+		}
 
 		if (!ret &&
 		    write_object_file(result_buf.ptr, result_buf.size,
-				      OBJ_BLOB, &result->oid))
-			ret = error(_("unable to add %s to database"), path);
-
+				      OBJ_BLOB, &result->oid)) {
+			path_msg(opt, ERROR_OBJECT_WRITE_FAILED, 0,
+				 pathnames[0], pathnames[1], pathnames[2], NULL,
+				 _("error: unable to add %s to database"), path);
+			ret = -1;
+		}
 		free(result_buf.ptr);
+
 		if (ret)
 			return -1;
 		if (merge_status > 0)
@@ -3577,18 +3598,26 @@ static int sort_dirs_next_to_their_children(const char *one, const char *two)
 		return c1 - c2;
 }
 
-static int read_oid_strbuf(const struct object_id *oid,
-			   struct strbuf *dst)
+static int read_oid_strbuf(struct merge_options *opt,
+			   const struct object_id *oid,
+			   struct strbuf *dst,
+			   const char *path)
 {
 	void *buf;
 	enum object_type type;
 	unsigned long size;
 	buf = repo_read_object_file(the_repository, oid, &type, &size);
-	if (!buf)
-		return error(_("cannot read object %s"), oid_to_hex(oid));
+	if (!buf) {
+		path_msg(opt, ERROR_OBJECT_READ_FAILED, 0,
+			 path, NULL, NULL, NULL,
+			 _("error: cannot read object %s"), oid_to_hex(oid));
+		return -1;
+	}
 	if (type != OBJ_BLOB) {
 		free(buf);
-		return error(_("object %s is not a blob"), oid_to_hex(oid));
+		path_msg(opt, ERROR_OBJECT_NOT_A_BLOB, 0,
+			 path, NULL, NULL, NULL,
+			 _("error: object %s is not a blob"), oid_to_hex(oid));
 	}
 	strbuf_attach(dst, buf, size, size + 1);
 	return 0;
@@ -3612,8 +3641,8 @@ static int blob_unchanged(struct merge_options *opt,
 	if (oideq(&base->oid, &side->oid))
 		return 1;
 
-	if (read_oid_strbuf(&base->oid, &basebuf) ||
-	    read_oid_strbuf(&side->oid, &sidebuf))
+	if (read_oid_strbuf(opt, &base->oid, &basebuf, path) ||
+	    read_oid_strbuf(opt, &side->oid, &sidebuf, path))
 		goto error_return;
 	/*
 	 * Note: binary | is used so that both renormalizations are
diff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh
index b6db5c2cc36..9bf95249347 100755
--- a/t/t6406-merge-attr.sh
+++ b/t/t6406-merge-attr.sh
@@ -295,7 +295,7 @@ test_expect_success !WINDOWS 'custom merge driver that is killed with a signal o
 	>./please-abort &&
 	echo "* merge=custom" >.gitattributes &&
 	test_expect_code 2 git merge recursive-a 2>err &&
-	grep "^error: failed to execute internal merge" err &&
+	grep "error: failed to execute internal merge" err &&
 	git ls-files -u >output &&
 	git diff --name-only HEAD >>output &&
 	test_must_be_empty output
-- 
gitgitgadget

Re: [PATCH 0/7] Fix and improve some error codepaths in merge-ort

[Taylor Blau]

On Thu, Jun 13, 2024 at 08:25:00PM +0000, Elijah Newren via GitGitGadget wrote:
> Elijah Newren (7):
>   merge-ort: extract handling of priv member into reusable function
>   merge-ort: maintain expected invariant for priv member
>   merge-ort: fix type of local 'clean' var in handle_content_merge()
>   merge-ort: clearer propagation of failure-to-function from
>     merge_submodule
>   merge-ort: loosen commented requirements
>   merge-ort: upon merge abort, only show messages causing the abort
>   merge-ort: convert more error() cases to path_msg()
>
>  merge-ort.c           | 167 +++++++++++++++++++++++++++++++-----------
>  t/t6406-merge-attr.sh |  42 ++++++++++-
>  2 files changed, 164 insertions(+), 45 deletions(-)

Very nice. I had a couple of minor thoughts on the earlier patches, but
I agree with the substantive ones that printing only messages related to
paths that we were processing when something went horribly wrong is a
good idea.

I don't think that either of my comments alone merit a reroll, and I'd
be happy to see this version move along as-is. But I'm equally happy if
you want to fix a typo here or there or do some bikeshedding ;-).

Thanks for fixing this issue.

Thanks,
Taylor

[PATCH v2 0/7] Fix and improve some error codepaths in merge-ort

[Elijah Newren via GitGitGadget]

Changes since v1:

 * Remove the conditional outside of move_opt_priv_to_result_priv() for
   patches 1 & 2
 * Fix the consistency issues in patch 6 (refer to updated enum value in
   comment, use consistent prefix on error messages)
 * Fix typo in patch 2
 * Fix incorrect claim in commit message of patch 2 and provide more detail
   about which merges we are interested in checking for a very specific exit
   code.

This series started as a just a fix for the abort hit in merge-ort when
custom merge drivers error out (see
https://lore.kernel.org/git/75F8BD12-7743-4863-B4C5-049FDEC4645E@gearset.com/).
However, while working on that, I found a few other issues around error
codepaths in merge-ort. So this series:

 * Patches 1-2: fix the reported abort problem
 * Patches 3-4: make code in handle_content_merges() easier to handle when
   we hit errors
 * Patch 5: fix a misleading comment
 * Patches 6-7: make error handling (immediate print vs. letting callers get
   the error information) more consistent

The last two patches change the behavior slightly for error codepaths, and
there's a question about whether we should show only the error messages that
caused an early termination of the merge, or if we should also show any
conflict messages for other paths that were handled before we hit the early
termination. These patches made a decision but feel free to take those last
two patches as more of an RFC.

Elijah Newren (7):
  merge-ort: extract handling of priv member into reusable function
  merge-ort: maintain expected invariant for priv member
  merge-ort: fix type of local 'clean' var in handle_content_merge()
  merge-ort: clearer propagation of failure-to-function from
    merge_submodule
  merge-ort: loosen commented requirements
  merge-ort: upon merge abort, only show messages causing the abort
  merge-ort: convert more error() cases to path_msg()

 merge-ort.c           | 168 +++++++++++++++++++++++++++++++-----------
 t/t6406-merge-attr.sh |  42 ++++++++++-
 2 files changed, 164 insertions(+), 46 deletions(-)


base-commit: 8d94cfb54504f2ec9edc7ca3eb5c29a3dd3675ae
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1748%2Fnewren%2Ffix-error-cases-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1748/newren/fix-error-cases-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1748

Range-diff vs v1:

 1:  d4ba1fccd91 ! 1:  5c50c0b3aad merge-ort: extract handling of priv member into reusable function
     @@ merge-ort.c: static void merge_check_renames_reusable(struct merge_result *resul
      +	 * to move it.
      +	 */
      +	assert(opt->priv && !result->priv);
     -+	if (!opt->priv->call_depth) {
     -+		result->priv = opt->priv;
     -+		result->_properly_initialized = RESULT_INITIALIZED;
     -+		opt->priv = NULL;
     -+	}
     ++	result->priv = opt->priv;
     ++	result->_properly_initialized = RESULT_INITIALIZED;
     ++	opt->priv = NULL;
      +}
      +
       /*
     @@ merge-ort.c: static void merge_ort_nonrecursive_internal(struct merge_options *o
      -		result->_properly_initialized = RESULT_INITIALIZED;
      -		opt->priv = NULL;
      -	}
     -+	move_opt_priv_to_result_priv(opt, result);
     ++	if (!opt->priv->call_depth)
     ++		move_opt_priv_to_result_priv(opt, result);
       }
       
       /*
 2:  17c97301baa ! 2:  d1adec6d105 merge-ort: maintain expected invariant for priv member
     @@ Commit message
          Both merge_switch_to_result() and merge_finalize() expect
             opt->priv == NULL && result->priv != NULL
          which is supposed to be set up by our move_opt_priv_to_result_priv()
     -    function.  However, two codepath dealing with error cases did not
     +    function.  However, two codepaths dealing with error cases did not
          execute this necessary logic, which could result in assertion failures
          (or, if assertions were compiled out, could result in segfaults).  Fix
          the oversight and add a test that would have caught one of these
          problems.
      
          While at it, also tighten an existing test for a non-recursive merge
     -    to verify that it fails correctly, i.e. with the expected exit code
     -    rather than with an assertion failure.
     +    to verify that it fails with appropriate status.  Most merge tests in
     +    the testsuite check either for success or conflicts; those testing for
     +    neither are rare and it is good to ensure they support the invariant
     +    assumed by builtin/merge.c in this comment:
     +        /*
     +         * The backend exits with 1 when conflicts are
     +         * left to be resolved, with 2 when it does not
     +         * handle the given merge at all.
     +         */
     +    So, explicitly check for the exit status of 2 in these cases.
      
          Reported-by: Matt Cree <matt.cree@gearset.com>
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
       ## merge-ort.c ##
     -@@ merge-ort.c: static void move_opt_priv_to_result_priv(struct merge_options *opt,
     - 	 * to move it.
     - 	 */
     - 	assert(opt->priv && !result->priv);
     --	if (!opt->priv->call_depth) {
     -+	if (!opt->priv->call_depth || result->clean < 0) {
     - 		result->priv = opt->priv;
     - 		result->_properly_initialized = RESULT_INITIALIZED;
     - 		opt->priv = NULL;
      @@ merge-ort.c: static void merge_ort_nonrecursive_internal(struct merge_options *opt,
       		    oid_to_hex(&side1->object.oid),
       		    oid_to_hex(&side2->object.oid));
     @@ merge-ort.c: static void merge_ort_nonrecursive_internal(struct merge_options *o
       		return;
       	}
       	trace2_region_leave("merge", "collect_merge_info", opt->repo);
     +@@ merge-ort.c: static void merge_ort_nonrecursive_internal(struct merge_options *opt,
     + 		/* existence of conflicted entries implies unclean */
     + 		result->clean &= strmap_empty(&opt->priv->conflicted);
     + 	}
     +-	if (!opt->priv->call_depth)
     ++	if (!opt->priv->call_depth || result->clean < 0)
     + 		move_opt_priv_to_result_priv(opt, result);
     + }
     + 
      
       ## t/t6406-merge-attr.sh ##
      @@ t/t6406-merge-attr.sh: test_expect_success !WINDOWS 'custom merge driver that is killed with a signal'
 3:  23bb3386114 = 3:  034b91db1d2 merge-ort: fix type of local 'clean' var in handle_content_merge()
 4:  2789c58cd3f = 4:  2813a15b48b merge-ort: clearer propagation of failure-to-function from merge_submodule
 5:  975fbddf305 = 5:  750acae4dba merge-ort: loosen commented requirements
 6:  71391b18c1a ! 6:  6756956d0c7 merge-ort: upon merge abort, only show messages causing the abort
     @@ merge-ort.c: enum conflict_and_info_types {
      +
      +	/*
      +	 * Something is seriously wrong; cannot even perform merge;
     -+	 * Keep this group _last_ other than NB_CONFLICT_TYPES
     ++	 * Keep this group _last_ other than NB_TOTAL_TYPES
      +	 */
      +	ERROR_SUBMODULE_CORRUPT,
       
     @@ merge-ort.c: static int merge_submodule(struct merge_options *opt,
      -		path_msg(opt, CONFLICT_SUBMODULE_CORRUPT, 0,
      +		path_msg(opt, ERROR_SUBMODULE_CORRUPT, 0,
       			 path, NULL, NULL, NULL,
     - 			 _("Failed to merge submodule %s "
     +-			 _("Failed to merge submodule %s "
     ++			 _("error: failed to merge submodule %s "
       			   "(repository corrupt)"),
     + 			 path);
     + 		ret = -1;
      @@ merge-ort.c: static int merge_submodule(struct merge_options *opt,
       	}
       	ret2 = repo_in_merge_bases(&subrepo, commit_b, commit_a);
 7:  32ae44b6260 ! 7:  500433edf49 merge-ort: convert more error() cases to path_msg()
     @@ Commit message
      
       ## merge-ort.c ##
      @@ merge-ort.c: enum conflict_and_info_types {
     - 	 * Keep this group _last_ other than NB_CONFLICT_TYPES
     + 	 * Keep this group _last_ other than NB_TOTAL_TYPES
       	 */
       	ERROR_SUBMODULE_CORRUPT,
      +	ERROR_THREEWAY_CONTENT_MERGE_FAILED,

-- 
gitgitgadget

[PATCH v2 1/7] merge-ort: extract handling of priv member into reusable function

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

In preparation for a subsequent commit which will ensure we do not
forget to maintain our invariants for the priv member in error
codepaths, extract the necessary functionality out into a separate
function.  This change is cosmetic at this point, and introduces no
changes beyond an extra assertion sanity check.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-ort.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/merge-ort.c b/merge-ort.c
index eaede6cead9..700ddfccb90 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -5000,6 +5000,26 @@ static void merge_check_renames_reusable(struct merge_result *result,
 
 /*** Function Grouping: merge_incore_*() and their internal variants ***/
 
+static void move_opt_priv_to_result_priv(struct merge_options *opt,
+					 struct merge_result *result)
+{
+	/*
+	 * opt->priv and result->priv are a bit weird.  opt->priv contains
+	 * information that we can re-use in subsequent merge operations to
+	 * enable our cached renames optimization.  The best way to provide
+	 * that to subsequent merges is putting it in result->priv.
+	 * However, putting it directly there would mean retrofitting lots
+	 * of functions in this file to also take a merge_result pointer,
+	 * which is ugly and annoying.  So, we just make sure at the end of
+	 * the merge (the outer merge if there are internal recursive ones)
+	 * to move it.
+	 */
+	assert(opt->priv && !result->priv);
+	result->priv = opt->priv;
+	result->_properly_initialized = RESULT_INITIALIZED;
+	opt->priv = NULL;
+}
+
 /*
  * Originally from merge_trees_internal(); heavily adapted, though.
  */
@@ -5060,11 +5080,8 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,
 		/* existence of conflicted entries implies unclean */
 		result->clean &= strmap_empty(&opt->priv->conflicted);
 	}
-	if (!opt->priv->call_depth) {
-		result->priv = opt->priv;
-		result->_properly_initialized = RESULT_INITIALIZED;
-		opt->priv = NULL;
-	}
+	if (!opt->priv->call_depth)
+		move_opt_priv_to_result_priv(opt, result);
 }
 
 /*
-- 
gitgitgadget

[PATCH v2 2/7] merge-ort: maintain expected invariant for priv member

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

The calling convention for the merge machinery is
   One call to          init_merge_options()
   One or more calls to merge_incore_[non]recursive()
   One call to          merge_finalize()
      (possibly indirectly via merge_switch_to_result())
Both merge_switch_to_result() and merge_finalize() expect
   opt->priv == NULL && result->priv != NULL
which is supposed to be set up by our move_opt_priv_to_result_priv()
function.  However, two codepaths dealing with error cases did not
execute this necessary logic, which could result in assertion failures
(or, if assertions were compiled out, could result in segfaults).  Fix
the oversight and add a test that would have caught one of these
problems.

While at it, also tighten an existing test for a non-recursive merge
to verify that it fails with appropriate status.  Most merge tests in
the testsuite check either for success or conflicts; those testing for
neither are rare and it is good to ensure they support the invariant
assumed by builtin/merge.c in this comment:
    /*
     * The backend exits with 1 when conflicts are
     * left to be resolved, with 2 when it does not
     * handle the given merge at all.
     */
So, explicitly check for the exit status of 2 in these cases.

Reported-by: Matt Cree <matt.cree@gearset.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-ort.c           |  3 ++-
 t/t6406-merge-attr.sh | 42 +++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/merge-ort.c b/merge-ort.c
index 700ddfccb90..dc62aaf6d11 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -5050,6 +5050,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,
 		    oid_to_hex(&side1->object.oid),
 		    oid_to_hex(&side2->object.oid));
 		result->clean = -1;
+		move_opt_priv_to_result_priv(opt, result);
 		return;
 	}
 	trace2_region_leave("merge", "collect_merge_info", opt->repo);
@@ -5080,7 +5081,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,
 		/* existence of conflicted entries implies unclean */
 		result->clean &= strmap_empty(&opt->priv->conflicted);
 	}
-	if (!opt->priv->call_depth)
+	if (!opt->priv->call_depth || result->clean < 0)
 		move_opt_priv_to_result_priv(opt, result);
 }
 
diff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh
index 156a1efacfe..b6db5c2cc36 100755
--- a/t/t6406-merge-attr.sh
+++ b/t/t6406-merge-attr.sh
@@ -185,7 +185,7 @@ test_expect_success !WINDOWS 'custom merge driver that is killed with a signal'
 
 	>./please-abort &&
 	echo "* merge=custom" >.gitattributes &&
-	test_must_fail git merge main 2>err &&
+	test_expect_code 2 git merge main 2>err &&
 	grep "^error: failed to execute internal merge" err &&
 	git ls-files -u >output &&
 	git diff --name-only HEAD >>output &&
@@ -261,4 +261,44 @@ test_expect_success 'binary files with union attribute' '
 	grep -i "warning.*cannot merge.*HEAD vs. bin-main" output
 '
 
+test_expect_success !WINDOWS 'custom merge driver that is killed with a signal on recursive merge' '
+	test_when_finished "rm -f output please-abort" &&
+	test_when_finished "git checkout side" &&
+
+	git reset --hard anchor &&
+
+	git checkout -b base-a main^ &&
+	echo base-a >text &&
+	git commit -m base-a text &&
+
+	git checkout -b base-b main^ &&
+	echo base-b >text &&
+	git commit -m base-b text &&
+
+	git checkout -b recursive-a base-a &&
+	test_must_fail git merge base-b &&
+	echo recursive-a >text &&
+	git add text &&
+	git commit -m recursive-a &&
+
+	git checkout -b recursive-b base-b &&
+	test_must_fail git merge base-a &&
+	echo recursive-b >text &&
+	git add text &&
+	git commit -m recursive-b &&
+
+	git config --replace-all \
+	merge.custom.driver "./custom-merge %O %A %B 0 %P %S %X %Y" &&
+	git config --replace-all \
+	merge.custom.name "custom merge driver for testing" &&
+
+	>./please-abort &&
+	echo "* merge=custom" >.gitattributes &&
+	test_expect_code 2 git merge recursive-a 2>err &&
+	grep "^error: failed to execute internal merge" err &&
+	git ls-files -u >output &&
+	git diff --name-only HEAD >>output &&
+	test_must_be_empty output
+'
+
 test_done
-- 
gitgitgadget

Re: [PATCH v2 2/7] merge-ort: maintain expected invariant for priv member

[Derrick Stolee]

On 6/18/24 11:00 PM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> The calling convention for the merge machinery is
>     One call to          init_merge_options()
>     One or more calls to merge_incore_[non]recursive()
>     One call to          merge_finalize()
>        (possibly indirectly via merge_switch_to_result())
> Both merge_switch_to_result() and merge_finalize() expect
>     opt->priv == NULL && result->priv != NULL
> which is supposed to be set up by our move_opt_priv_to_result_priv()
> function.  However, two codepaths dealing with error cases did not
> execute this necessary logic, which could result in assertion failures
> (or, if assertions were compiled out, could result in segfaults).  Fix
> the oversight and add a test that would have caught one of these
> problems.

"one of these problems" is key here.

> @@ -5050,6 +5050,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,
>   		    oid_to_hex(&side1->object.oid),
>   		    oid_to_hex(&side2->object.oid));
>   		result->clean = -1;
> +		move_opt_priv_to_result_priv(opt, result);
>   		return;
>   	} >   	trace2_region_leave("merge", "collect_merge_info", opt->repo);

Removing this line does not cause your test script to fail. This is
understandable as this case only happens during a parse failure, so
it would be unreasonable to generate a test case that only fails
for such a reason.

> @@ -5080,7 +5081,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,
>   		/* existence of conflicted entries implies unclean */
>   		result->clean &= strmap_empty(&opt->priv->conflicted);
>   	}
> -	if (!opt->priv->call_depth)
> +	if (!opt->priv->call_depth || result->clean < 0)
>   		move_opt_priv_to_result_priv(opt, result);
>   }

Removing this change does get caught by the new test.

Thanks,
-Stolee

[PATCH v2 3/7] merge-ort: fix type of local 'clean' var in handle_content_merge()

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

handle_content_merge() returns an int.  Every caller of
handle_content_merge() expects an int.  However, we declare a local
variable 'clean' that we use for the return value to be unsigned.  To
make matters worse, we also assign 'clean' the return value of
merge_submodule() in one codepath, which is defined to return an int.
It seems that the only reason to have 'clean' be unsigned was to allow a
cutesy bit manipulation operation to be well-defined.  Fix the type of
the 'clean' local in handle_content_merge().

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-ort.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/merge-ort.c b/merge-ort.c
index dc62aaf6d11..be0f5bc3838 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -2109,7 +2109,7 @@ static int handle_content_merge(struct merge_options *opt,
 	 * merges, which happens for example with rename/rename(2to1) and
 	 * rename/add conflicts.
 	 */
-	unsigned clean = 1;
+	int clean = 1;
 
 	/*
 	 * handle_content_merge() needs both files to be of the same type, i.e.
@@ -2184,7 +2184,8 @@ static int handle_content_merge(struct merge_options *opt,
 		free(result_buf.ptr);
 		if (ret)
 			return -1;
-		clean &= (merge_status == 0);
+		if (merge_status > 0)
+			clean = 0;
 		path_msg(opt, INFO_AUTO_MERGING, 1, path, NULL, NULL, NULL,
 			 _("Auto-merging %s"), path);
 	} else if (S_ISGITLINK(a->mode)) {
-- 
gitgitgadget

Re: [PATCH v2 3/7] merge-ort: fix type of local 'clean' var in handle_content_merge()

[Derrick Stolee]

On 6/18/24 11:00 PM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> handle_content_merge() returns an int.  Every caller of
> handle_content_merge() expects an int.  However, we declare a local
> variable 'clean' that we use for the return value to be unsigned.  To
> make matters worse, we also assign 'clean' the return value of
> merge_submodule() in one codepath, which is defined to return an int.
> It seems that the only reason to have 'clean' be unsigned was to allow a
> cutesy bit manipulation operation to be well-defined.  Fix the type of
> the 'clean' local in handle_content_merge().

> @@ -2184,7 +2184,8 @@ static int handle_content_merge(struct merge_options *opt,
>   		free(result_buf.ptr);
>   		if (ret)
>   			return -1;
> -		clean &= (merge_status == 0);
> +		if (merge_status > 0)
> +			clean = 0;
>   		path_msg(opt, INFO_AUTO_MERGING, 1, path, NULL, NULL, NULL,
>   			 _("Auto-merging %s"), path);
>   	} else if (S_ISGITLINK(a->mode)) {

Even after this removal of this cute bitflip, there is one more
subtle use still in the code:

diff --git a/merge-ort.c b/merge-ort.c
index 8dfe80f1009..569014eef31 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -2629,7 +2629,8 @@ static char *check_for_directory_rename(struct 
merge_options *opt,
  	new_path = handle_path_level_conflicts(opt, path, side_index,
  					       rename_info,
  					       &collisions[side_index]);
-	*clean_merge &= (new_path != NULL);
+	if (*clean_merge && !new_path)
+		*clean_merge = 0;

  	return new_path;
  }

I had to think very carefully about this cleverness to be sure
that this conversion is right (and I'm only mostly sure). When
(new_path != NULL) is false, then we definitely set *clean_merge
to zero. Otherwise, we set it to 1 (but only when it was already
1 or -1). Technically, this does change the behavior by not
squashing -1 into 1, but that is less likely to be an existing
value of *clean_merge.

There are other uses of "clean &= collect_renames(...)" but that
appears to be fine because collect_renames() never results in an
abort state (returning -1).

Thanks,
-Stolee

[PATCH v2 4/7] merge-ort: clearer propagation of failure-to-function from merge_submodule

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

The 'clean' member variable is somewhat of a tri-state (1 = clean, 0 =
conflicted, -1 = failure-to-determine), but we often like to think of
it as binary (ignoring the possibility of a negative value) and use
constructs like '!clean' to reflect this.  However, these constructs
can make codepaths more difficult to understand, unless we handle the
negative case early and return pre-emptively; do that in
handle_content_merge() to make the code a bit easier to read.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-ort.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/merge-ort.c b/merge-ort.c
index be0f5bc3838..d187c966c6a 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -2193,6 +2193,8 @@ static int handle_content_merge(struct merge_options *opt,
 		clean = merge_submodule(opt, pathnames[0],
 					two_way ? null_oid() : &o->oid,
 					&a->oid, &b->oid, &result->oid);
+		if (clean < 0)
+			return -1;
 		if (opt->priv->call_depth && two_way && !clean) {
 			result->mode = o->mode;
 			oidcpy(&result->oid, &o->oid);
-- 
gitgitgadget

Re: [PATCH v2 4/7] merge-ort: clearer propagation of failure-to-function from merge_submodule

[Derrick Stolee]

On 6/18/24 11:00 PM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> The 'clean' member variable is somewhat of a tri-state (1 = clean, 0 =
> conflicted, -1 = failure-to-determine), but we often like to think of
> it as binary (ignoring the possibility of a negative value) and use
> constructs like '!clean' to reflect this.  However, these constructs
> can make codepaths more difficult to understand, unless we handle the
> negative case early and return pre-emptively; do that in
> handle_content_merge() to make the code a bit easier to read.

This patch is correct and valuable.

Would it be valuable to go a bit further and turn 'clean' into
an enum that reflects these states? Perhaps that would prevent
future changes from slipping into this mistake.

Thanks,
-Stolee

Re: [PATCH v2 4/7] merge-ort: clearer propagation of failure-to-function from merge_submodule

[Elijah Newren]

On Thu, Jun 27, 2024 at 7:12 PM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 6/18/24 11:00 PM, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@gmail.com>
> >
> > The 'clean' member variable is somewhat of a tri-state (1 = clean, 0 =
> > conflicted, -1 = failure-to-determine), but we often like to think of
> > it as binary (ignoring the possibility of a negative value) and use
> > constructs like '!clean' to reflect this.  However, these constructs
> > can make codepaths more difficult to understand, unless we handle the
> > negative case early and return pre-emptively; do that in
> > handle_content_merge() to make the code a bit easier to read.
>
> This patch is correct and valuable.
>
> Would it be valuable to go a bit further and turn 'clean' into
> an enum that reflects these states? Perhaps that would prevent
> future changes from slipping into this mistake.

That may make sense to investigate, but I suspect it may be a bigger
change and would recommend making such a clean up a separate series.

Also, I'm curious if it makes sense to finish off replacing recursive
with ort first; as long as recursive exists, it has the same problem
and in fact was the source of using a tri-state 'clean' variable and
thus would need the same cleanup.  But if we replace recursive with
ort (making explicit requests for 'recursive' be handled by 'ort', as
originally designed and intended), that cuts the number of sites
needing this cleanup in half.

Re: [PATCH v2 4/7] merge-ort: clearer propagation of failure-to-function from merge_submodule

[Derrick Stolee]

On 6/27/24 10:38 PM, Elijah Newren wrote:
> On Thu, Jun 27, 2024 at 7:12 PM Derrick Stolee <stolee@gmail.com> wrote:
>>
>> On 6/18/24 11:00 PM, Elijah Newren via GitGitGadget wrote:
>>> From: Elijah Newren <newren@gmail.com>
>>>
>>> The 'clean' member variable is somewhat of a tri-state (1 = clean, 0 =
>>> conflicted, -1 = failure-to-determine), but we often like to think of
>>> it as binary (ignoring the possibility of a negative value) and use
>>> constructs like '!clean' to reflect this.  However, these constructs
>>> can make codepaths more difficult to understand, unless we handle the
>>> negative case early and return pre-emptively; do that in
>>> handle_content_merge() to make the code a bit easier to read.
>>
>> This patch is correct and valuable.
>>
>> Would it be valuable to go a bit further and turn 'clean' into
>> an enum that reflects these states? Perhaps that would prevent
>> future changes from slipping into this mistake.
> 
> That may make sense to investigate, but I suspect it may be a bigger
> change and would recommend making such a clean up a separate series.
> 
> Also, I'm curious if it makes sense to finish off replacing recursive
> with ort first; as long as recursive exists, it has the same problem
> and in fact was the source of using a tri-state 'clean' variable and
> thus would need the same cleanup.  But if we replace recursive with
> ort (making explicit requests for 'recursive' be handled by 'ort', as
> originally designed and intended), that cuts the number of sites
> needing this cleanup in half.

Your fast response to this message means that I didn't see this when
I mentioned it in my closing of the review (in response to your
cover letter).

Reducing the size of the conversion would definitely be good to do,
and then you could also consider using the existing ll_merge_result
enum, though it technically has four states.

Thanks,
-Stolee

[PATCH v2 5/7] merge-ort: loosen commented requirements

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

The comment above type_short_descriptions claimed that the order had to
match what was found in the conflict_info_and_types enum.  Since
type_short_descriptions uses designated initializers, the order should
not actually matter; I am guessing that positional initializers may have
been under consideration when that comment was added, but the comment
was not updated when designated initializers were chosen.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-ort.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/merge-ort.c b/merge-ort.c
index d187c966c6a..d0b13463283 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -553,7 +553,7 @@ enum conflict_and_info_types {
  * Short description of conflict type, relied upon by external tools.
  *
  * We can add more entries, but DO NOT change any of these strings.  Also,
- * Order MUST match conflict_info_and_types.
+ * please ensure the order matches what is used in conflict_info_and_types.
  */
 static const char *type_short_descriptions[] = {
 	/*** "Simple" conflicts and informational messages ***/
-- 
gitgitgadget

[PATCH v2 6/7] merge-ort: upon merge abort, only show messages causing the abort

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

When something goes wrong enough that we need to abort early and not
even attempt merging the remaining files, it probably does not make
sense to report conflicts messages for the subset of files we processed
before hitting the fatal error.  Instead, only show the messages
associated with paths where we hit the fatal error.  Also, print these
messages to stderr rather than stdout.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-ort.c | 78 ++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 53 insertions(+), 25 deletions(-)

diff --git a/merge-ort.c b/merge-ort.c
index d0b13463283..b337e4d74ef 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -543,10 +543,24 @@ enum conflict_and_info_types {
 	CONFLICT_SUBMODULE_HISTORY_NOT_AVAILABLE,
 	CONFLICT_SUBMODULE_MAY_HAVE_REWINDS,
 	CONFLICT_SUBMODULE_NULL_MERGE_BASE,
-	CONFLICT_SUBMODULE_CORRUPT,
+
+	/* INSERT NEW ENTRIES HERE */
+
+	/*
+	 * Keep this entry after all regular conflict and info types; only
+	 * errors (failures causing immediate abort of the merge) should
+	 * come after this.
+	 */
+	NB_REGULAR_CONFLICT_TYPES,
+
+	/*
+	 * Something is seriously wrong; cannot even perform merge;
+	 * Keep this group _last_ other than NB_TOTAL_TYPES
+	 */
+	ERROR_SUBMODULE_CORRUPT,
 
 	/* Keep this entry _last_ in the list */
-	NB_CONFLICT_TYPES,
+	NB_TOTAL_TYPES,
 };
 
 /*
@@ -597,8 +611,10 @@ static const char *type_short_descriptions[] = {
 		"CONFLICT (submodule may have rewinds)",
 	[CONFLICT_SUBMODULE_NULL_MERGE_BASE] =
 		"CONFLICT (submodule lacks merge base)",
-	[CONFLICT_SUBMODULE_CORRUPT] =
-		"CONFLICT (submodule corrupt)"
+
+	/* Something is seriously wrong; cannot even perform merge */
+	[ERROR_SUBMODULE_CORRUPT] =
+		"ERROR (submodule corrupt)",
 };
 
 struct logical_conflict_info {
@@ -762,7 +778,8 @@ static void path_msg(struct merge_options *opt,
 
 	/* Sanity checks */
 	assert(omittable_hint ==
-	       !starts_with(type_short_descriptions[type], "CONFLICT") ||
+	       (!starts_with(type_short_descriptions[type], "CONFLICT") &&
+		!starts_with(type_short_descriptions[type], "ERROR")) ||
 	       type == CONFLICT_DIR_RENAME_SUGGESTED);
 	if (opt->record_conflict_msgs_as_headers && omittable_hint)
 		return; /* Do not record mere hints in headers */
@@ -1817,9 +1834,9 @@ static int merge_submodule(struct merge_options *opt,
 	/* check whether both changes are forward */
 	ret2 = repo_in_merge_bases(&subrepo, commit_o, commit_a);
 	if (ret2 < 0) {
-		path_msg(opt, CONFLICT_SUBMODULE_CORRUPT, 0,
+		path_msg(opt, ERROR_SUBMODULE_CORRUPT, 0,
 			 path, NULL, NULL, NULL,
-			 _("Failed to merge submodule %s "
+			 _("error: failed to merge submodule %s "
 			   "(repository corrupt)"),
 			 path);
 		ret = -1;
@@ -1828,9 +1845,9 @@ static int merge_submodule(struct merge_options *opt,
 	if (ret2 > 0)
 		ret2 = repo_in_merge_bases(&subrepo, commit_o, commit_b);
 	if (ret2 < 0) {
-		path_msg(opt, CONFLICT_SUBMODULE_CORRUPT, 0,
+		path_msg(opt, ERROR_SUBMODULE_CORRUPT, 0,
 			 path, NULL, NULL, NULL,
-			 _("Failed to merge submodule %s "
+			 _("error: failed to merge submodule %s "
 			   "(repository corrupt)"),
 			 path);
 		ret = -1;
@@ -1848,9 +1865,9 @@ static int merge_submodule(struct merge_options *opt,
 	/* Case #1: a is contained in b or vice versa */
 	ret2 = repo_in_merge_bases(&subrepo, commit_a, commit_b);
 	if (ret2 < 0) {
-		path_msg(opt, CONFLICT_SUBMODULE_CORRUPT, 0,
+		path_msg(opt, ERROR_SUBMODULE_CORRUPT, 0,
 			 path, NULL, NULL, NULL,
-			 _("Failed to merge submodule %s "
+			 _("error: failed to merge submodule %s "
 			   "(repository corrupt)"),
 			 path);
 		ret = -1;
@@ -1867,9 +1884,9 @@ static int merge_submodule(struct merge_options *opt,
 	}
 	ret2 = repo_in_merge_bases(&subrepo, commit_b, commit_a);
 	if (ret2 < 0) {
-		path_msg(opt, CONFLICT_SUBMODULE_CORRUPT, 0,
+		path_msg(opt, ERROR_SUBMODULE_CORRUPT, 0,
 			 path, NULL, NULL, NULL,
-			 _("Failed to merge submodule %s "
+			 _("error: failed to merge submodule %s "
 			   "(repository corrupt)"),
 			 path);
 		ret = -1;
@@ -1901,9 +1918,9 @@ static int merge_submodule(struct merge_options *opt,
 					 &merges);
 	switch (parent_count) {
 	case -1:
-		path_msg(opt, CONFLICT_SUBMODULE_CORRUPT, 0,
+		path_msg(opt, ERROR_SUBMODULE_CORRUPT, 0,
 			 path, NULL, NULL, NULL,
-			 _("Failed to merge submodule %s "
+			 _("error: failed to merge submodule %s "
 			   "(repository corrupt)"),
 			 path);
 		ret = -1;
@@ -4646,6 +4663,7 @@ void merge_display_update_messages(struct merge_options *opt,
 	struct hashmap_iter iter;
 	struct strmap_entry *e;
 	struct string_list olist = STRING_LIST_INIT_NODUP;
+	FILE *o = stdout;
 
 	if (opt->record_conflict_msgs_as_headers)
 		BUG("Either display conflict messages or record them as headers, not both");
@@ -4662,6 +4680,10 @@ void merge_display_update_messages(struct merge_options *opt,
 	}
 	string_list_sort(&olist);
 
+	/* Print to stderr if we hit errors rather than just conflicts */
+	if (result->clean < 0)
+		o = stderr;
+
 	/* Iterate over the items, printing them */
 	for (int path_nr = 0; path_nr < olist.nr; ++path_nr) {
 		struct string_list *conflicts = olist.items[path_nr].util;
@@ -4669,25 +4691,31 @@ void merge_display_update_messages(struct merge_options *opt,
 			struct logical_conflict_info *info =
 				conflicts->items[i].util;
 
+			/* On failure, ignore regular conflict types */
+			if (result->clean < 0 &&
+			    info->type < NB_REGULAR_CONFLICT_TYPES)
+				continue;
+
 			if (detailed) {
-				printf("%lu", (unsigned long)info->paths.nr);
-				putchar('\0');
+				fprintf(o, "%lu", (unsigned long)info->paths.nr);
+				fputc('\0', o);
 				for (int n = 0; n < info->paths.nr; n++) {
-					fputs(info->paths.v[n], stdout);
-					putchar('\0');
+					fputs(info->paths.v[n], o);
+					fputc('\0', o);
 				}
-				fputs(type_short_descriptions[info->type],
-				      stdout);
-				putchar('\0');
+				fputs(type_short_descriptions[info->type], o);
+				fputc('\0', o);
 			}
-			puts(conflicts->items[i].string);
+			fputs(conflicts->items[i].string, o);
+			fputc('\n', o);
 			if (detailed)
-				putchar('\0');
+				fputc('\0', o);
 		}
 	}
 	string_list_clear(&olist, 0);
 
-	print_submodule_conflict_suggestion(&opti->conflicted_submodules);
+	if (result->clean >= 0)
+		print_submodule_conflict_suggestion(&opti->conflicted_submodules);
 
 	/* Also include needed rename limit adjustment now */
 	diff_warn_rename_limit("merge.renamelimit",
-- 
gitgitgadget

[PATCH v2 7/7] merge-ort: convert more error() cases to path_msg()

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

merge_submodule() stores errors using path_msg(), whereas other call
sites make use of the error() function.  This is inconsistent, and
moving towards path_msg() seems more friendly for libification efforts
since it will allow the caller to determine whether the error messages
need to be printed.

Note that this deferred handling of error messages changes the error
message in a recursive merge from
  error: failed to execute internal merge
to
  From inner merge:  error: failed to execute internal merge
which provides a little more information about the error which may be
useful.  Since the recursive merge strategy still only shows the older
error, we had to adjust the new testcase introduced a few commits ago to
just search for the older message somewhere in the output.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-ort.c           | 53 +++++++++++++++++++++++++++++++++----------
 t/t6406-merge-attr.sh |  2 +-
 2 files changed, 42 insertions(+), 13 deletions(-)

diff --git a/merge-ort.c b/merge-ort.c
index b337e4d74ef..8dfe80f1009 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -558,6 +558,10 @@ enum conflict_and_info_types {
 	 * Keep this group _last_ other than NB_TOTAL_TYPES
 	 */
 	ERROR_SUBMODULE_CORRUPT,
+	ERROR_THREEWAY_CONTENT_MERGE_FAILED,
+	ERROR_OBJECT_WRITE_FAILED,
+	ERROR_OBJECT_READ_FAILED,
+	ERROR_OBJECT_NOT_A_BLOB,
 
 	/* Keep this entry _last_ in the list */
 	NB_TOTAL_TYPES,
@@ -615,6 +619,14 @@ static const char *type_short_descriptions[] = {
 	/* Something is seriously wrong; cannot even perform merge */
 	[ERROR_SUBMODULE_CORRUPT] =
 		"ERROR (submodule corrupt)",
+	[ERROR_THREEWAY_CONTENT_MERGE_FAILED] =
+		"ERROR (three-way content merge failed)",
+	[ERROR_OBJECT_WRITE_FAILED] =
+		"ERROR (object write failed)",
+	[ERROR_OBJECT_READ_FAILED] =
+		"ERROR (object read failed)",
+	[ERROR_OBJECT_NOT_A_BLOB] =
+		"ERROR (object is not a blob)",
 };
 
 struct logical_conflict_info {
@@ -2190,15 +2202,24 @@ static int handle_content_merge(struct merge_options *opt,
 					  pathnames, extra_marker_size,
 					  &result_buf);
 
-		if ((merge_status < 0) || !result_buf.ptr)
-			ret = error(_("failed to execute internal merge"));
+		if ((merge_status < 0) || !result_buf.ptr) {
+			path_msg(opt, ERROR_THREEWAY_CONTENT_MERGE_FAILED, 0,
+				 pathnames[0], pathnames[1], pathnames[2], NULL,
+				 _("error: failed to execute internal merge for %s"),
+				 path);
+			ret = -1;
+		}
 
 		if (!ret &&
 		    write_object_file(result_buf.ptr, result_buf.size,
-				      OBJ_BLOB, &result->oid))
-			ret = error(_("unable to add %s to database"), path);
-
+				      OBJ_BLOB, &result->oid)) {
+			path_msg(opt, ERROR_OBJECT_WRITE_FAILED, 0,
+				 pathnames[0], pathnames[1], pathnames[2], NULL,
+				 _("error: unable to add %s to database"), path);
+			ret = -1;
+		}
 		free(result_buf.ptr);
+
 		if (ret)
 			return -1;
 		if (merge_status > 0)
@@ -3577,18 +3598,26 @@ static int sort_dirs_next_to_their_children(const char *one, const char *two)
 		return c1 - c2;
 }
 
-static int read_oid_strbuf(const struct object_id *oid,
-			   struct strbuf *dst)
+static int read_oid_strbuf(struct merge_options *opt,
+			   const struct object_id *oid,
+			   struct strbuf *dst,
+			   const char *path)
 {
 	void *buf;
 	enum object_type type;
 	unsigned long size;
 	buf = repo_read_object_file(the_repository, oid, &type, &size);
-	if (!buf)
-		return error(_("cannot read object %s"), oid_to_hex(oid));
+	if (!buf) {
+		path_msg(opt, ERROR_OBJECT_READ_FAILED, 0,
+			 path, NULL, NULL, NULL,
+			 _("error: cannot read object %s"), oid_to_hex(oid));
+		return -1;
+	}
 	if (type != OBJ_BLOB) {
 		free(buf);
-		return error(_("object %s is not a blob"), oid_to_hex(oid));
+		path_msg(opt, ERROR_OBJECT_NOT_A_BLOB, 0,
+			 path, NULL, NULL, NULL,
+			 _("error: object %s is not a blob"), oid_to_hex(oid));
 	}
 	strbuf_attach(dst, buf, size, size + 1);
 	return 0;
@@ -3612,8 +3641,8 @@ static int blob_unchanged(struct merge_options *opt,
 	if (oideq(&base->oid, &side->oid))
 		return 1;
 
-	if (read_oid_strbuf(&base->oid, &basebuf) ||
-	    read_oid_strbuf(&side->oid, &sidebuf))
+	if (read_oid_strbuf(opt, &base->oid, &basebuf, path) ||
+	    read_oid_strbuf(opt, &side->oid, &sidebuf, path))
 		goto error_return;
 	/*
 	 * Note: binary | is used so that both renormalizations are
diff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh
index b6db5c2cc36..9bf95249347 100755
--- a/t/t6406-merge-attr.sh
+++ b/t/t6406-merge-attr.sh
@@ -295,7 +295,7 @@ test_expect_success !WINDOWS 'custom merge driver that is killed with a signal o
 	>./please-abort &&
 	echo "* merge=custom" >.gitattributes &&
 	test_expect_code 2 git merge recursive-a 2>err &&
-	grep "^error: failed to execute internal merge" err &&
+	grep "error: failed to execute internal merge" err &&
 	git ls-files -u >output &&
 	git diff --name-only HEAD >>output &&
 	test_must_be_empty output
-- 
gitgitgadget

Re: [PATCH v2 7/7] merge-ort: convert more error() cases to path_msg()

[Jeff King]

On Wed, Jun 19, 2024 at 03:00:19AM +0000, Elijah Newren via GitGitGadget wrote:

> +static int read_oid_strbuf(struct merge_options *opt,
> +			   const struct object_id *oid,
> +			   struct strbuf *dst,
> +			   const char *path)
>  {
>  	void *buf;
>  	enum object_type type;
>  	unsigned long size;
>  	buf = repo_read_object_file(the_repository, oid, &type, &size);
> -	if (!buf)
> -		return error(_("cannot read object %s"), oid_to_hex(oid));
> +	if (!buf) {
> +		path_msg(opt, ERROR_OBJECT_READ_FAILED, 0,
> +			 path, NULL, NULL, NULL,
> +			 _("error: cannot read object %s"), oid_to_hex(oid));
> +		return -1;
> +	}
>  	if (type != OBJ_BLOB) {
>  		free(buf);
> -		return error(_("object %s is not a blob"), oid_to_hex(oid));
> +		path_msg(opt, ERROR_OBJECT_NOT_A_BLOB, 0,
> +			 path, NULL, NULL, NULL,
> +			 _("error: object %s is not a blob"), oid_to_hex(oid));
>  	}
>  	strbuf_attach(dst, buf, size, size + 1);
>  	return 0;

This loses the early return in the "type != OBJ_BLOB" code path. So we
free(buf), but then continue on to the strbuf_attach() call on the
dangling pointer. Should it "return -1" like the earlier conditional?

-Peff

Re: [PATCH v2 7/7] merge-ort: convert more error() cases to path_msg()

[Elijah Newren]

On Tue, Jul 2, 2024 at 2:33 PM Jeff King <peff@peff.net> wrote:
>
> On Wed, Jun 19, 2024 at 03:00:19AM +0000, Elijah Newren via GitGitGadget wrote:
>
> > +static int read_oid_strbuf(struct merge_options *opt,
> > +                        const struct object_id *oid,
> > +                        struct strbuf *dst,
> > +                        const char *path)
> >  {
> >       void *buf;
> >       enum object_type type;
> >       unsigned long size;
> >       buf = repo_read_object_file(the_repository, oid, &type, &size);
> > -     if (!buf)
> > -             return error(_("cannot read object %s"), oid_to_hex(oid));
> > +     if (!buf) {
> > +             path_msg(opt, ERROR_OBJECT_READ_FAILED, 0,
> > +                      path, NULL, NULL, NULL,
> > +                      _("error: cannot read object %s"), oid_to_hex(oid));
> > +             return -1;
> > +     }
> >       if (type != OBJ_BLOB) {
> >               free(buf);
> > -             return error(_("object %s is not a blob"), oid_to_hex(oid));
> > +             path_msg(opt, ERROR_OBJECT_NOT_A_BLOB, 0,
> > +                      path, NULL, NULL, NULL,
> > +                      _("error: object %s is not a blob"), oid_to_hex(oid));
> >       }
> >       strbuf_attach(dst, buf, size, size + 1);
> >       return 0;
>
> This loses the early return in the "type != OBJ_BLOB" code path. So we
> free(buf), but then continue on to the strbuf_attach() call on the
> dangling pointer. Should it "return -1" like the earlier conditional?
>
> -Peff

Oops!  That's embarrassing.  Thanks for catching; I'll send in a patch
on top since this is already in next.

Re: [PATCH v2 7/7] merge-ort: convert more error() cases to path_msg()

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> Oops!  That's embarrassing.  Thanks for catching; I'll send in a patch
> on top since this is already in next.

Thanks.

Re: [PATCH v2 7/7] merge-ort: convert more error() cases to path_msg()

[Jeff King]

On Wed, Jul 03, 2024 at 08:48:59AM -0700, Elijah Newren wrote:

> > >       if (type != OBJ_BLOB) {
> > >               free(buf);
> > > -             return error(_("object %s is not a blob"), oid_to_hex(oid));
> > > +             path_msg(opt, ERROR_OBJECT_NOT_A_BLOB, 0,
> > > +                      path, NULL, NULL, NULL,
> > > +                      _("error: object %s is not a blob"), oid_to_hex(oid));
> > >       }
> > >       strbuf_attach(dst, buf, size, size + 1);
> > >       return 0;
> >
> > This loses the early return in the "type != OBJ_BLOB" code path. So we
> > free(buf), but then continue on to the strbuf_attach() call on the
> > dangling pointer. Should it "return -1" like the earlier conditional?
> 
> Oops!  That's embarrassing.  Thanks for catching; I'll send in a patch
> on top since this is already in next.

Yeah, I only caught it because Coverity flagged it when it hit 'next'.
It is quite subtle. :)

-Peff

Re: [PATCH v2 0/7] Fix and improve some error codepaths in merge-ort

[Derrick Stolee]

On 6/18/24 11:00 PM, Elijah Newren via GitGitGadget wrote:
> This series started as a just a fix for the abort hit in merge-ort when
> custom merge drivers error out (see
> https://lore.kernel.org/git/75F8BD12-7743-4863-B4C5-049FDEC4645E@gearset.com/).
> However, while working on that, I found a few other issues around error
> codepaths in merge-ort. So this series:
> 
>   * Patches 1-2: fix the reported abort problem
>   * Patches 3-4: make code in handle_content_merges() easier to handle when
>     we hit errors
>   * Patch 5: fix a misleading comment
>   * Patches 6-7: make error handling (immediate print vs. letting callers get
>     the error information) more consistent

I saw this in Junio's email about series requiring review, so I took a
look despite missing v1. Stepping through each commit in my local copy
helped make sure that these changes were correct in their proper context.

> The last two patches change the behavior slightly for error codepaths, and
> there's a question about whether we should show only the error messages that
> caused an early termination of the merge, or if we should also show any
> conflict messages for other paths that were handled before we hit the early
> termination. These patches made a decision but feel free to take those last
> two patches as more of an RFC.

I also support this change in the final two patches.

One thing I mentioned in an earlier reply was "why not use an
enum in this tri-state 'clean' variable?" and then I tried to
make just such a patch. There were two problems:

  1. There were hundreds of changes that would be difficult to
     do in small bits (but maybe doable).

  2. There is already an 'enum ll_merge_result' in merge-ll.h
     that is silently passed back from merge_3way() in merge-ort.c.
     While it's technically four states, maybe it would suffice to
     use that enum instead of creating a new one.

But I'll leave that as something to think about another time. It
does not change the merit of this series. I left a note about
another "&=" case that wasn't touched, but it's not wrong as-is.

Thanks,
-Stolee

Re: [PATCH v2 0/7] Fix and improve some error codepaths in merge-ort

[Junio C Hamano]

Derrick Stolee <stolee@gmail.com> writes:

> I saw this in Junio's email about series requiring review, so I took a
> look despite missing v1. Stepping through each commit in my local copy
> helped make sure that these changes were correct in their proper context.
>
>> The last two patches change the behavior slightly for error codepaths, and
>> there's a question about whether we should show only the error messages that
>> caused an early termination of the merge, or if we should also show any
>> conflict messages for other paths that were handled before we hit the early
>> termination. These patches made a decision but feel free to take those last
>> two patches as more of an RFC.
>
> I also support this change in the final two patches.
>
> One thing I mentioned in an earlier reply was "why not use an
> enum in this tri-state 'clean' variable?" and then I tried to
> make just such a patch. ...
> ... But I'll leave that as something to think about another time. It
> does not change the merit of this series. I left a note about
> another "&=" case that wasn't touched, but it's not wrong as-is.

Thanks for having done a thorough review, including exploration of
an alternative.  Really appreciated.