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

From: Derrick Stolee <stolee@gmail.com>
Date: Feb 9, 2021, 13:33 UTC
Message-ID: <48a208c2-75fe-773e-aa2a-12a90dd06ddd@gmail.com>
In-Reply-To: <ce2173aa1fb7ac3830bf8856a14558446e16a3dd.1612870326.git.gitgitgadget@gmail.com>

On 2/9/2021 6:32 AM, Elijah Newren via GitGitGadget wrote:

Show 10 quoted lines

> +	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

Show 11 quoted lines

> +	} 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.

Show 17 quoted lines

> +
> +		/* 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