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

[Junio C Hamano <gitster@pobox.com>]

"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.