Re: [PATCH v3 05/12] refs: move submodule slash stripping code to get_submodule_ref_store

From: Duy Nguyen <pclouds@gmail.com>
Date: Apr 20, 2017, 11:56 UTC
Message-ID: <CACsJy8A1aR-=QqqZdw+i5Hv2kXptTzYcn5tFsMBDYk6vBwUwZg@mail.gmail.com>
In-Reply-To: <a74cf309-fb16-2f45-8189-d1d0c655dea4@kdbg.org>

On Thu, Apr 20, 2017 at 5:02 AM, Johannes Sixt <j6t@kdbg.org> wrote:

Show 19 quoted lines

> Am 19.04.2017 um 13:01 schrieb Nguyễn Thái Ngọc Duy:
>>
>> @@ -1558,7 +1543,17 @@ struct ref_store *get_submodule_ref_store(const
>> char *submodule)
>>  {
>>         struct strbuf submodule_sb = STRBUF_INIT;
>>         struct ref_store *refs;
>> +       char *to_free = NULL;
>>         int ret;
>> +       size_t len;
>> +
>> +       if (submodule) {
>> +               len = strlen(submodule);
>> +               while (len && submodule[len - 1] == '/')
>
>
> What is the source of the value of 'submodule'? Is it an index entry? Or did
> it pass through parse_pathspec? In these cases it is correct to compare
> against literal '/'. Otherwise, is_dir_sep() is preferred.

I've looked at the callers. Yes it is a path and is_dir_sep() should be used. Since this has been like this in the current code, unless there's some more changes in this series or you insist, I would hold this change back, wait for this series to settle and submit it later (I'll have to do that anyway to kill the get_main_store() call in this function).

-- 
Duy