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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 13, 2024, 21:00 UTC
Message-ID: <xmqqjzisimnj.fsf@gitster.g>
In-Reply-To: <d4ba1fccd9145db9b3fe1530881052315cfa16b8.1718310307.git.gitgitgadget@gmail.com>

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