> On 6 Jun 2025, at 10:05 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Aditya Garg <gargaditya08@xxxxxxxx> 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. Ok :). So, you want me to add checks for PLAIN and LOGIN, or the current logic is fine. I'd prefer using the current logic to avoid potential regressions, but its your call. > >> "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. Alright, I'll add it as a comment in the code.
