From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 05 Jun 2025 16:33:51 GMT
Subject: Re: [PATCH v13 02/10] imap-send: add support for OAuth2.0 authentication
Message-ID: <xmqqy0u687ww.fsf@gitster.g>
In-Reply-To: <0d28e337cfe7ce3c52490544875474b3502d2081.1749112640.git.gargaditya08@live.com>

Aditya Garg <gargaditya08@live.com> writes:

> +			} else if (!strcmp(srvc->auth_method, "OAUTHBEARER")) {
> +				if (!CAP(AUTH_OAUTHBEARER)) {
> +					fprintf(stderr, "You specified "
> +						"OAUTHBEARER as authentication method, "
> +						"but %s doesn't support it.\n", srvc->host);
> +					goto bail;
> +				}
> +
> +				#ifdef NO_OPENSSL
> +				fprintf(stderr, "You are trying to use OAUTHBEARER authentication mechanism "
> +					"with OpenSSL library, but its support has not been compiled in.");
> +				goto bail;
> +				#endif

Ugly.  Can we avoid #ifdef/#endif in the middle of such a main flow
of the logic?  Hiding such ugliness by indenting the #ifdef/#endif
directives as if they are just one of the code lines is doubly ugly.

>  	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?