On 9/23/2021 2:23 PM, Junio C Hamano wrote:
> Derrick Stolee <stolee@xxxxxxxxx> writes:
>
>> On 9/23/2021 9:39 AM, Derrick Stolee wrote:
>>> On 9/22/2021 7:13 PM, Junio C Hamano wrote:
>>>> "Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>>>>
>>>>> +static int path_matches_dir_pattern(const char *pathname,
>>>>> + int pathlen,
>>>>> + int *dtype,
>>>>> + struct path_pattern *pattern,
>>>>> + struct index_state *istate)
>>>>> +{
>>>>> + *dtype = resolve_dtype(*dtype, istate, pathname, pathlen);
>>>>> + if (*dtype != DT_DIR)
>>>>> + return 0;
>>>>> +
>>>>> + return 1;
>>>>> +}
>>>>
>>>> The function name and parameter list have "pattern" but as far as I
>>>> can see any "matches" or "pattern" comes into the picture. The code
>>>> in the caller after calling this function may be doing pattern
>>>> matching, but not this one.
>>>>
>>>> What this helper is doing is "signal if the pathname in the working
>>>> tree is supposed to be a directory with the return value, while
>>>> filling *dtype with what kind of thing it is."
>>>>
>>>> path_must_be_dir_in_working_tree() or something, perhaps?
>>>
>>> Yes, a rename would be prudent here. Thanks.
>>
>> Of course, when I go to amend the commit, the commit message says
>>
>> We will expand the path_matches_dir_pattern() method in a following
>> change.
>>
>> which means that more will follow that will actually care about the
>> pattern and matching as a directory.
>>
>> After looking at the extension in the next patch, do you still think a
>> rename is necessary?
>
> When the focus and purpose of the function changes, it may warrant a
> rename to include "matching" or "pattern", but not before.
>
> Or we might be seeing a premature refactoring with these two steps.
> Are we gaining multiple callers of this function before it gets
> extended to care about pattern and matching? If not, perhaps
> teaching the inlined codepath about the pattern and matching in
> place first before extracting the code to a helper function for
> readability and reusability may help make the resulting series
> easier to follow, and we do not have to see a function with a
> misleading name.
Squashing these two patches together has the same effect, but
takes a little bit extra work to see that the re-used code is
the same. It's small enough that I don't see that as a huge
hurdle.
Thanks,
-Stolee
