> On 6 Jun 2025, at 12:29 PM, Aditya Garg <gargaditya08@xxxxxxxx> wrote:
>
>
>
>
>> On 6 Jun 2025, at 12:18 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>> Aditya Garg <gargaditya08@xxxxxxxx> writes:
>>
>>> Might look less ugly, but will result in a compiler warning that this will always
>>> be true if compiled with NO_OPENSSL. If you are fine with that, good. Else tbh
>>> I am out of ideas :(.
>>
>> Sounds like a good place to use NOT_CONSTANT(), it seems?
>
> Ok so I was wrong here. The warning actually comes when we compile
> WITHOUT NO_OPENSSL, since auth_oauthbearer is a function there, and
> will attract waddress warnings,
>
> And NOT_CONSTANT doesn't help here.
So this brings us back to the same problem. Compiled warnings (Waddress).
Shown without NO_OPENSSL and not with OPENSSL, with no other way in my
mind than using macros for the compiler.
Or maybe we can just use the original way, in which it died using a function?
>>
>> if (NOT_CONSTANT(!auth_oauthbearer)) {
>> ... skip the thing ...
>> }
>>
>>
>>>>> server_fill_credential(srvc, cred);
>>>>> curl_easy_setopt(curl, CURLOPT_USERNAME, srvc->user);
>>>>> - curl_easy_setopt(curl, CURLOPT_PASSWORD, srvc->pass);
>>>>> +
>>>>> + if (!srvc->auth_method ||
>>>>> + (strcmp(srvc->auth_method, "XOAUTH2") &&
>>>>> + strcmp(srvc->auth_method, "OAUTHBEARER")))
>>>>> + curl_easy_setopt(curl, CURLOPT_PASSWORD, srvc->pass);
>>>> Can we clarify this part, possibly with an in-code comment?
>>>> "Unless XOAUTH2 or OAUTHBEARER, use the password" sounds a bit
>>>> strange. What about methods other than these two that are not a
>>>> plain simple password authentication? Will we remember extending
>>>> this code when we add yet another one to exclude it like XOAUTH2 and
>>>> OAUTHBEARER are excluded with this patch?
>>
>>> Let me answer this first. CURLOPT_PASSWORD is for plain or login type
>>> authentication, and if srvc->auth_method is not defined, curl's behaviour
>>> defaults to them.
>>
>> Which makes it sound like if (!srvc->auth_method) is enough?
>>
>>> OAUTHBEARER and XOAUTH2 use CURLOPT_XOAUTH2_BEARER
>>> in curl, which can use either of them based on what server says.
>>
>> That is what we can read from the updated code.
>>
>> The question is what happens when the user sets srvc->auth_method to
>> something other than NULL (unused---use plain password), "XOAUTH2"
>> or "OAUTHBEARER".
>>
>> If the answer to that question is ...
>>
>>> Other auth methods
>>> are not supported yet in this code, and this is the reason CRAM_MD5 is supported
>>> by only OpenSSL.
>>
>> ... "with srvc->auth_method set to other methods like CRAM_MD5, the
>> control would never enter this codepath, as they are implemented
>> elsewhere", then I think it would make more sense to write the above
>> like this:
>>
>> if (!srvc->auth_method)
>> curl_easy_setopt(curl, CURLOPT_PASSWORD, srvc->pass);
>> else if (strcmp(srvc->auth_method, "XOAUTH2") &&
>> strcmp(srvc->auth_method, "OAUTHBEARER"))
>> BUG("we only support XOAUTH2 and OAUTHBEARER in this codepath");
>>
>> Or the code is not protecting this code path so control can reach
>> with auth_method set to CRAM_MD5 here (e.g. when built without
>> OpenSSL)? If so, replace BUG("message") with die(_("message"))
>> above.
>>
>> On the other hand, if you are trying to fall back to plain password
>> when other unhandled methods are specified, I would expect that the
>> code to read more like:
>>
>> if (srvc->auth_method &&
>> (!strcmp(srvc->auth_method, "XOAUTH2") ||
>> !strcmp(srvc->auth_method, "OAUTHBEARER")))
>> ;
>> else {
>> if (srvc->auth_method)
>> warning("auth method %s not supported,
>> falling back to plain password",
>> srvc->auth_method);
>> curl_easy_setopt(curl, CURLOPT_PASSWORD, srvc->pass);
>> }
>>
>> I cannot quite tell which one you meant, but I am guessing that the
>> former is the case from your explanation.
>>
>> Thanks.
