Re: [PATCH v2 2/4] diffcore-rename: complete find_basename_matches()

From: Derrick Stolee <stolee@gmail.com>
Date: Feb 9, 2021, 17:34 UTC
Message-ID: <d0154183-371f-5ada-f4dd-dab7b3267c8c@gmail.com>
In-Reply-To: <CABPp-BEz7ZUUUPF6EkK0RzWq5O=kYbNFFtg=iMbk607kQ-nmbg@mail.gmail.com>

On 2/9/2021 12:17 PM, Elijah Newren wrote:

Show 11 quoted lines

> On Tue, Feb 9, 2021 at 5:25 AM Derrick Stolee <stolee@gmail.com> wrote:
>>
>> On 2/9/2021 6:32 AM, Elijah Newren via GitGitGadget wrote:
>>> +             /* Get the basename */
>>> +             base = strrchr(filename, '/');
>>> +             base = (base ? base+1 : filename);
>>
>> Here is the third instance of this in the same function. At minimum we should
>> extract a helper for you to consume.
> 
> Where by "this" you mean these last two lines, right?

Correct. The reason to use a helper is to ease cognitive load when reading the code. These lines are identical and serve the same purpose. By making a "get_basename()" helper and using it as

	base = get_basename(filename);

makes it easy to understand what is happening without needing to think carefully about it. For example, I had to remember that strrchr() returns NULL when '/' is not found, not the first character of the string.

Show 12 quoted lines

> And perhaps explain why I'm not using either basename(3) or
> gitbasename() from git-compat-util.h?  (The latter of which I just
> learned about while responding to the review of this patch.)
> 
> or maybe gitbasename can do the job, but the skip_dos_drive_prefix()
> and the munging of the string passed in both worry me.  And the
> is_dir_sep() looks inefficient since I know I'm dealing with filenames
> as stored in git internally, and thus can only use '/' characters.
> Hmm...
> 
> Yeah, I think I'll add my own helper in this file, since you want one,
> and just use it.

Right. I almost made a point to say "Don't use find_last_dir_sep()" because it uses platform-specific directory separators. Your helper is based on in-memory representation that always uses Unix-style paths.

Thanks, -Stolee