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

[Junio C Hamano <gitster@pobox.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?