On Fri, Apr 21, 2017 at 10:27 PM, Michael Haggerty <mhagger@xxxxxxxxxxxx> wrote:
> 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@xxxxxxxxx>
>> ---
>> 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?
In a nearby thread we discussed whether we want to tighten the
submodule names and paths as some of the code path do funny
things on funny input. As 'submodule' here refers to a path (and not
a name), I would think '///' is not possible/buggy, rather we'd want to
discuss if we want to extend the check to is_dir_sep, which would
include '\' on Windows.
Thanks,
Stefan