Thread (4 messages) 4 messages, 4 authors, 2023-05-18

Re: [PATCH] run-command.c: fix missing include under `NO_PTHREADS`

From: Junio C Hamano <hidden>
Date: 2023-05-16 19:12:11

Possibly related (same subject, not in this thread)

Taylor Blau [off-list ref] writes:
When building git with `NO_PTHREADS=YesPlease`, we fail to build
run-command.o since we don't have a definition for ALLOC_GROW:

    $ make NO_PTHREADS=1 DEVELOPER=1 run-command.o
    GIT_VERSION = 2.41.0.rc0.1.g787eb3beae.dirty
        CC run-command.o
    run-command.c: In function ‘git_atexit’:
    run-command.c:1103:9: error: implicit declaration of function ‘ALLOC_GROW’ [-Werror=implicit-function-declaration]
     1103 |         ALLOC_GROW(git_atexit_hdlrs.handlers, git_atexit_hdlrs.nr + 1, git_atexit_hdlrs.alloc);
          |         ^~~~~~~~~~
    cc1: all warnings being treated as errors
    make: *** [Makefile:2715: run-command.o] Error 1
I am OK to give a reproduction recipe, i.e. the "$ make" command
line, to make it easy for people, who cannot remember how to define
make variables from the command line, to try out themselves, but I
hate the "build transcript" in the log message, when a few lines of
prose suffices.

How about this?

    Git 2.41-rc0 fails to compile run-command.c with NO_PTHREADS
    defined, i.e.

      $ make NO_PTHREADS=1 run-command.o

    as ALLOC_GROW() macro is used in the atexit() emulation but the
    macro definition is not available.
This bisects to 36bf195890 (alloc.h: move ALLOC_GROW() functions from
cache.h, 2023-02-24), which replaced includes of "cache.h" with
"alloc.h", which is the new home of `ALLOC_GROW()` (and
`ALLOC_GROW_BY()`).
Good.
Insert something like:

    We can see that run-command.c is the only one that try to use
    these macros without including <alloc.h>.

      $ git grep -l -e '[^_]ALLOC_GROW(' -e 'ALLOC_GROW_BY(' \*.c | sort >/tmp/1
      $ git grep -l 'alloc\.h' \*.c | sort >/tmp/2
      $ comm -23 /tmp/[12]
      compat/simple-ipc/ipc-unix-socket.c
      run-command.c

    The "compat/" file only talks about the macro in the comment,
    and is not broken.

here.  What I am aiming for is to tell readers what they do not have
to spend their time on.  As we encourage people to make sure to look
for other errors that come from the same root cause when making a
fix, avoiding duplicated work by telling what they do not have to
look at is rather important.
run-command.c compiles fine when `NO_PTHREADS` is undefined, since its
use of `ALLOC_GROW()` is behind a `#ifndef NO_PTHREADS`. (Everything
else compiles fine when NO_PTHREADS is defined, so this is the only spot
that needs fixing).
I think we can say that, but is probably coered by the three-line
summary I gave above.
We could fix this by conditionally including "alloc.h" when
`NO_PTHREADS` is defined.  But we have relatively few examples of
conditional includes throughout the tree[^1].

Instead, include "alloc.h" unconditionally in run-command.c to allow it
to successfully compile even when NO_PTHREADS is defined.
Good.
[^1]: With `git grep -E -A1 '#if(n)?def' -- **/*.c | grep '#include' -B1`.
Good.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help