diff mbox series

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

Message ID d4ba1fccd9145db9b3fe1530881052315cfa16b8.1718310307.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Fix and improve some error codepaths in merge-ort | expand

Commit Message

Elijah Newren June 13, 2024, 8:25 p.m. UTC 
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(-)

Comments

Junio C Hamano June 13, 2024, 9 p.m. UTC | #1 
"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.
Taylor Blau June 13, 2024, 10:52 p.m. UTC | #2 
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
diff mbox series

Patch

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);
 }
 
 /*