Re: [PATCH v2 3/4] diffcore-rename: guide inexact rename detection based on basenames

[Derrick Stolee <stolee@gmail.com>]

On 2/9/2021 6:32 AM, Elijah Newren via GitGitGadget wrote:
> +	num_sources = rename_src_nr;
> +
> +	if (want_copies || break_idx) {
> +		/*
> +		 * Cull sources:
> +		 *   - remove ones corresponding to exact renames
> +		 */
> +		trace2_region_enter("diff", "cull after exact", options->repo);
> +		remove_unneeded_paths_from_src(want_copies);
> +		trace2_region_leave("diff", "cull after exact", options->repo);

Isn't this the same as

> +	} else {
> +		/* Determine minimum score to match basenames */
> +		int min_basename_score = (int)(5*minimum_score + 0*MAX_SCORE)/5;
> +
> +		/*
> +		 * Cull sources:
> +		 *   - remove ones involved in renames (found via exact match)
> +		 */
> +		trace2_region_enter("diff", "cull exact", options->repo);
> +		remove_unneeded_paths_from_src(want_copies);
> +		trace2_region_leave("diff", "cull exact", options->repo);

...this? (except the regions are renamed)

Could this be simplified as:

+	num_sources = rename_src_nr;
+
+	trace2_region_enter("diff", "cull after exact", options->repo);
+	remove_unneeded_paths_from_src(want_copies);
+	trace2_region_leave("diff", "cull after exact", options->repo);
+
+	if (!want_copies && !break_idx) {
+		/* Determine minimum score to match basenames */

I realize you probably changed the region names on purpose to distinguish
that there are two "cull" regions in the case of no copies, but I think
that isn't really worth different names. Better to have a consistent region
name around the same activity in both cases.

> +		int min_basename_score = (int)(5*minimum_score + 0*MAX_SCORE)/5;

Did you intend for this to be 5*min + 0*MAX? This seems wrong if you want
this value to be different from minimum_score.

> +
> +		/* Utilize file basenames to quickly find renames. */
> +		trace2_region_enter("diff", "basename matches", options->repo);
> +		rename_count += find_basename_matches(options,
> +						      min_basename_score,
> +						      rename_src_nr);
> +		trace2_region_leave("diff", "basename matches", options->repo);
> +
> +		/*
> +		 * Cull sources, again:
> +		 *   - remove ones involved in renames (found via basenames)
> +		 */
> +		trace2_region_enter("diff", "cull basename", options->repo);
> +		remove_unneeded_paths_from_src(want_copies);
> +		trace2_region_leave("diff", "cull basename", options->repo);
> +	}
> +

Thanks,
-Stolee