Re: [PATCH v2 12/13] merge-ort: handle interactions of caching and rename/rename(1to1) cases

[Derrick Stolee <stolee@gmail.com>]

On 5/3/21 10:12 PM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> As documented in Documentation/technical/remembering-renames.txt, and as
> tested for in the two testcases in t6429 with "rename same file
> identically" in their description, there is one case where we need to
> have renames in one commit NOT be cached for the next commit in our
> rebase sequence -- namely, rename/rename(1to1) cases.  Rather than
> specifically trying to uncache those and fix up dir_rename_counts() to
> match (which would also be valid but more work), we simply disable the
> optimization when this really rare type of rename occurs.
> 
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  merge-ort.c | 30 +++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/merge-ort.c b/merge-ort.c
> index 741970cd05e7..2fc98b803d1c 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -2100,6 +2100,9 @@ static int process_renames(struct merge_options *opt,
>  			VERIFY_CI(side2);
>  
>  			if (!strcmp(pathnames[1], pathnames[2])) {
> +				struct rename_info *ri = &opt->priv->renames;
> +				int j;
> +
>  				/* Both sides renamed the same way */
>  				assert(side1 == side2);
>  				memcpy(&side1->stages[0], &base->stages[0],
> @@ -2109,6 +2112,16 @@ static int process_renames(struct merge_options *opt,
>  				base->merged.is_null = 1;
>  				base->merged.clean = 1;
>  
> +				/*
> +				 * Disable remembering renames optimization;
> +				 * rename/rename(1to1) is incredibly rare, and
> +				 * just disabling the optimization is easier
> +				 * than purging cached_pairs,
> +				 * cached_target_names, and dir_rename_counts.
> +				 */
> +				for (j = 0; j < 3; j++)
> +					ri->merge_trees[j] = NULL;
> +

nit: it might be helpful to initialize ri closer to when it will
be used, just to have the value of 'ri' be easier to find when
looking at this loop.

> @@ -3896,7 +3909,22 @@ static void merge_check_renames_reusable(struct merge_options *opt,
>  
>  	renames = &opti->renames;
>  	merge_trees = renames->merge_trees;
> -	/* merge_trees[0..2] will only be NULL if opti is */
> +
> +	/*
> +	 * Handle case where previous merge operation did not want cache to
> +	 * take effect, e.g. because rename/rename(1to1) makes it invalid.
> +	 */
> +	if (!merge_trees[0]) {
> +		assert(!merge_trees[0] && !merge_trees[1] && !merge_trees[2]);
> +		renames->cached_pairs_valid_side = 0; /* neither side valid */
> +		return;
> +	}
> +
> +	/*
> +	 * Handle other cases; note that merge_trees[0..2] will only
> +	 * be NULL if opti is, or if all three were manually set to
> +	 * NULL by e.g. rename/rename(1to1) handling.
> +	 */
>  	assert(merge_trees[0] && merge_trees[1] && merge_trees[2]);

Thanks,
-Stolee