On 2/9/2021 12:17 PM, Elijah Newren wrote: > On Tue, Feb 9, 2021 at 5:25 AM Derrick Stolee <stolee@xxxxxxxxx> 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. > 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
