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

[Derrick Stolee <stolee@gmail.com>]

On 3/9/2021 5:40 PM, Elijah Newren wrote:
> On Tue, Mar 9, 2021 at 2:21 PM Derrick Stolee <stolee@gmail.com> wrote:
>>
>> On 3/8/2021 7:09 PM, Elijah Newren via GitGitGadget wrote:
>>> @@ -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()?
> 
> Yeah, good flag.  I should add a comment to the commit message about
> how these are still needed in initialize_dir_rename_info() for
> basename-guided directory rename detection...and will also play a role
> in some of the special cases where renames are needed for more than
> three-way content merges.  And add a comment about how by the end of
> the series, relevant_sources will be passed to find_basename_matches()
> so that it can skip over those paths.
 
Ah, so by the end, find_basename_matches() will be coupled to the
relevant_sources set directory instead of relying on the culling
happening earlier? Interesting. It seems to me like it would be
better to have them be less coupled by relying on the culling
before calling find_basename_matches(), but I'll keep reading to
see your good reason for not doing that.

Thanks,
-Stolee