* [PATCH 1/1] imap-send: include strbuf.h @ 2023-05-17 7:06 Christian Hesse 2023-05-17 15:49 ` Junio C Hamano 0 siblings, 1 reply; 25+ messages in thread From: Christian Hesse @ 2023-05-17 7:06 UTC (permalink / raw) To: git; +Cc: Christian Hesse 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(+) 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.40.1 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 1/1] imap-send: include strbuf.h 2023-05-17 7:06 [PATCH 1/1] imap-send: include strbuf.h Christian Hesse @ 2023-05-17 15:49 ` Junio C Hamano 2023-05-17 16:02 ` Taylor Blau 0 siblings, 1 reply; 25+ messages in thread From: Junio C Hamano @ 2023-05-17 15:49 UTC (permalink / raw) To: Christian Hesse; +Cc: git, Christian Hesse 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; ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/1] imap-send: include strbuf.h 2023-05-17 15:49 ` Junio C Hamano @ 2023-05-17 16:02 ` Taylor Blau 2023-05-17 16:19 ` Junio C Hamano 2023-05-17 16:23 ` Taylor Blau 0 siblings, 2 replies; 25+ messages in thread From: Taylor Blau @ 2023-05-17 16:02 UTC (permalink / raw) To: Junio C Hamano; +Cc: Christian Hesse, git, Christian Hesse 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/ ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/1] imap-send: include strbuf.h 2023-05-17 16:02 ` Taylor Blau @ 2023-05-17 16:19 ` Junio C Hamano 2023-05-17 16:31 ` Taylor Blau 2023-05-17 20:12 ` Christian Hesse 2023-05-17 16:23 ` Taylor Blau 1 sibling, 2 replies; 25+ messages in thread From: Junio C Hamano @ 2023-05-17 16:19 UTC (permalink / raw) To: Taylor Blau; +Cc: Christian Hesse, git, Christian Hesse 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/1] imap-send: include strbuf.h 2023-05-17 16:19 ` Junio C Hamano @ 2023-05-17 16:31 ` Taylor Blau 2023-05-17 20:12 ` Christian Hesse 1 sibling, 0 replies; 25+ messages in thread From: Taylor Blau @ 2023-05-17 16:31 UTC (permalink / raw) To: Junio C Hamano; +Cc: Christian Hesse, git, Elijah Newren 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 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 1/1] imap-send: include strbuf.h 2023-05-17 16:19 ` Junio C Hamano 2023-05-17 16:31 ` Taylor Blau @ 2023-05-17 20:12 ` Christian Hesse 2023-05-17 20:18 ` Christian Hesse 1 sibling, 1 reply; 25+ messages in thread From: Christian Hesse @ 2023-05-17 20:12 UTC (permalink / raw) To: Junio C Hamano; +Cc: Taylor Blau, git, Christian Hesse [-- Attachment #1: Type: text/plain, Size: 2140 bytes --] 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. -- main(a){char*c=/* Schoene Gruesse */"B?IJj;MEH" "CX:;",b;for(a/* Best regards my address: */=0;b=c[a++];) putchar(b-1/(/* Chris cc -ox -xc - && ./x */b/42*2-3)*42);} [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/1] imap-send: include strbuf.h 2023-05-17 20:12 ` Christian Hesse @ 2023-05-17 20:18 ` Christian Hesse 2023-05-18 15:56 ` Junio C Hamano 0 siblings, 1 reply; 25+ messages in thread From: Christian Hesse @ 2023-05-17 20:18 UTC (permalink / raw) To: Junio C Hamano; +Cc: Taylor Blau, git, Christian Hesse [-- Attachment #1: Type: text/plain, Size: 804 bytes --] 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! -- main(a){char*c=/* Schoene Gruesse */"B?IJj;MEH" "CX:;",b;for(a/* Best regards my address: */=0;b=c[a++];) putchar(b-1/(/* Chris cc -ox -xc - && ./x */b/42*2-3)*42);} [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/1] imap-send: include strbuf.h 2023-05-17 20:18 ` Christian Hesse @ 2023-05-18 15:56 ` Junio C Hamano 0 siblings, 0 replies; 25+ messages in thread From: Junio C Hamano @ 2023-05-18 15:56 UTC (permalink / raw) To: Christian Hesse; +Cc: Taylor Blau, git, Christian Hesse 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/1] imap-send: include strbuf.h 2023-05-17 16:02 ` Taylor Blau 2023-05-17 16:19 ` Junio C Hamano @ 2023-05-17 16:23 ` Taylor Blau 2023-05-17 16:53 ` Junio C Hamano 1 sibling, 1 reply; 25+ messages in thread From: Taylor Blau @ 2023-05-17 16:23 UTC (permalink / raw) To: Junio C Hamano; +Cc: Christian Hesse, git, Christian Hesse 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/1] imap-send: include strbuf.h 2023-05-17 16:23 ` Taylor Blau @ 2023-05-17 16:53 ` Junio C Hamano 2023-05-17 17:01 ` Junio C Hamano 0 siblings, 1 reply; 25+ messages in thread From: Junio C Hamano @ 2023-05-17 16:53 UTC (permalink / raw) To: Taylor Blau; +Cc: Christian Hesse, git, Christian Hesse 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/1] imap-send: include strbuf.h 2023-05-17 16:53 ` Junio C Hamano @ 2023-05-17 17:01 ` Junio C Hamano 2023-05-17 17:58 ` Taylor Blau 0 siblings, 1 reply; 25+ messages in thread From: Junio C Hamano @ 2023-05-17 17:01 UTC (permalink / raw) To: Taylor Blau; +Cc: Christian Hesse, git, Christian Hesse 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/1] imap-send: include strbuf.h 2023-05-17 17:01 ` Junio C Hamano @ 2023-05-17 17:58 ` Taylor Blau 2023-05-17 18:06 ` rsbecker 2023-05-17 18:09 ` Junio C Hamano 0 siblings, 2 replies; 25+ messages in thread From: Taylor Blau @ 2023-05-17 17:58 UTC (permalink / raw) To: Junio C Hamano; +Cc: Christian Hesse, git, Christian Hesse 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* RE: [PATCH 1/1] imap-send: include strbuf.h 2023-05-17 17:58 ` Taylor Blau @ 2023-05-17 18:06 ` rsbecker 2023-05-17 18:12 ` Junio C Hamano 2023-05-17 18:09 ` Junio C Hamano 1 sibling, 1 reply; 25+ messages in thread From: rsbecker @ 2023-05-17 18:06 UTC (permalink / raw) To: 'Taylor Blau', 'Junio C Hamano' Cc: 'Christian Hesse', git, 'Christian Hesse' 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/1] imap-send: include strbuf.h 2023-05-17 18:06 ` rsbecker @ 2023-05-17 18:12 ` Junio C Hamano 2023-05-17 19:30 ` rsbecker 0 siblings, 1 reply; 25+ messages in thread From: Junio C Hamano @ 2023-05-17 18:12 UTC (permalink / raw) To: rsbecker Cc: 'Taylor Blau', 'Christian Hesse', git, 'Christian Hesse' <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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* RE: [PATCH 1/1] imap-send: include strbuf.h 2023-05-17 18:12 ` Junio C Hamano @ 2023-05-17 19:30 ` rsbecker 0 siblings, 0 replies; 25+ messages in thread From: rsbecker @ 2023-05-17 19:30 UTC (permalink / raw) To: 'Junio C Hamano' Cc: 'Taylor Blau', 'Christian Hesse', git, 'Christian Hesse' 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/1] imap-send: include strbuf.h 2023-05-17 17:58 ` Taylor Blau 2023-05-17 18:06 ` rsbecker @ 2023-05-17 18:09 ` Junio C Hamano 2023-05-17 21:38 ` Taylor Blau 1 sibling, 1 reply; 25+ messages in thread From: Junio C Hamano @ 2023-05-17 18:09 UTC (permalink / raw) To: Taylor Blau; +Cc: Christian Hesse, git, Christian Hesse 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/1] imap-send: include strbuf.h 2023-05-17 18:09 ` Junio C Hamano @ 2023-05-17 21:38 ` Taylor Blau 2023-05-18 16:01 ` Junio C Hamano 0 siblings, 1 reply; 25+ messages in thread From: Taylor Blau @ 2023-05-17 21:38 UTC (permalink / raw) To: Junio C Hamano; +Cc: Christian Hesse, git, Christian Hesse 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/1] imap-send: include strbuf.h 2023-05-17 21:38 ` Taylor Blau @ 2023-05-18 16:01 ` Junio C Hamano 2023-05-18 18:25 ` Jeff King 0 siblings, 1 reply; 25+ messages in thread From: Junio C Hamano @ 2023-05-18 16:01 UTC (permalink / raw) To: Taylor Blau; +Cc: Christian Hesse, git, Christian Hesse 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/1] imap-send: include strbuf.h 2023-05-18 16:01 ` Junio C Hamano @ 2023-05-18 18:25 ` Jeff King 2023-05-18 20:49 ` Junio C Hamano 0 siblings, 1 reply; 25+ messages in thread From: Jeff King @ 2023-05-18 18:25 UTC (permalink / raw) To: Junio C Hamano; +Cc: Taylor Blau, Christian Hesse, git, Christian Hesse 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/1] imap-send: include strbuf.h 2023-05-18 18:25 ` Jeff King @ 2023-05-18 20:49 ` Junio C Hamano 0 siblings, 0 replies; 25+ messages in thread From: Junio C Hamano @ 2023-05-18 20:49 UTC (permalink / raw) To: Jeff King; +Cc: Taylor Blau, Christian Hesse, git, Christian Hesse 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 1/1] imap-send: include strbuf.h @ 2024-02-09 22:26 Christian Hesse 2024-02-09 22:42 ` Junio C Hamano 0 siblings, 1 reply; 25+ messages in thread From: Christian Hesse @ 2024-02-09 22:26 UTC (permalink / raw) To: Git Mailing List; +Cc: Christian Hesse From: Christian Hesse <mail@eworm.de> We had this fixed in 3307f7dde2ae8f5281d0782f7291a073c9b1cdc2, and it broke again in eea0e59ffbed6e33d171ace5be13cde9faa41639. 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. This time make the include conditional... Does that prevent from loosing it again? Signed-off-by: Christian Hesse <mail@eworm.de> --- imap-send.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/imap-send.c b/imap-send.c index f2e1947e63..cae494c663 100644 --- a/imap-send.c +++ b/imap-send.c @@ -34,6 +34,8 @@ typedef void *SSL; #endif #ifdef USE_CURL_FOR_IMAP_SEND #include "http.h" +#else +#include "strbuf.h" #endif #if defined(USE_CURL_FOR_IMAP_SEND) -- 2.43.1 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 1/1] imap-send: include strbuf.h 2024-02-09 22:26 Christian Hesse @ 2024-02-09 22:42 ` Junio C Hamano 2024-02-09 22:54 ` Junio C Hamano 0 siblings, 1 reply; 25+ messages in thread From: Junio C Hamano @ 2024-02-09 22:42 UTC (permalink / raw) To: Christian Hesse, Philippe Blain; +Cc: Git Mailing List, Christian Hesse Christian Hesse <list@eworm.de> writes: > From: Christian Hesse <mail@eworm.de> > > We had this fixed in 3307f7dde2ae8f5281d0782f7291a073c9b1cdc2, > and it broke again in eea0e59ffbed6e33d171ace5be13cde9faa41639. Thanks, already reported and fixed, I believe? ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/1] imap-send: include strbuf.h 2024-02-09 22:42 ` Junio C Hamano @ 2024-02-09 22:54 ` Junio C Hamano 2024-02-10 20:01 ` Christian Hesse 0 siblings, 1 reply; 25+ messages in thread From: Junio C Hamano @ 2024-02-09 22:54 UTC (permalink / raw) To: Christian Hesse; +Cc: Philippe Blain, Git Mailing List, Christian Hesse Junio C Hamano <gitster@pobox.com> writes: > Christian Hesse <list@eworm.de> writes: > >> From: Christian Hesse <mail@eworm.de> >> >> We had this fixed in 3307f7dde2ae8f5281d0782f7291a073c9b1cdc2, >> and it broke again in eea0e59ffbed6e33d171ace5be13cde9faa41639. > > Thanks, already reported and fixed, I believe? Oops, missing link: https://lore.kernel.org/git/pull.1664.git.git.1706833113569.gitgitgadget@gmail.com/ ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/1] imap-send: include strbuf.h 2024-02-09 22:54 ` Junio C Hamano @ 2024-02-10 20:01 ` Christian Hesse 2024-02-11 2:42 ` Junio C Hamano 0 siblings, 1 reply; 25+ messages in thread From: Christian Hesse @ 2024-02-10 20:01 UTC (permalink / raw) To: Junio C Hamano; +Cc: Philippe Blain, Git Mailing List, Christian Hesse [-- Attachment #1: Type: text/plain, Size: 968 bytes --] Junio C Hamano <gitster@pobox.com> on Fri, 2024/02/09 14:54: > Junio C Hamano <gitster@pobox.com> writes: > > > Christian Hesse <list@eworm.de> writes: > > > >> From: Christian Hesse <mail@eworm.de> > >> > >> We had this fixed in 3307f7dde2ae8f5281d0782f7291a073c9b1cdc2, > >> and it broke again in eea0e59ffbed6e33d171ace5be13cde9faa41639. > > > > Thanks, already reported and fixed, I believe? > > Oops, missing link: > > https://lore.kernel.org/git/pull.1664.git.git.1706833113569.gitgitgadget@gmail.com/ Sorry, missed that... Probably because the breakage went into 2.43.1, but the upstream fix did not. So sorry for the noise. Anyway... does it make sense to move the include into the condition? -- main(a){char*c=/* Schoene Gruesse */"B?IJj;MEH" "CX:;",b;for(a/* Best regards my address: */=0;b=c[a++];) putchar(b-1/(/* Chris cc -ox -xc - && ./x */b/42*2-3)*42);} [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/1] imap-send: include strbuf.h 2024-02-10 20:01 ` Christian Hesse @ 2024-02-11 2:42 ` Junio C Hamano 0 siblings, 0 replies; 25+ messages in thread From: Junio C Hamano @ 2024-02-11 2:42 UTC (permalink / raw) To: Christian Hesse; +Cc: Philippe Blain, Git Mailing List, Christian Hesse Christian Hesse <list@eworm.de> writes: >> Oops, missing link: >> >> https://lore.kernel.org/git/pull.1664.git.git.1706833113569.gitgitgadget@gmail.com/ > > Sorry, missed that... Probably because the breakage went into 2.43.1, but the > upstream fix did not. So sorry for the noise. Please don't be. Duplicated reports are much much better than no reports due to "well, this must have been reported already by somebody else". Thanks for reporting. > Anyway... does it make sense to move the include into the condition? I do not think so. The original breakage was because it implicitly relied on the fact that http.h, which is included only when USE_CURL_FOR_IMAP_SEND is defined, happens to include strbuf.h, even though the code that does not rely on USE_CURL_FOR_IMAP_SEND do unconditionally rely on the strbuf facility being available to them, possibly combined with the fact that not too many people build imap-send with USE_CURL_FOR_IMAP_SEND disabled. So the conditional thing still rely on an implicit assumption you are making, i.e. "http.h will forever be including strbuf.h", which is fragile when people from time to time come and make sweeping "header clean-up". Which is a good thing. But we need to be careful, and one way to help us being careful against such a header clean-up is to make sure you include what you use yourself, instead of assuming that somebody else you include will keep doing so. ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2024-02-11 2:42 UTC | newest] Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-05-17 7:06 [PATCH 1/1] imap-send: include strbuf.h Christian Hesse 2023-05-17 15:49 ` Junio C Hamano 2023-05-17 16:02 ` Taylor Blau 2023-05-17 16:19 ` Junio C Hamano 2023-05-17 16:31 ` Taylor Blau 2023-05-17 20:12 ` Christian Hesse 2023-05-17 20:18 ` Christian Hesse 2023-05-18 15:56 ` Junio C Hamano 2023-05-17 16:23 ` Taylor Blau 2023-05-17 16:53 ` Junio C Hamano 2023-05-17 17:01 ` Junio C Hamano 2023-05-17 17:58 ` Taylor Blau 2023-05-17 18:06 ` rsbecker 2023-05-17 18:12 ` Junio C Hamano 2023-05-17 19:30 ` rsbecker 2023-05-17 18:09 ` Junio C Hamano 2023-05-17 21:38 ` Taylor Blau 2023-05-18 16:01 ` Junio C Hamano 2023-05-18 18:25 ` Jeff King 2023-05-18 20:49 ` Junio C Hamano -- strict thread matches above, loose matches on Subject: below -- 2024-02-09 22:26 Christian Hesse 2024-02-09 22:42 ` Junio C Hamano 2024-02-09 22:54 ` Junio C Hamano 2024-02-10 20:01 ` Christian Hesse 2024-02-11 2:42 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).