Re: [PATCH v3 2/6] worktree.c: find_worktree() learns to identify worktrees by basename

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 2, 2016, 16:49 UTC
Message-ID: <xmqqfusvv8lq.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CACsJy8B+j2im7XOV==tBtki=tOCN4k3ZHz6Jp4fq4qjqarb+ew@mail.gmail.com>

Duy Nguyen <pclouds@gmail.com> writes:

Show 16 quoted lines

> On Thu, Jun 2, 2016 at 1:44 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> We would
>>> need to convert or match both '/' and '\' in "to/foo" case because of
>>> Windows, so it's not much easier than basename().
>>
>> I never said "easier to implement".  But can this codepath get
>> backslashed paths in the first place?  I somehow thought that
>> normalization would happen a lot before the control reaches here.
>>
>> You'll be calling into fspathcmp() anyway; shouldn't the function
>> know that '/' and '\' are equivalent on some platforms, or is it
>> legal to only call fspathcmp() on a single path component without
>> directory separator?
>
> We still need to calculate the length to compare, which could be
> problematic when utf-8 is involved, or some other encoding.

Hmph. I was unaware that fspathcmp() used here does more than byte-for-byte processing, which would cause problems due to encoding issues when you hand code the comparison.

+static struct worktree *find_worktree_by_basename(struct worktree **list, + const char *base_name) +{ + struct worktree *found = NULL; + int nr_found = 0; + + for (; *list && nr_found < 2; list++) { + char *path = xstrdup((*list)->path); + if (!fspathcmp(base_name, basename(path))) { + found = *list; + nr_found++; + } + free(path); + } + return nr_found == 1 ? found : NULL; +}

> If we always split at '/' boundary though (e.g. "abc/def/ghi",
> "def/ghi" or "ghi" but never "ef/ghi") then it should be ok.

Does "basename()" used here know '/' and '\' can both be a directory separator, or does worktree->path have a normalized representation of the path, i.e. '/' is the only directory separator?