threads / patch / 61631

patch, 7 partsmerge-ort: extract handling of priv member into reusable function

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

## tl;dr

34 messages between Jun 13, 2024 and Jul 6, 2024. Diffs are folded; open one to read it.

replies: 33people: 6as markdown or json

Elijah Newren via GitGitGadget· Jun 13, 2024, 20:25 UTC · lore

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

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
Elijah Newren via GitGitGadget· Jun 13, 2024, 20:25 UTC · re: Elijah Newren via GitGitGadget · lore
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(-)
Show changes to merge-ort.c +23 −5
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
Junio C Hamano· Jun 13, 2024, 21:00 UTC · re: Elijah Newren via GitGitGadget · lore

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

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 36 quoted lines
> +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.
Taylor Blau· Jun 13, 2024, 22:52 UTC · re: Junio C Hamano · lore

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

On Thu, Jun 13, 2024 at 02:00:00PM -0700, Junio C Hamano wrote:
Show 49 quoted lines
> "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

Elijah Newren via GitGitGadget· Jun 13, 2024, 20:25 UTC · re: Elijah Newren via GitGitGadget · lore

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

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(-)
Show changes to 2 files +43 −2

merge-ort.c, t/t6406-merge-attr.sh

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
Taylor Blau· Jun 13, 2024, 22:59 UTC · re: Elijah Newren via GitGitGadget · lore

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

On Thu, Jun 13, 2024 at 08:25:02PM +0000, Elijah Newren via GitGitGadget wrote:
Show 11 quoted lines
> 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/ ?
Show 8 quoted lines
> 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.
Show 61 quoted lines
> 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

Elijah Newren· Jun 19, 2024, 02:58 UTC · re: Taylor Blau · lore

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

On Thu, Jun 13, 2024 at 10:59 PM Taylor Blau <me@ttaylorr.com> wrote:
Show 15 quoted lines
>
> 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.
Show 8 quoted lines
> > 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.
Show 5 quoted lines
> 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.

Elijah Newren via GitGitGadget· Jun 13, 2024, 20:25 UTC · re: Elijah Newren via GitGitGadget · lore

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

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(-)
Show changes to merge-ort.c +3 −2
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
Elijah Newren via GitGitGadget· Jun 13, 2024, 20:25 UTC · re: Elijah Newren via GitGitGadget · lore

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

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(+)
Show changes to merge-ort.c +2 −0
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
Elijah Newren via GitGitGadget· Jun 13, 2024, 20:25 UTC · re: Elijah Newren via GitGitGadget · lore

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

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(-)
Show changes to merge-ort.c +1 −1
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
Elijah Newren via GitGitGadget· Jun 13, 2024, 20:25 UTC · re: Elijah Newren via GitGitGadget · lore

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

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(-)
Show changes to merge-ort.c +52 −24
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
Eric Sunshine· Jun 14, 2024, 04:19 UTC · re: Elijah Newren via GitGitGadget · lore

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

On Thu, Jun 13, 2024 at 4:25 PM Elijah Newren via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 18 quoted lines
> 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...

Show 5 quoted lines
> +       ERROR_SUBMODULE_CORRUPT,
>
>         /* Keep this entry _last_ in the list */
> -       NB_CONFLICT_TYPES,
> +       NB_TOTAL_TYPES,
... but NB_CONFLICT_TYPES gets removed here.
Show 6 quoted lines
> @@ -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?

Elijah Newren· Jun 19, 2024, 02:58 UTC · re: Eric Sunshine · lore

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

On Fri, Jun 14, 2024 at 4:19 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 42 quoted lines
>
> 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.

Elijah Newren via GitGitGadget· Jun 13, 2024, 20:25 UTC · re: Elijah Newren via GitGitGadget · lore

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

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(-)
Show changes to 2 files +42 −13

merge-ort.c, t/t6406-merge-attr.sh

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
Taylor Blau· Jun 13, 2024, 23:04 UTC · re: Elijah Newren via GitGitGadget · lore

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

On Thu, Jun 13, 2024 at 08:25:00PM +0000, Elijah Newren via GitGitGadget wrote:
Show 13 quoted lines
> 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

Elijah Newren via GitGitGadget· Jun 19, 2024, 03:00 UTC · re: Elijah Newren via GitGitGadget · lore

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

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
Elijah Newren via GitGitGadget· Jun 19, 2024, 03:00 UTC · re: Elijah Newren via GitGitGadget · lore

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

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(-)
Show changes to merge-ort.c +22 −5
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
Elijah Newren via GitGitGadget· Jun 19, 2024, 03:00 UTC · re: Elijah Newren via GitGitGadget · lore

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

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(-)
Show changes to 2 files +43 −2

merge-ort.c, t/t6406-merge-attr.sh

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
Derrick Stolee· Jun 28, 2024, 02:09 UTC · re: Elijah Newren via GitGitGadget · lore

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

On 6/18/24 11:00 PM, Elijah Newren via GitGitGadget wrote:
Show 15 quoted lines
> 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.
Show 7 quoted lines
> @@ -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.

Show 8 quoted lines
> @@ -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

Elijah Newren via GitGitGadget· Jun 19, 2024, 03:00 UTC · re: Elijah Newren via GitGitGadget · lore

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

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(-)
Show changes to merge-ort.c +3 −2
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
Derrick Stolee· Jun 28, 2024, 02:44 UTC · re: Elijah Newren via GitGitGadget · lore

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

On 6/18/24 11:00 PM, Elijah Newren via GitGitGadget wrote:
Show 10 quoted lines
> 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().
Show 10 quoted lines
> @@ -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:

Show changes to merge-ort.c +2 −2
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
Elijah Newren via GitGitGadget· Jun 19, 2024, 03:00 UTC · re: Elijah Newren via GitGitGadget · lore

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

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(+)
Show changes to merge-ort.c +2 −0
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
Derrick Stolee· Jun 28, 2024, 02:12 UTC · re: Elijah Newren via GitGitGadget · lore

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

On 6/18/24 11:00 PM, Elijah Newren via GitGitGadget wrote:
Show 9 quoted lines
> 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

Elijah Newren· Jun 28, 2024, 02:38 UTC · re: Derrick Stolee · lore

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

On Thu, Jun 27, 2024 at 7:12 PM Derrick Stolee <stolee@gmail.com> wrote:
Show 17 quoted lines
>
> 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.

Derrick Stolee· Jun 28, 2024, 02:47 UTC · re: Elijah Newren · lore

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

On 6/27/24 10:38 PM, Elijah Newren wrote:
Show 29 quoted lines
> 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

Elijah Newren via GitGitGadget· Jun 19, 2024, 03:00 UTC · re: Elijah Newren via GitGitGadget · lore

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

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(-)
Show changes to merge-ort.c +1 −1
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
Elijah Newren via GitGitGadget· Jun 19, 2024, 03:00 UTC · re: Elijah Newren via GitGitGadget · lore

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

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(-)
Show changes to merge-ort.c +53 −25
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
Elijah Newren via GitGitGadget· Jun 19, 2024, 03:00 UTC · re: Elijah Newren via GitGitGadget · lore

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

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(-)
Show changes to 2 files +42 −13

merge-ort.c, t/t6406-merge-attr.sh

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
Jeff King· Jul 2, 2024, 21:33 UTC · re: Elijah Newren via GitGitGadget · lore

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

On Wed, Jun 19, 2024 at 03:00:19AM +0000, Elijah Newren via GitGitGadget wrote:
Show 26 quoted lines
> +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
Elijah Newren· Jul 3, 2024, 15:48 UTC · re: Jeff King · lore

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

On Tue, Jul 2, 2024 at 2:33 PM Jeff King <peff@peff.net> wrote:
Show 35 quoted lines
>
> 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.

Junio C Hamano· Jul 3, 2024, 18:35 UTC · re: Elijah Newren · lore

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

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.
Jeff King· Jul 6, 2024, 06:11 UTC · re: Elijah Newren · lore

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

On Wed, Jul 03, 2024 at 08:48:59AM -0700, Elijah Newren wrote:
Show 16 quoted lines
> > >       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
Derrick Stolee· Jun 28, 2024, 02:45 UTC · re: Elijah Newren via GitGitGadget · lore

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

On 6/18/24 11:00 PM, Elijah Newren via GitGitGadget wrote:
Show 12 quoted lines
> 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.

Show 6 quoted lines
> 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

Junio C Hamano· Jun 28, 2024, 17:11 UTC · re: Derrick Stolee · lore

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

Derrick Stolee <stolee@gmail.com> writes:
Show 19 quoted lines
> 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.

← back to recent threads