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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 5, 2025, 16:33 UTC
Message-ID: <xmqqy0u687ww.fsf@gitster.g>
In-Reply-To: <0d28e337cfe7ce3c52490544875474b3502d2081.1749112640.git.gargaditya08@live.com>

Aditya Garg <gargaditya08@live.com> writes:

Show 13 quoted lines

> +			} 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.

Show 8 quoted lines

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