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

> On 04/19/2017 01:01 PM, Nguyễn Thái Ngọc Duy wrote:
>> This is a better place that will benefit all submodule callers instead
>> of just resolve_gitlink_ref()
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>>  refs.c | 33 +++++++++++++++++----------------
>>  1 file changed, 17 insertions(+), 16 deletions(-)
>>
>> diff --git a/refs.c b/refs.c
>> index 5902a3d9e5..26474cb62a 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -1422,25 +1422,10 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
>>  int resolve_gitlink_ref(const char *submodule, const char *refname,
>>                       unsigned char *sha1)
>>  {
>> -     size_t len = strlen(submodule);
>>       struct ref_store *refs;
>>       int flags;
>>
>> -     while (len && submodule[len - 1] == '/')
>> -             len--;
>> -
>> -     if (!len)
>> -             return -1;
>> -
>> -     if (submodule[len]) {
>> -             /* We need to strip off one or more trailing slashes */
>> -             char *stripped = xmemdupz(submodule, len);
>> -
>> -             refs = get_submodule_ref_store(stripped);
>> -             free(stripped);
>> -     } else {
>> -             refs = get_submodule_ref_store(submodule);
>> -     }
>> +     refs = get_submodule_ref_store(submodule);
>>
>>       if (!refs)
>>               return -1;
>> @@ -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] == '/')
>> +                     len--;
>> +             if (!len)
>> +                     submodule = NULL;
>> +     }
>
> Ugh. Should a submodule named "///" *really* be considered to refer to
> the main ref_store? I understand that's what the code did before this
> patch, but it seems to me more like an accident of the old design rather
> than something worth supporting. In other words, if a caller would
> really pass us such a string, it seems like we could declare the caller
> buggy, no?