Re: [PATCH 1/1] imap-send: include strbuf.h

From: Taylor Blau <me@ttaylorr.com>
Date: May 17, 2023, 16:31 UTC
Message-ID: <ZGUBdoLeiqTQYQ9Z@nand.local>
In-Reply-To: <xmqqilcrq6a9.fsf@gitster.g>

On Wed, May 17, 2023 at 09:19:58AM -0700, Junio C Hamano wrote:

Show 8 quoted lines

> OK, so the fix seems to make sense, but the justification for the
> change needs to be rewritten, I think.
>
>     We make liberal use of the strbuf API functions and types, but
>     the inclusion of <strbuf.h> comes indirectly by including
>     <http.h>, which does not happen if you build with NO_CURL.
>
> or something like that?

Agreed. Here's a patch that summarizes my investigation. Thanks again, Christian, for reporting!

--- 8< ---

Subject: [PATCH] imap-send.c: fix compilation under NO_CURL and ancient
 versions

When building imap-send.c with an ancient (pre-7.34.0, which was released in 2013) version of curl, or with `NO_CURL` defined, Git 2.41.0-rc0 fails to compile imap-send.c, i.e.

    $ make NO_CURL=1 imap-send.o

This is similar to 52c0f3318d (run-command.c: fix missing include under `NO_PTHREADS`, 2023-05-16), but the bisection points at ba3d1c73da (treewide: remove unnecessary cache.h includes, 2023-02-24) instead.

The trivial fix is to include "strbuf.h" unconditionally, which is a noop for most builds, and saves us otherwise.

To check for other *.c files that might suffer from the same issue, we can run:

    git grep -l -e '[^_]xstrdup(' -e 'strbuf_[a-z0-9A-Z_]*(' \*.c |
    while read f
    do
        if ! gcc -I $(pwd) -E $f | grep -q 'struct strbuf {'
        then
            echo "==> $f NOT OK";
        fi
    done

...which runs the preprocessor on (roughly) all C source files that call `xstrdup()` or use the strbuf API. The resulting list looks (on my machine) lile:

    ==> compat/fsmonitor/fsm-listen-darwin.c NOT OK
    ==> compat/mingw.c NOT OK
    ==> contrib/credential/osxkeychain/git-credential-osxkeychain.c NOT OK
    ==> pager.c NOT OK
    ==> refs/iterator.c NOT OK
    ==> refs/ref-cache.c NOT OK
    ==> string-list.c NOT OK
    ==> t/helper/test-mktemp.c NOT OK

The ones in compat are OK to ignore since they both fail to compile on my non-Windows machine (I am missing the `<dispatch/dispatch.h>` and `<windows.h>` headers, respectively).

The one in contrib is fine to ignore, since it has its own definition of xstrdup().

pager.c is OK, since it only needs xstrdup(), not any other parts of the strbuf API. It gets a declaration of xstrdup() from git-compat-util.h refs/iterator.c, refs/ref-cache.c, string-list.c, and t/helper/test-mktemp.c are all OK for the same reason.

So this is the only spot that needs fixing.

Reported-by: Christian Hesse <mail@eworm.de>
Original-fix-by: Christian Hesse <mail@eworm.de>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 imap-send.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/imap-send.c b/imap-send.c
index a62424e90a..7f5426177a 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -29,6 +29,7 @@
 #include "run-command.h"
 #include "parse-options.h"
 #include "setup.h"
+#include "strbuf.h"
 #include "wrapper.h"
 #if defined(NO_OPENSSL) && !defined(HAVE_OPENSSL_CSPRNG)
 typedef void *SSL;
--
2.41.0.rc0.1.g01bd045298