git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Aditya Garg <gargaditya08@live.com>
Date
Jun 6, 2025, 16:50 UTC
Message-ID
<PN3PR01MB959762A91208AEF089D3302BB86EA@PN3PR01MB9597.INDPRD01.PROD.OUTLOOK.COM>
In-Reply-To
<xmqqmsak3jwp.fsf@gitster.g>
Show 10 quoted lines
> On 6 Jun 2025, at 10:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Aditya Garg <gargaditya08@live.com> 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
Show 26 quoted lines
> 
> --- * ------ * ------ * ------ * ------ * ---
> 
> $ 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.
Show 34 quoted lines
> 
> 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.

Show 150 quoted lines
> 
>    } 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;
Previous: Junio C HamanoNext: Junio C Hamano
Message 4 of 7 in “Re: [PATCH v13 02/10] imap-send: add support for OAuth2.0 authentication”
  1. Aditya GargJun 6, 2025
  2. Aditya GargJun 6, 2025
  3. Junio C HamanoJun 6, 2025
  4. Aditya GargJun 6, 2025
  5. Junio C HamanoJun 6, 2025
  6. Aditya GargJun 6, 2025
  7. Aditya GargJun 6, 2025

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.