Re: [PATCH v2 1/8] diffcore-rename: enable filtering possible rename sources

[Derrick Stolee <stolee@gmail.com>]

On 3/8/2021 7:09 PM, Elijah Newren via GitGitGadget wrote:> @@ -1127,9 +1137,10 @@ void diffcore_rename_extended(struct diff_options *options,
>  		/*
>  		 * Cull sources:
>  		 *   - remove ones corresponding to exact renames
> +		 *   - remove ones not found in relevant_sources
>  		 */
>  		trace2_region_enter("diff", "cull after exact", options->repo);
> -		remove_unneeded_paths_from_src(want_copies);
> +		remove_unneeded_paths_from_src(want_copies, relevant_sources);
>  		trace2_region_leave("diff", "cull after exact", options->repo);

In this case, we are checking for copies. Perhaps there is a reason
why we want to include relevant_sources in that case, so I'll look
for it later.

>  	} else {
>  		/* Determine minimum score to match basenames */
> @@ -1148,7 +1159,7 @@ void diffcore_rename_extended(struct diff_options *options,
>  		 *   - remove ones involved in renames (found via exact match)
>  		 */
>  		trace2_region_enter("diff", "cull after exact", options->repo);
> -		remove_unneeded_paths_from_src(want_copies);
> +		remove_unneeded_paths_from_src(want_copies, NULL);
>  		trace2_region_leave("diff", "cull after exact", options->repo);
>  
>  		/* Preparation for basename-driven matching. */
> @@ -1167,9 +1178,10 @@ void diffcore_rename_extended(struct diff_options *options,
>  		/*
>  		 * Cull sources, again:
>  		 *   - remove ones involved in renames (found via basenames)
> +		 *   - remove ones not found in relevant_sources
>  		 */
>  		trace2_region_enter("diff", "cull basename", options->repo);
> -		remove_unneeded_paths_from_src(want_copies);
> +		remove_unneeded_paths_from_src(want_copies, relevant_sources);
>  		trace2_region_leave("diff", "cull basename", options->repo);

This seems backwards from your cover letter. You are using the exact renames
_and_ basename matches to remove the unneeded paths. Why are we not stripping
out the relevant_sources in the call further up, before we call
find_basename_matches()?

Thanks,
-Stolee