Re: [PATCH v2 14/16] pathspec: create strip submodule slash helpers

From: Duy Nguyen <pclouds@gmail.com>
Date: Dec 9, 2016, 12:16 UTC
Message-ID: <CACsJy8AX6C8Tux9_8ZynBMNS2EW2pKQOGK8k0hVmbWvbZ8pa=Q@mail.gmail.com>
In-Reply-To: <xmqqfulyrlmf.fsf@gitster.mtv.corp.google.com>

On Fri, Dec 9, 2016 at 7:28 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 17 quoted lines

> Brandon Williams <bmwill@google.com> writes:
>
>> +static void strip_submodule_slash_cheap(struct pathspec_item *item)
>> +{
>> +     int i;
>> +
>> +     if ((item->len >= 1 && item->match[item->len - 1] == '/') &&
>> +         (i = cache_name_pos(item->match, item->len - 1)) >= 0 &&
>> +         S_ISGITLINK(active_cache[i]->ce_mode)) {
>> +             item->len--;
>> +             item->match[item->len] = '\0';
>> +     }
>> +}
>
> I know that this is merely a moved code, but while I was reading
> this, it triggered "Do not make an assignment inside if condition"
> check.

Yeah with a function of its own, it's probably better to separate that assignment.

> But more importantly, is the code even correct?  If the path
> for the submodule is unmerged, we would get a negative i that points
> at the conflicting entry; don't we want to do something about it, at
> least when we have a submodule entry at stage #2 (i.e. ours)?

In my defense I was simply moving (again!) the code from strip_trailing_slash_from_submodules() in b69bb3f:builtin/ls-files.c. Could be an improvement point for submodule people, I guess.

-- 
Duy