Subject: Re: [PATCH v13 02/10] imap-send: add support for OAuth2.0 authentication

From: Junio C Hamano <[email-masked]>
Date: Thu, 05 Jun 2025 21:35:28 -0700

Aditya Garg <[email-masked]> writes:

>>Which makes it sound like if (!srvc->auth_method) is enough?
>>
>
> No. If the user specifies PLAIN or LOGIN then it's not enough.

That invites another question.  Why aren't we checking for PLAIN or
LOGIN and when one of them is given use the password?  What is
written in the patch looks backwards, in that we seem to assume that
a method that is not XOAUTH2 and OAUTHBEARER must be PLAIN or LOGIN
(or anything that wants us to pass CURLOPT_PASSWORD).  IOW, why
isn't the code more like

	if (/* not using the more advanced method interface? */
	    !srvc->auth_method ||
	    /* method interface that takes password? */
	    !strcmp(srvc->auth_method, "PLAIN")	||
	    !strcmp(srvc->auth_method, "LOGIN"))
		curl_easy_setopt(curl, CURLOPT_PASSWORD, srvc->pass);

if, after all, PLAIN/LOGIN are what triggers password
authentication?

> So, considering the fact that the original code for imap-send
> was setting CURLOPT_LOGIN_OPTIONS
> unconditionally and
> was running the AUTH command even if auth was set to CRAM-MD5
> or whatever, I just preferred to not change that behaviour since I
> may cause some regression. There is a tiny possibility that CRAM-MD5
> may work, but I don't really have any free SMTP server which uses
> that method itself.
>
> In short, just to be very safe here, I decided to not mingle with the
> logic much and simple decided to use a seperate tested logic
> for OAuth2.0 and let the same logic be used for rest cases.

Don't be short ;-) Be long in your log message to help future
developers.  In short, you want to make your proposed log message so
clear that future developers who found this commit in "git log -p"
to come asking you these questions---that is why reviewers are
supposed to ask questions and ask for clarifications.

> "Set CURLOPT_LOGIN_OPTIONS irrespective of whether there is
> an auth method specified or not, unless it's OAuth2.0, where we
> use a different curl API"

That is a very good thing to write down either in-code comment
and/or the log message to avoid future developers come bugging you
with the same questions as I did.

Thanks.