> On 6 Jun 2025, at 10:08 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Aditya Garg <gargaditya08@xxxxxxxx> writes:
>
>> Ok so I was wrong here. The warning actually comes when we compile
>> WITHOUT NO_OPENSSL, since auth_oauthbearer is a function there, and
>> will attract waddress warnings,
>
> But the patch in this thread, when compiled with NO_OPENSSL, gives
> "-Wunreachable".
Uggh
>
> --- * ------ * ------ * ------ * ------ * ---
>
> $ make V=1 CC=clang DEVELOPER=YesPlease CFLAGS=-Waddress NO_OPENSSL=NoThanks imap-send.o
> * new build flags
> clang -o imap-send.o -c -MF ./.depend/imap-send.o.d -MQ imap-send.o -MMD -MP -Werror -Wall -pedantic -Wpedantic -std=gnu99 -Wdeclaration-after-statement -Wformat-security -Wold-style-definition -Woverflow -Wpointer-arith -Wstrict-prototypes -Wunused -Wvla -Wwrite-strings -fno-common -Wunreachable-code -Wcomma -Wtautological-constant-out-of-range-compare -Wextra -Wmissing-prototypes -Wno-empty-body -Wno-missing-field-initializers -Waddress -I. -DGIT_HOST_CPU="\"x86_64\"" -DHAVE_ALLOCA_H -DUSE_CURL_FOR_IMAP_SEND -DNO_OPENSSL -DSUPPORTS_SIMPLE_IPC -DSHA1_DC -DSHA1DC_NO_STANDARD_INCLUDES -DSHA1DC_INIT_SAFE_HASH_DEFAULT=0 -DSHA1DC_CUSTOM_INCLUDE_SHA1_C="\"git-compat-util.h\"" -DSHA1DC_CUSTOM_INCLUDE_UBC_CHECK_C="\"git-compat-util.h\"" -DSHA256_BLK -DHAVE_PATHS_H -DHAVE_DEV_TTY -DHAVE_CLOCK_GETTIME -DHAVE_CLOCK_MONOTONIC -DHAVE_SYNC_FILE_RANGE -DHAVE_SYSINFO -DHAVE_GETDELIM -DHAVE_GETRANDOM -DFREAD_READS_DIRECTORIES -DNO_STRLCPY -DSHELL_PATH='"/bin/sh"' imap-send.c
> imap-send.c:1344:5: error: code will never be executed [-Werror,-Wunreachable-code]
> 1344 | memset(&cb, 0, sizeof(cb));
> | ^~~~~~
> imap-send.c:1322:5: error: code will never be executed [-Werror,-Wunreachable-code]
> 1322 | memset(&cb, 0, sizeof(cb));
> | ^~~~~~
> imap-send.c:1300:5: error: code will never be executed [-Werror,-Wunreachable-code]
> 1300 | memset(&cb, 0, sizeof(cb));
> | ^~~~~~
> imap-send.c:1278:5: error: code will never be executed [-Werror,-Wunreachable-code]
> 1278 | memset(&cb, 0, sizeof(cb));
> | ^~~~~~
> 4 errors generated.
> make: *** [Makefile:2820: imap-send.o] Error 1
>
> --- * ------ * ------ * ------ * ------ * ---
>
> See the patch attached at the end [*] that you can apply to 'seen'
> to try out; the result compiles with all combinations of gcc/clang
> and with and without NO_OPENSSL.
Great, thanks for the patch. Lemme see if I can make something nice.
>
> Having said that, I think you should first clean up the repetitions
> before piling a lot of different auth_method on top.
>
> A part of the resulting code after applying [*] to 'seen' looks like
> this:
>
> } else if (!strcmp(srvc->auth_method, "CRAM-MD5")) {
> if (!CAP(AUTH_CRAM_MD5)) {
> fprintf(stderr, "You specified "
> "CRAM-MD5 as authentication method, "
> "but %s doesn't support it.\n", srvc->host);
> goto bail;
> }
>
> /* CRAM-MD5 */
> memset(&cb, 0, sizeof(cb));
> cb.cont = auth_cram_md5;
>
> if (NOT_CONSTANT(!cb.cont)) {
> fprintf(stderr, "If you want to use CRAM-MD5 authentication mechanism, "
> "you have to build git-imap-send with OpenSSL library.");
> goto bail;
> }
> if (imap_exec(ctx, &cb, "AUTHENTICATE CRAM-MD5") != RESP_OK) {
> fprintf(stderr, "IMAP error: AUTHENTICATE CRAM-MD5 failed\n");
> goto bail;
> }
>
> This pattern repeats for different auth methods as you add
> copy-and-paste, which made the code way too long and too deeply
> indented to be readable. Why not make the above into a single
> helper function that you can reuse, so that the call site can become
> something like
I was thinking the same but though the code would be more readable
in case if repeats. With GCC, even the binaries were of the same size.
>
> } else if (!strcmp(srvc->auth_method, "PLAIN")) {
> if (try_auth_method(srvc, ctx, "PLAIN", AUTH_PLAIN, auth_plain))
> goto bail;
> } else if (!strcmp(srvc->auth_method, "CRAM-MD5")) {
> if (try_auth_method(srvc, ctx, "CRAM-MD5", AUTH_CRAM_MD5, auth_cram_md5))
> goto bail;
> } ...
>
> If you did it at the very beginning of the series (i.e. when you
> only have CRAM_MD5 you inherited from the original before adding all
> the other new ones), each addition would become a lot simpler and
> main flow in the the resulting code would become quite simple like
> the above.
>
> static int try_auth_method(struct imap_server_conf *srvc,
> struct imap_store *ctx,
> const char *auth_method,
> enum CAPABILITY cap,
> int (*fn)(struct imap_store *, const char *))
> {
> struct imap_cmd_cb cb = {0};
>
> if (!CAP(cap)) {
> fprintf(stderr, "You specified "
> "%s as authentication method, "
> "but %s doesn't support it.\n",
> auth_method, srvc->host);
> return -1;
> }
> cb.cont = fn;
>
> if (NOT_CONSTANT(!cb.cont)) {
> fprintf(stderr, "If you want to use %s authentication mechanism, "
> "you have to build git-imap-send with OpenSSL library.",
> auth_method);
> return -1;
> }
> if (imap_exec(ctx, &cb, "AUTHENTICATE %s", auth_method) != RESP_OK) {
> fprintf(stderr, "IMAP error: AUTHENTICATE %s failed\n",
> auth_method);
> return -1;
> }
> return 0;
> }
>
> So here is that [*] patch that is incorrectly indented to avoid even
> deeper nesting.
>
> ---- >8 ----
>
> imap-send.c | 49 ++++++++++++++++++++++++-------------------------
> 1 file changed, 24 insertions(+), 25 deletions(-)
>
> diff --git i/imap-send.c w/imap-send.c
> index 6dbbf37125..bba2ef0cd0 100644
> --- i/imap-send.c
> +++ w/imap-send.c
> @@ -1267,16 +1267,16 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c
> goto bail;
> }
>
> - #ifdef NO_OPENSSL
> + /* PLAIN */
> + memset(&cb, 0, sizeof(cb));
> + cb.cont = auth_plain;
> +
> + if (NOT_CONSTANT(!cb.cont)) {
> fprintf(stderr, "You are trying to use PLAIN authentication mechanism "
> "with OpenSSL library, but its support has not been compiled in.");
> goto bail;
> - #endif
> -
> - /* PLAIN */
> + }
>
> - memset(&cb, 0, sizeof(cb));
> - cb.cont = auth_plain;
> if (imap_exec(ctx, &cb, "AUTHENTICATE PLAIN") != RESP_OK) {
> fprintf(stderr, "IMAP error: AUTHENTICATE PLAIN failed\n");
> goto bail;
> @@ -1289,16 +1289,16 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c
> goto bail;
> }
>
> - #ifdef NO_OPENSSL
> - fprintf(stderr, "If you want to use CRAM-MD5 authentication mechanism, "
> - "you have to build git-imap-send with OpenSSL library.");
> - goto bail;
> - #endif
>
> /* CRAM-MD5 */
> -
> memset(&cb, 0, sizeof(cb));
> cb.cont = auth_cram_md5;
> +
> + if (NOT_CONSTANT(!cb.cont)) {
> + fprintf(stderr, "If you want to use CRAM-MD5 authentication mechanism, "
> + "you have to build git-imap-send with OpenSSL library.");
> + goto bail;
> + }
> if (imap_exec(ctx, &cb, "AUTHENTICATE CRAM-MD5") != RESP_OK) {
> fprintf(stderr, "IMAP error: AUTHENTICATE CRAM-MD5 failed\n");
> goto bail;
> @@ -1311,16 +1311,15 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c
> 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
> -
> /* OAUTHBEARER */
> -
> memset(&cb, 0, sizeof(cb));
> cb.cont = auth_oauthbearer;
> +
> + if (NOT_CONSTANT(!cb.cont)) {
> + fprintf(stderr, "You are trying to use OAUTHBEARER authentication mechanism "
> + "with OpenSSL library, but its support has not been compiled in.");
> + goto bail;
> + }
> if (imap_exec(ctx, &cb, "AUTHENTICATE OAUTHBEARER") != RESP_OK) {
> fprintf(stderr, "IMAP error: AUTHENTICATE OAUTHBEARER failed\n");
> goto bail;
> @@ -1333,16 +1332,16 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c
> goto bail;
> }
>
> - #ifdef NO_OPENSSL
> + /* XOAUTH2 */
> + memset(&cb, 0, sizeof(cb));
> + cb.cont = auth_xoauth2;
> +
> + if (NOT_CONSTANT(!cb.cont)) {
> fprintf(stderr, "You are trying to use XOAUTH2 authentication mechanism "
> "with OpenSSL library, but its support has not been compiled in.");
> goto bail;
> - #endif
> -
> - /* XOAUTH2 */
> + }
>
> - memset(&cb, 0, sizeof(cb));
> - cb.cont = auth_xoauth2;
> if (imap_exec(ctx, &cb, "AUTHENTICATE XOAUTH2") != RESP_OK) {
> fprintf(stderr, "IMAP error: AUTHENTICATE XOAUTH2 failed\n");
> goto bail;
