| Message ID | 20230517070632.71884-1-list@eworm.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 3307f7dde2ae8f5281d0782f7291a073c9b1cdc2 |
| Headers | show |
| Series | [1/1] imap-send: include strbuf.h | expand |
Christian Hesse <list@eworm.de> writes: > From: Christian Hesse <mail@eworm.de> > > We use xstrfmt() here, so let's include the header file. > > Signed-off-by: Christian Hesse <mail@eworm.de> > --- > imap-send.c | 1 + > 1 file changed, 1 insertion(+) Puzzled. For me Git 2.41-rc0 builds as-is without this change just fine, it seems. I know there are many header file shuffling patches flying around, and I have seen some of them, but is this a fix for one of these patches? Thanks. > > 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;
On Wed, May 17, 2023 at 08:49:37AM -0700, Junio C Hamano wrote: > Christian Hesse <list@eworm.de> writes: > > > From: Christian Hesse <mail@eworm.de> > > > > We use xstrfmt() here, so let's include the header file. > > > > Signed-off-by: Christian Hesse <mail@eworm.de> > > --- > > imap-send.c | 1 + > > 1 file changed, 1 insertion(+) > > Puzzled. For me Git 2.41-rc0 builds as-is without this change just > fine, it seems. It will fail to build for ancient versions of curl (pre-7.34.0, which was released in 2013), or if you build with `NO_CURL=1`. > I know there are many header file shuffling patches flying around, and > I have seen some of them, but is this a fix for one of these patches? Similar to [1], this bisects to ba3d1c73da (treewide: remove unnecessary cache.h includes, 2023-02-24). Thanks, Taylor [1]: https://lore.kernel.org/git/ZGP2tw0USsj9oecZ@nand.local/
Taylor Blau <me@ttaylorr.com> writes: > On Wed, May 17, 2023 at 08:49:37AM -0700, Junio C Hamano wrote: >> Christian Hesse <list@eworm.de> writes: >> >> > From: Christian Hesse <mail@eworm.de> >> > >> > We use xstrfmt() here, so let's include the header file. >> > >> > Signed-off-by: Christian Hesse <mail@eworm.de> >> > --- >> > imap-send.c | 1 + >> > 1 file changed, 1 insertion(+) >> >> Puzzled. For me Git 2.41-rc0 builds as-is without this change just >> fine, it seems. > > It will fail to build for ancient versions of curl (pre-7.34.0, which > was released in 2013), or if you build with `NO_CURL=1`. xstrfmt() is used at exactly one place, inside "#ifndef NO_OPENSSL", in the implementation of the static function cram(). Ah, the mention of that function was a huge red herring. There are tons of strbuf API calls in the file outside any conditional compilation, and where it inherits the include from is "http.h", that is conditionally included. 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? Thanks.
On Wed, May 17, 2023 at 12:02:04PM -0400, Taylor Blau wrote: > > I know there are many header file shuffling patches flying around, and > > I have seen some of them, but is this a fix for one of these patches? > > Similar to [1], this bisects to ba3d1c73da (treewide: remove unnecessary > cache.h includes, 2023-02-24). I'm looking through other files to see if we have other uses of the strbuf API that are hidden behind a #define without a corresponding #include "strbuf.h". Here's the (gross) script I wrote up to check: 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 Here's the list: ==> 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 I think that this is the only spot we need to worry about. Thanks, Taylor
On Wed, May 17, 2023 at 09:19:58AM -0700, Junio C Hamano wrote: > 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
Taylor Blau <me@ttaylorr.com> writes: > Here's the (gross) script I wrote up to check: > > 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 I am a bit puzzled. What does the above prove, more than what your regular compilation that does not fail, tells us? Doesn't -E expand recursively, so for the case of imap-send.c, with your usual configuration, wouldn't it have grabbed "struct strbuf" via inclusion of <http.h> indirectly anyway? > Here's the list: > > ==> 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 I think that this is the only spot we need to worry about.
Junio C Hamano <gitster@pobox.com> writes: >> if ! gcc -I $(pwd) -E $f | grep -q 'struct strbuf {' > ... > What does the above prove, more than what your regular compilation > that does not fail, tells us? It is actually worse than that, isn't it? This does not even use the definition in the config.mak.uname, so it is not even matching your build environment. I am uncomfortable to use this as an explanation of what due diligence we did to convince ourselves that this fix should cover all similar issues. Perhaps I am grossly misunderstanding what your investigation did? Thanks.
On Wed, May 17, 2023 at 10:01:35AM -0700, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > >> if ! gcc -I $(pwd) -E $f | grep -q 'struct strbuf {' > > ... > > What does the above prove, more than what your regular compilation > > that does not fail, tells us? > > It is actually worse than that, isn't it? This does not even use > the definition in the config.mak.uname, so it is not even matching > your build environment. > > I am uncomfortable to use this as an explanation of what due > diligence we did to convince ourselves that this fix should cover > all similar issues. Perhaps I am grossly misunderstanding what your > investigation did? Oof, yes, you are right: diff -u \ <(gcc -I . -E imap-send.c) \ <(gcc -DNO_CURL=1 -I . -E imap-send.c) How *should* we test this? Thanks, Taylor
On Wednesday, May 17, 2023 1:58 PM, Taylor Blau wrote: >On Wed, May 17, 2023 at 10:01:35AM -0700, Junio C Hamano wrote: >> Junio C Hamano <gitster@pobox.com> writes: >> >> >> if ! gcc -I $(pwd) -E $f | grep -q 'struct strbuf {' >> > ... >> > What does the above prove, more than what your regular compilation >> > that does not fail, tells us? >> >> It is actually worse than that, isn't it? This does not even use the >> definition in the config.mak.uname, so it is not even matching your >> build environment. >> >> I am uncomfortable to use this as an explanation of what due diligence >> we did to convince ourselves that this fix should cover all similar >> issues. Perhaps I am grossly misunderstanding what your investigation >> did? > >Oof, yes, you are right: > > diff -u \ > <(gcc -I . -E imap-send.c) \ > <(gcc -DNO_CURL=1 -I . -E imap-send.c) > >How *should* we test this? I hope not by using gcc, which is not currently a dependency. Using the C preprocessor directly might help in a more general sense, but you probably will need a knob for some compilers to work.
Taylor Blau <me@ttaylorr.com> writes: > On Wed, May 17, 2023 at 10:01:35AM -0700, Junio C Hamano wrote: >> Junio C Hamano <gitster@pobox.com> writes: >> >> >> if ! gcc -I $(pwd) -E $f | grep -q 'struct strbuf {' >> > ... >> > What does the above prove, more than what your regular compilation >> > that does not fail, tells us? >> >> It is actually worse than that, isn't it? This does not even use >> the definition in the config.mak.uname, so it is not even matching >> your build environment. >> >> I am uncomfortable to use this as an explanation of what due >> diligence we did to convince ourselves that this fix should cover >> all similar issues. Perhaps I am grossly misunderstanding what your >> investigation did? > > Oof, yes, you are right: > > diff -u \ > <(gcc -I . -E imap-send.c) \ > <(gcc -DNO_CURL=1 -I . -E imap-send.c) > > How *should* we test this? My inclination is punt and simply do not to claim that we have done a good due diligence to ensure with all permutations of ifdef we are including necessary headers.
<rsbecker@nexbridge.com> writes: >>Oof, yes, you are right: >> >> diff -u \ >> <(gcc -I . -E imap-send.c) \ >> <(gcc -DNO_CURL=1 -I . -E imap-send.c) >> >>How *should* we test this? > > I hope not by using gcc, which is not currently a > dependency. Using the C preprocessor directly might help in a more > general sense, but you probably will need a knob for some > compilers to work. I am not going to suggest trying all permutations of CPP macros to make sure we cover all the #ifdef'ed sections, but -E to show CPP output is pretty common feature not limited to "gcc", so if we were to do that, we'd very likely use the usual $(CC) Makefile macro to invoke such a test.
On Wednesday, May 17, 2023 2:12 PM, Junio C Hamano wrote: ><rsbecker@nexbridge.com> writes: > >>>Oof, yes, you are right: >>> >>> diff -u \ >>> <(gcc -I . -E imap-send.c) \ >>> <(gcc -DNO_CURL=1 -I . -E imap-send.c) >>> >>>How *should* we test this? >> >> I hope not by using gcc, which is not currently a dependency. Using >> the C preprocessor directly might help in a more general sense, but >> you probably will need a knob for some compilers to work. > >I am not going to suggest trying all permutations of CPP macros to make sure we >cover all the #ifdef'ed sections, but -E to show CPP output is pretty common feature >not limited to "gcc", so if we were to do that, we'd very likely use the usual $(CC) >Makefile macro to invoke such a test. -E would work for me, but I do recall other platforms that would not (but can't place them at the moment). No objection from me, but it still might be useful to have a variable, like CPPFLAGS=-E (by default), that might help others, in config.mak.uname.
Junio C Hamano <gitster@pobox.com> on Wed, 2023/05/17 09:19: > Taylor Blau <me@ttaylorr.com> writes: > > > On Wed, May 17, 2023 at 08:49:37AM -0700, Junio C Hamano wrote: > >> Christian Hesse <list@eworm.de> writes: > >> > >> > From: Christian Hesse <mail@eworm.de> > >> > > >> > We use xstrfmt() here, so let's include the header file. > >> > > >> > Signed-off-by: Christian Hesse <mail@eworm.de> > >> > --- > >> > imap-send.c | 1 + > >> > 1 file changed, 1 insertion(+) > >> > >> Puzzled. For me Git 2.41-rc0 builds as-is without this change just > >> fine, it seems. I prepared cgit to build with libgit.a 2.41.0-rc0. While cgit itself builds fine (with some justifications of course), building git for the test suite failed. > > It will fail to build for ancient versions of curl (pre-7.34.0, which > > was released in 2013), or if you build with `NO_CURL=1`. Indeed we have NO_CURL=1 in cgit's Makefile... > xstrfmt() is used at exactly one place, inside "#ifndef NO_OPENSSL", > in the implementation of the static function cram(). > > Ah, the mention of that function was a huge red herring. Well, the warning about implicit declaration of xstrfmt() was this one that popped up... :) Sorry for the confusion. > There are > tons of strbuf API calls in the file outside any conditional > compilation, and where it inherits the include from is "http.h", > that is conditionally included. > > 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? Fine with me! Do you want me to re-send the patch or do you modify this on the fly? > Thanks.
Christian Hesse <list@eworm.de> on Wed, 2023/05/17 22:12: > > 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? > > Fine with me! > Do you want me to re-send the patch or do you modify this on the fly? Found it in next branch already. Thanks a lot!
On Wed, May 17, 2023 at 11:09:16AM -0700, Junio C Hamano wrote: > Taylor Blau <me@ttaylorr.com> writes: > > > On Wed, May 17, 2023 at 10:01:35AM -0700, Junio C Hamano wrote: > >> Junio C Hamano <gitster@pobox.com> writes: > >> > >> >> if ! gcc -I $(pwd) -E $f | grep -q 'struct strbuf {' > >> > ... > >> > What does the above prove, more than what your regular compilation > >> > that does not fail, tells us? > >> > >> It is actually worse than that, isn't it? This does not even use > >> the definition in the config.mak.uname, so it is not even matching > >> your build environment. > >> > >> I am uncomfortable to use this as an explanation of what due > >> diligence we did to convince ourselves that this fix should cover > >> all similar issues. Perhaps I am grossly misunderstanding what your > >> investigation did? > > > > Oof, yes, you are right: > > > > diff -u \ > > <(gcc -I . -E imap-send.c) \ > > <(gcc -DNO_CURL=1 -I . -E imap-send.c) > > > > How *should* we test this? > > My inclination is punt and simply do not to claim that we have done > a good due diligence to ensure with all permutations of ifdef we are > including necessary headers. I think that's the best course of action, too. I see that it's already on 'next', thanks. Thanks, Taylor
Christian Hesse <list@eworm.de> writes: > Christian Hesse <list@eworm.de> on Wed, 2023/05/17 22:12: >> > 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? >> >> Fine with me! >> Do you want me to re-send the patch or do you modify this on the fly? > > Found it in next branch already. Thanks a lot! We made moderate amount of these kind of header shuffling this cycle, and it is inevitable to encounter a gotcha like this when building with anything less than the most common configurations. Thanks for reporting and fixing; very much appreciated.
Taylor Blau <me@ttaylorr.com> writes: >> My inclination is punt and simply do not to claim that we have done >> a good due diligence to ensure with all permutations of ifdef we are >> including necessary headers. > > I think that's the best course of action, too. I see that it's already > on 'next', thanks. Yeah, I am actuall hoping that somebody clever, with time, comes up with a systematic way to give us better coverage, but until then, I think it is better to honestly record where we are to future developers. Thanks.
On Thu, May 18, 2023 at 09:01:19AM -0700, Junio C Hamano wrote: > Taylor Blau <me@ttaylorr.com> writes: > > >> My inclination is punt and simply do not to claim that we have done > >> a good due diligence to ensure with all permutations of ifdef we are > >> including necessary headers. > > > > I think that's the best course of action, too. I see that it's already > > on 'next', thanks. > > Yeah, I am actuall hoping that somebody clever, with time, comes up > with a systematic way to give us better coverage, but until then, I > think it is better to honestly record where we are to future > developers. I faced a similar issue with the -Wunused-parameter patches. Just when I thought I had everything annotated, I'd find some obscure Makefile knob that compiled new code (or even in one case disabled some code that used a variable!). I never came up with a good solution, but relying on CI helped, since it just builds more of the combinations. Obviously that didn't catch this case. We could try to hit more platforms / combinations of knobs in CI, but there are diminishing returns on the compute time. But at least in this case, the old "if it is important, somebody will build it and report the problem" line of thinking worked out. So maybe that is enough. I guess maybe that was more philosophical than helpful. ;) -Peff
Jeff King <peff@peff.net> writes: > On Thu, May 18, 2023 at 09:01:19AM -0700, Junio C Hamano wrote: > >> Yeah, I am actuall hoping that somebody clever, with time, comes up >> with a systematic way to give us better coverage, but until then, I >> think it is better to honestly record where we are to future >> developers. > > I faced a similar issue with the -Wunused-parameter patches. Just when I > thought I had everything annotated, I'd find some obscure Makefile knob > that compiled new code (or even in one case disabled some code that used > a variable!). > ... > But at least in this case, the old "if it is important, somebody will > build it and report the problem" line of thinking worked out. So maybe > that is enough. Ah, I guess the debugging situation is quite similar with that topic. Thanks for your insight.
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;