From: Adrian Ratiu <adrian.ratiu@collabora.com>
Date: Fri, 05 Dec 2025 17:25:37 GMT
Subject: Re: [PATCH v5 2/7] builtin/credential-store: move is_rfc3986_unreserved to url.[ch]
Message-ID: <871pl8g7se.fsf@collabora.com>
In-Reply-To: <aTLNLnF_ZLpMnso-@pks.im>

On Fri, 05 Dec 2025, Patrick Steinhardt <ps@pks.im> wrote:
> On Wed, Nov 19, 2025 at 11:10:25PM +0200, Adrian Ratiu wrote:
>> is_rfc3986_unreserved() was moved to credential-store.c and was made
>> static by f89854362c (credential-store: move related functions to
>> credential-store file, 2023-06-06) under a correct assumption, at the
>> time, that it was the only place using it.
>> 
>> However now we need it to apply URL-encoding to submodule names when
>> constructing gitdir paths, to avoid conflicts, so bring it back as a
>> public function exposed via url.h, instead of the old helper path
>> (strbuf), which has nothing to do with 3986 encoding/decoding anymore.
>> 
>> This function will be used by submodule.c in the next commit.
>
> Nit: this statement isn't true anymore :)
>

Indeed, thanks for catching these.

I will fix both nits in v6.

>> diff --git a/url.c b/url.c
>> index 282b12495a..0fb1859b28 100644
>> --- a/url.c
>> +++ b/url.c
>> @@ -3,6 +3,17 @@
>>  #include "strbuf.h"
>>  #include "url.h"
>>  
>> +/*
>> + * The set of unreserved characters as per STD66 (RFC3986) is
>> + * '[A-Za-z0-9-._~]'. These characters are safe to appear in URI
>> + * components without percent-encoding.
>> + */
>> +int is_rfc3986_unreserved(char ch)
>> +{
>> +	return isalnum(ch) ||
>> +		ch == '-' || ch == '_' || ch == '.' || ch == '~';
>> +}
>> +
>>  int is_urlschemechar(int first_flag, int ch)
>>  {
>>  	/*
>
> Nit: the function documentation should rather live in the header file.
>
> Patrick