{"thread":{"id":"59756","subject":"[PATCH 1/1] imap-send: include strbuf.h","startedAt":"2023-05-17T07:12:44Z","lastAt":"2023-05-18T20:50:19Z","messageCount":20,"participants":["Christian Hesse","Junio C Hamano","Taylor Blau","rsbecker@nexbridge.com","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"477430","messageId":"20230517070632.71884-1-list@eworm.de","threadId":"59756","inReplyTo":null,"subject":"[PATCH 1/1] imap-send: include strbuf.h","fromName":"Christian Hesse","fromEmail":"list@eworm.de","sentAt":"2023-05-17T07:06:32Z","receivedAt":"2023-05-17T07:12:44Z","isPatch":true,"sender":{"key":"list@eworm.de","avatar":"https://gravatar.com/avatar/ec9a78d63ae8bf8efdc06867449c0a3e763066c462c2c2f9f103ac4675109e14?d=mp&s=160"},"body":"From: Christian Hesse \n\nWe use xstrfmt() here, so let's include the header file.\n\nSigned-off-by: Christian Hesse \n---\n imap-send.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/imap-send.c b/imap-send.c\nindex a62424e90a..7f5426177a 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -29,6 +29,7 @@\n #include \"run-command.h\"\n #include \"parse-options.h\"\n #include \"setup.h\"\n+#include \"strbuf.h\"\n #include \"wrapper.h\"\n #if defined(NO_OPENSSL) && !defined(HAVE_OPENSSL_CSPRNG)\n typedef void *SSL;\n-- \n2.40.1\n\n"},{"id":"477452","messageId":"xmqqwn17q7ou.fsf@gitster.g","threadId":"59756","inReplyTo":"20230517070632.71884-1-list@eworm.de","subject":"Re: [PATCH 1/1] imap-send: include strbuf.h","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-17T15:49:37Z","receivedAt":"2023-05-17T15:49:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Hesse writes:\n\n> From: Christian Hesse \n>\n> We use xstrfmt() here, so let's include the header file.\n>\n> Signed-off-by: Christian Hesse \n> ---\n> imap-send.c | 1 +\n> 1 file changed, 1 insertion(+)\n\nPuzzled. For me Git 2.41-rc0 builds as-is without this change just\nfine, it seems. I know there are many header file shuffling patches\nflying around, and I have seen some of them, but is this a fix for\none of these patches?\n\nThanks.\n\n>\n> diff --git a/imap-send.c b/imap-send.c\n> index a62424e90a..7f5426177a 100644\n> --- a/imap-send.c\n> +++ b/imap-send.c\n> @@ -29,6 +29,7 @@\n> #include \"run-command.h\"\n> #include \"parse-options.h\"\n> #include \"setup.h\"\n> +#include \"strbuf.h\"\n> #include \"wrapper.h\"\n> #if defined(NO_OPENSSL) && !defined(HAVE_OPENSSL_CSPRNG)\n> typedef void *SSL;\n"},{"id":"477454","messageId":"ZGT6fEZFumAsZnxu@nand.local","threadId":"59756","inReplyTo":"xmqqwn17q7ou.fsf@gitster.g","subject":"Re: [PATCH 1/1] imap-send: include strbuf.h","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-17T16:02:04Z","receivedAt":"2023-05-17T16:02:20Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, May 17, 2023 at 08:49:37AM -0700, Junio C Hamano wrote:\n> Christian Hesse writes:\n>\n> > From: Christian Hesse \n> >\n> > We use xstrfmt() here, so let's include the header file.\n> >\n> > Signed-off-by: Christian Hesse \n> > ---\n> > imap-send.c | 1 +\n> > 1 file changed, 1 insertion(+)\n>\n> Puzzled. For me Git 2.41-rc0 builds as-is without this change just\n> fine, it seems.\n\nIt will fail to build for ancient versions of curl (pre-7.34.0, which\nwas released in 2013), or if you build with `NO_CURL=1`.\n\n> I know there are many header file shuffling patches flying around, and\n> I have seen some of them, but is this a fix for one of these patches?\n\nSimilar to [1], this bisects to ba3d1c73da (treewide: remove unnecessary\ncache.h includes, 2023-02-24).\n\nThanks,\nTaylor\n\n[1]: https://lore.kernel.org/git/ZGP2tw0USsj9oecZ@nand.local/\n"},{"id":"477455","messageId":"xmqqilcrq6a9.fsf@gitster.g","threadId":"59756","inReplyTo":"ZGT6fEZFumAsZnxu@nand.local","subject":"Re: [PATCH 1/1] imap-send: include strbuf.h","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-17T16:19:58Z","receivedAt":"2023-05-17T16:20:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> On Wed, May 17, 2023 at 08:49:37AM -0700, Junio C Hamano wrote:\n>> Christian Hesse writes:\n>>\n>> > From: Christian Hesse \n>> >\n>> > We use xstrfmt() here, so let's include the header file.\n>> >\n>> > Signed-off-by: Christian Hesse \n>> > ---\n>> > imap-send.c | 1 +\n>> > 1 file changed, 1 insertion(+)\n>>\n>> Puzzled. For me Git 2.41-rc0 builds as-is without this change just\n>> fine, it seems.\n>\n> It will fail to build for ancient versions of curl (pre-7.34.0, which\n> was released in 2013), or if you build with `NO_CURL=1`.\n\nxstrfmt() is used at exactly one place, inside \"#ifndef NO_OPENSSL\",\nin the implementation of the static function cram().\n\nAh, the mention of that function was a huge red herring. There are\ntons of strbuf API calls in the file outside any conditional\ncompilation, and where it inherits the include from is \"http.h\",\nthat is conditionally included.\n\nOK, so the fix seems to make sense, but the justification for the\nchange needs to be rewritten, I think.\n\n We make liberal use of the strbuf API functions and types, but\n the inclusion of comes indirectly by including\n , which does not happen if you build with NO_CURL.\n\nor something like that?\n\nThanks.\n"},{"id":"477456","messageId":"ZGT/eK6+IKlCM6Sg@nand.local","threadId":"59756","inReplyTo":"ZGT6fEZFumAsZnxu@nand.local","subject":"Re: [PATCH 1/1] imap-send: include strbuf.h","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-17T16:23:20Z","receivedAt":"2023-05-17T16:23:26Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, May 17, 2023 at 12:02:04PM -0400, Taylor Blau wrote:\n> > I know there are many header file shuffling patches flying around, and\n> > I have seen some of them, but is this a fix for one of these patches?\n>\n> Similar to [1], this bisects to ba3d1c73da (treewide: remove unnecessary\n> cache.h includes, 2023-02-24).\n\nI'm looking through other files to see if we have other uses of the\nstrbuf API that are hidden behind a #define without a corresponding\n#include \"strbuf.h\".\n\nHere's the (gross) script I wrote up to check:\n\n git grep -l -e '[^_]xstrdup(' -e 'strbuf_[a-z0-9A-Z_]*(' \\*.c |\n while read f\n do\n if ! gcc -I $(pwd) -E $f | grep -q 'struct strbuf {'\n then\n echo \"==> $f NOT OK\";\n fi\n done\n\nHere's the list:\n\n ==> compat/fsmonitor/fsm-listen-darwin.c NOT OK\n ==> compat/mingw.c NOT OK\n ==> contrib/credential/osxkeychain/git-credential-osxkeychain.c NOT OK\n ==> pager.c NOT OK\n ==> refs/iterator.c NOT OK\n ==> refs/ref-cache.c NOT OK\n ==> string-list.c NOT OK\n ==> t/helper/test-mktemp.c NOT OK\n\nThe ones in compat are OK to ignore since they both fail to compile on\nmy non-Windows machine (I am missing the `` and\n`` headers, respectively).\n\nThe one in contrib is fine to ignore, since it has its own definition of\nxstrdup().\n\npager.c is OK, since it only needs xstrdup(), not any other parts of the\nstrbuf API. It gets a declaration of xstrdup() from git-compat-util.h\nrefs/iterator.c, refs/ref-cache.c, string-list.c, and\nt/helper/test-mktemp.c are all OK for the same reason.\n\nSo I think that this is the only spot we need to worry about.\n\nThanks,\nTaylor\n"},{"id":"477458","messageId":"ZGUBdoLeiqTQYQ9Z@nand.local","threadId":"59756","inReplyTo":"xmqqilcrq6a9.fsf@gitster.g","subject":"Re: [PATCH 1/1] imap-send: include strbuf.h","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-17T16:31:50Z","receivedAt":"2023-05-17T16:31:55Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, May 17, 2023 at 09:19:58AM -0700, Junio C Hamano wrote:\n> OK, so the fix seems to make sense, but the justification for the\n> change needs to be rewritten, I think.\n>\n> We make liberal use of the strbuf API functions and types, but\n> the inclusion of comes indirectly by including\n> , which does not happen if you build with NO_CURL.\n>\n> or something like that?\n\nAgreed. Here's a patch that summarizes my investigation. Thanks again,\nChristian, for reporting!\n\n--- 8< ---\n\nSubject: [PATCH] imap-send.c: fix compilation under NO_CURL and ancient\n versions\n\nWhen building imap-send.c with an ancient (pre-7.34.0, which was\nreleased in 2013) version of curl, or with `NO_CURL` defined, Git\n2.41.0-rc0 fails to compile imap-send.c, i.e.\n\n $ make NO_CURL=1 imap-send.o\n\nThis is similar to 52c0f3318d (run-command.c: fix missing include under\n`NO_PTHREADS`, 2023-05-16), but the bisection points at ba3d1c73da\n(treewide: remove unnecessary cache.h includes, 2023-02-24) instead.\n\nThe trivial fix is to include \"strbuf.h\" unconditionally, which is a\nnoop for most builds, and saves us otherwise.\n\nTo check for other *.c files that might suffer from the same issue, we\ncan run:\n\n git grep -l -e '[^_]xstrdup(' -e 'strbuf_[a-z0-9A-Z_]*(' \\*.c |\n while read f\n do\n if ! gcc -I $(pwd) -E $f | grep -q 'struct strbuf {'\n then\n echo \"==> $f NOT OK\";\n fi\n done\n\n...which runs the preprocessor on (roughly) all C source files that call\n`xstrdup()` or use the strbuf API. The resulting list looks (on my\nmachine) lile:\n\n ==> compat/fsmonitor/fsm-listen-darwin.c NOT OK\n ==> compat/mingw.c NOT OK\n ==> contrib/credential/osxkeychain/git-credential-osxkeychain.c NOT OK\n ==> pager.c NOT OK\n ==> refs/iterator.c NOT OK\n ==> refs/ref-cache.c NOT OK\n ==> string-list.c NOT OK\n ==> t/helper/test-mktemp.c NOT OK\n\nThe ones in compat are OK to ignore since they both fail to compile on\nmy non-Windows machine (I am missing the `` and\n`` headers, respectively).\n\nThe one in contrib is fine to ignore, since it has its own definition of\nxstrdup().\n\npager.c is OK, since it only needs xstrdup(), not any other parts of the\nstrbuf API. It gets a declaration of xstrdup() from git-compat-util.h\nrefs/iterator.c, refs/ref-cache.c, string-list.c, and\nt/helper/test-mktemp.c are all OK for the same reason.\n\nSo this is the only spot that needs fixing.\n\nReported-by: Christian Hesse \nOriginal-fix-by: Christian Hesse \nSigned-off-by: Taylor Blau \n---\n imap-send.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/imap-send.c b/imap-send.c\nindex a62424e90a..7f5426177a 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -29,6 +29,7 @@\n #include \"run-command.h\"\n #include \"parse-options.h\"\n #include \"setup.h\"\n+#include \"strbuf.h\"\n #include \"wrapper.h\"\n #if defined(NO_OPENSSL) && !defined(HAVE_OPENSSL_CSPRNG)\n typedef void *SSL;\n--\n2.41.0.rc0.1.g01bd045298\n\n"},{"id":"477460","messageId":"xmqqcz2yrjbe.fsf@gitster.g","threadId":"59756","inReplyTo":"ZGT/eK6+IKlCM6Sg@nand.local","subject":"Re: [PATCH 1/1] imap-send: include strbuf.h","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-17T16:53:09Z","receivedAt":"2023-05-17T16:54:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> Here's the (gross) script I wrote up to check:\n>\n> git grep -l -e '[^_]xstrdup(' -e 'strbuf_[a-z0-9A-Z_]*(' \\*.c |\n> while read f\n> do\n> if ! gcc -I $(pwd) -E $f | grep -q 'struct strbuf {'\n> then\n> echo \"==> $f NOT OK\";\n> fi\n> done\n\nI am a bit puzzled.\n\nWhat does the above prove, more than what your regular compilation\nthat does not fail, tells us? Doesn't -E expand recursively, so for\nthe case of imap-send.c, with your usual configuration, wouldn't it\nhave grabbed \"struct strbuf\" via inclusion of indirectly\nanyway?\n\n> Here's the list:\n>\n> ==> compat/fsmonitor/fsm-listen-darwin.c NOT OK\n> ==> compat/mingw.c NOT OK\n> ==> contrib/credential/osxkeychain/git-credential-osxkeychain.c NOT OK\n> ==> pager.c NOT OK\n> ==> refs/iterator.c NOT OK\n> ==> refs/ref-cache.c NOT OK\n> ==> string-list.c NOT OK\n> ==> t/helper/test-mktemp.c NOT OK\n>\n> The ones in compat are OK to ignore since they both fail to compile on\n> my non-Windows machine (I am missing the `` and\n> `` headers, respectively).\n>\n> The one in contrib is fine to ignore, since it has its own definition of\n> xstrdup().\n>\n> pager.c is OK, since it only needs xstrdup(), not any other parts of the\n> strbuf API. It gets a declaration of xstrdup() from git-compat-util.h\n> refs/iterator.c, refs/ref-cache.c, string-list.c, and\n> t/helper/test-mktemp.c are all OK for the same reason.\n>\n> So I think that this is the only spot we need to worry about.\n"},{"id":"477461","messageId":"xmqq8rdmrixc.fsf@gitster.g","threadId":"59756","inReplyTo":"xmqqcz2yrjbe.fsf@gitster.g","subject":"Re: [PATCH 1/1] imap-send: include strbuf.h","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-17T17:01:35Z","receivedAt":"2023-05-17T17:01:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> if ! gcc -I $(pwd) -E $f | grep -q 'struct strbuf {'\n> ...\n> What does the above prove, more than what your regular compilation\n> that does not fail, tells us?\n\nIt is actually worse than that, isn't it? This does not even use\nthe definition in the config.mak.uname, so it is not even matching\nyour build environment.\n\nI am uncomfortable to use this as an explanation of what due\ndiligence we did to convince ourselves that this fix should cover\nall similar issues. Perhaps I am grossly misunderstanding what your\ninvestigation did?\n\nThanks.\n"},{"id":"477466","messageId":"ZGUVvjG+xou3w8YW@nand.local","threadId":"59756","inReplyTo":"xmqq8rdmrixc.fsf@gitster.g","subject":"Re: [PATCH 1/1] imap-send: include strbuf.h","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-17T17:58:22Z","receivedAt":"2023-05-17T17:58:34Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, May 17, 2023 at 10:01:35AM -0700, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n> >> if ! gcc -I $(pwd) -E $f | grep -q 'struct strbuf {'\n> > ...\n> > What does the above prove, more than what your regular compilation\n> > that does not fail, tells us?\n>\n> It is actually worse than that, isn't it? This does not even use\n> the definition in the config.mak.uname, so it is not even matching\n> your build environment.\n>\n> I am uncomfortable to use this as an explanation of what due\n> diligence we did to convince ourselves that this fix should cover\n> all similar issues. Perhaps I am grossly misunderstanding what your\n> investigation did?\n\nOof, yes, you are right:\n\n diff -u \\\n <(gcc -I . -E imap-send.c) \\\n <(gcc -DNO_CURL=1 -I . -E imap-send.c)\n\nHow *should* we test this?\n\nThanks,\nTaylor\n"},{"id":"477468","messageId":"016701d988ea$4e4ffcd0$eaeff670$@nexbridge.com","threadId":"59756","inReplyTo":"ZGUVvjG+xou3w8YW@nand.local","subject":"RE: [PATCH 1/1] imap-send: include strbuf.h","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2023-05-17T18:06:24Z","receivedAt":"2023-05-17T18:06:39Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Wednesday, May 17, 2023 1:58 PM, Taylor Blau wrote:\n>On Wed, May 17, 2023 at 10:01:35AM -0700, Junio C Hamano wrote:\n>> Junio C Hamano writes:\n>>\n>> >> if ! gcc -I $(pwd) -E $f | grep -q 'struct strbuf {'\n>> > ...\n>> > What does the above prove, more than what your regular compilation\n>> > that does not fail, tells us?\n>>\n>> It is actually worse than that, isn't it? This does not even use the\n>> definition in the config.mak.uname, so it is not even matching your\n>> build environment.\n>>\n>> I am uncomfortable to use this as an explanation of what due diligence\n>> we did to convince ourselves that this fix should cover all similar\n>> issues. Perhaps I am grossly misunderstanding what your investigation\n>> did?\n>\n>Oof, yes, you are right:\n>\n> diff -u \\\n> <(gcc -I . -E imap-send.c) \\\n> <(gcc -DNO_CURL=1 -I . -E imap-send.c)\n>\n>How *should* we test this?\n\nI 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.\n\n"},{"id":"477469","messageId":"xmqqy1lmq183.fsf@gitster.g","threadId":"59756","inReplyTo":"ZGUVvjG+xou3w8YW@nand.local","subject":"Re: [PATCH 1/1] imap-send: include strbuf.h","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-17T18:09:16Z","receivedAt":"2023-05-17T18:09:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> On Wed, May 17, 2023 at 10:01:35AM -0700, Junio C Hamano wrote:\n>> Junio C Hamano writes:\n>>\n>> >> if ! gcc -I $(pwd) -E $f | grep -q 'struct strbuf {'\n>> > ...\n>> > What does the above prove, more than what your regular compilation\n>> > that does not fail, tells us?\n>>\n>> It is actually worse than that, isn't it? This does not even use\n>> the definition in the config.mak.uname, so it is not even matching\n>> your build environment.\n>>\n>> I am uncomfortable to use this as an explanation of what due\n>> diligence we did to convince ourselves that this fix should cover\n>> all similar issues. Perhaps I am grossly misunderstanding what your\n>> investigation did?\n>\n> Oof, yes, you are right:\n>\n> diff -u \\\n> <(gcc -I . -E imap-send.c) \\\n> <(gcc -DNO_CURL=1 -I . -E imap-send.c)\n>\n> How *should* we test this?\n\nMy inclination is punt and simply do not to claim that we have done\na good due diligence to ensure with all permutations of ifdef we are\nincluding necessary headers.\n\n"},{"id":"477470","messageId":"xmqqttwaq133.fsf@gitster.g","threadId":"59756","inReplyTo":"016701d988ea$4e4ffcd0$eaeff670$@nexbridge.com","subject":"Re: [PATCH 1/1] imap-send: include strbuf.h","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-17T18:12:16Z","receivedAt":"2023-05-17T18:12:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":" writes:\n\n>>Oof, yes, you are right:\n>>\n>> diff -u \\\n>> <(gcc -I . -E imap-send.c) \\\n>> <(gcc -DNO_CURL=1 -I . -E imap-send.c)\n>>\n>>How *should* we test this?\n>\n> I hope not by using gcc, which is not currently a\n> dependency. Using the C preprocessor directly might help in a more\n> general sense, but you probably will need a knob for some\n> compilers to work.\n\nI am not going to suggest trying all permutations of CPP macros to\nmake sure we cover all the #ifdef'ed sections, but -E to show CPP\noutput is pretty common feature not limited to \"gcc\", so if we were\nto do that, we'd very likely use the usual $(CC) Makefile macro to\ninvoke such a test.\n"},{"id":"477479","messageId":"017301d988f6$0fe7eb90$2fb7c2b0$@nexbridge.com","threadId":"59756","inReplyTo":"xmqqttwaq133.fsf@gitster.g","subject":"RE: [PATCH 1/1] imap-send: include strbuf.h","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2023-05-17T19:30:32Z","receivedAt":"2023-05-17T19:31:03Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Wednesday, May 17, 2023 2:12 PM, Junio C Hamano wrote:\n> writes:\n>\n>>>Oof, yes, you are right:\n>>>\n>>> diff -u \\\n>>> <(gcc -I . -E imap-send.c) \\\n>>> <(gcc -DNO_CURL=1 -I . -E imap-send.c)\n>>>\n>>>How *should* we test this?\n>>\n>> I hope not by using gcc, which is not currently a dependency. Using\n>> the C preprocessor directly might help in a more general sense, but\n>> you probably will need a knob for some compilers to work.\n>\n>I am not going to suggest trying all permutations of CPP macros to make\nsure we\n>cover all the #ifdef'ed sections, but -E to show CPP output is pretty\ncommon feature\n>not limited to \"gcc\", so if we were to do that, we'd very likely use the\nusual $(CC)\n>Makefile macro to invoke such a test.\n\n-E would work for me, but I do recall other platforms that would not (but\ncan't place them at the moment). No objection from me, but it still might be\nuseful to have a variable, like CPPFLAGS=-E (by default), that might help\nothers, in config.mak.uname.\n\n\n"},{"id":"477483","messageId":"20230517221850.1117204b@leda.eworm.net","threadId":"59756","inReplyTo":"20230517221237.590fb984@leda.eworm.net","subject":"Re: [PATCH 1/1] imap-send: include strbuf.h","fromName":"Christian Hesse","fromEmail":"list@eworm.de","sentAt":"2023-05-17T20:18:50Z","receivedAt":"2023-05-17T20:19:02Z","isPatch":true,"sender":{"key":"list@eworm.de","avatar":"https://gravatar.com/avatar/ec9a78d63ae8bf8efdc06867449c0a3e763066c462c2c2f9f103ac4675109e14?d=mp&s=160"},"body":"Christian Hesse on Wed, 2023/05/17 22:12:\n> > OK, so the fix seems to make sense, but the justification for the\n> > change needs to be rewritten, I think.\n> > \n> > We make liberal use of the strbuf API functions and types, but\n> > the inclusion of comes indirectly by including\n> > , which does not happen if you build with NO_CURL.\n> > \n> > or something like that? \n> \n> Fine with me!\n> Do you want me to re-send the patch or do you modify this on the fly?\n\nFound it in next branch already. Thanks a lot!\n-- \nmain(a){char*c=/* Schoene Gruesse */\"B?IJj;MEH\"\n\"CX:;\",b;for(a/* Best regards my address: */=0;b=c[a++];)\nputchar(b-1/(/* Chris cc -ox -xc - && ./x */b/42*2-3)*42);}\n"},{"id":"477486","messageId":"20230517221237.590fb984@leda.eworm.net","threadId":"59756","inReplyTo":"xmqqilcrq6a9.fsf@gitster.g","subject":"Re: [PATCH 1/1] imap-send: include strbuf.h","fromName":"Christian Hesse","fromEmail":"list@eworm.de","sentAt":"2023-05-17T20:12:37Z","receivedAt":"2023-05-17T20:22:36Z","isPatch":true,"sender":{"key":"list@eworm.de","avatar":"https://gravatar.com/avatar/ec9a78d63ae8bf8efdc06867449c0a3e763066c462c2c2f9f103ac4675109e14?d=mp&s=160"},"body":"Junio C Hamano on Wed, 2023/05/17 09:19:\n> Taylor Blau writes:\n> \n> > On Wed, May 17, 2023 at 08:49:37AM -0700, Junio C Hamano wrote: \n> >> Christian Hesse writes:\n> >> \n> >> > From: Christian Hesse \n> >> >\n> >> > We use xstrfmt() here, so let's include the header file.\n> >> >\n> >> > Signed-off-by: Christian Hesse \n> >> > ---\n> >> > imap-send.c | 1 +\n> >> > 1 file changed, 1 insertion(+) \n> >>\n> >> Puzzled. For me Git 2.41-rc0 builds as-is without this change just\n> >> fine, it seems. \n\nI prepared cgit to build with libgit.a 2.41.0-rc0. While cgit itself builds\nfine (with some justifications of course), building git for the test suite\nfailed.\n\n> > It will fail to build for ancient versions of curl (pre-7.34.0, which\n> > was released in 2013), or if you build with `NO_CURL=1`. \n\nIndeed we have NO_CURL=1 in cgit's Makefile...\n\n> xstrfmt() is used at exactly one place, inside \"#ifndef NO_OPENSSL\",\n> in the implementation of the static function cram().\n>\n> Ah, the mention of that function was a huge red herring.\n\nWell, the warning about implicit declaration of xstrfmt() was this one that\npopped up... :)\nSorry for the confusion.\n\n> There are\n> tons of strbuf API calls in the file outside any conditional\n> compilation, and where it inherits the include from is \"http.h\",\n> that is conditionally included.\n> \n> OK, so the fix seems to make sense, but the justification for the\n> change needs to be rewritten, I think.\n> \n> We make liberal use of the strbuf API functions and types, but\n> the inclusion of comes indirectly by including\n> , which does not happen if you build with NO_CURL.\n> \n> or something like that?\n\nFine with me!\nDo you want me to re-send the patch or do you modify this on the fly?\n \n> Thanks.\n-- \nmain(a){char*c=/* Schoene Gruesse */\"B?IJj;MEH\"\n\"CX:;\",b;for(a/* Best regards my address: */=0;b=c[a++];)\nputchar(b-1/(/* Chris cc -ox -xc - && ./x */b/42*2-3)*42);}\n"},{"id":"477488","messageId":"ZGVFnzyStiscDKh3@nand.local","threadId":"59756","inReplyTo":"xmqqy1lmq183.fsf@gitster.g","subject":"Re: [PATCH 1/1] imap-send: include strbuf.h","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-17T21:38:25Z","receivedAt":"2023-05-17T21:38:32Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, May 17, 2023 at 11:09:16AM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> > On Wed, May 17, 2023 at 10:01:35AM -0700, Junio C Hamano wrote:\n> >> Junio C Hamano writes:\n> >>\n> >> >> if ! gcc -I $(pwd) -E $f | grep -q 'struct strbuf {'\n> >> > ...\n> >> > What does the above prove, more than what your regular compilation\n> >> > that does not fail, tells us?\n> >>\n> >> It is actually worse than that, isn't it? This does not even use\n> >> the definition in the config.mak.uname, so it is not even matching\n> >> your build environment.\n> >>\n> >> I am uncomfortable to use this as an explanation of what due\n> >> diligence we did to convince ourselves that this fix should cover\n> >> all similar issues. Perhaps I am grossly misunderstanding what your\n> >> investigation did?\n> >\n> > Oof, yes, you are right:\n> >\n> > diff -u \\\n> > <(gcc -I . -E imap-send.c) \\\n> > <(gcc -DNO_CURL=1 -I . -E imap-send.c)\n> >\n> > How *should* we test this?\n>\n> My inclination is punt and simply do not to claim that we have done\n> a good due diligence to ensure with all permutations of ifdef we are\n> including necessary headers.\n\nI think that's the best course of action, too. I see that it's already\non 'next', thanks.\n\nThanks,\nTaylor\n"},{"id":"477528","messageId":"xmqqbkihvdiw.fsf@gitster.g","threadId":"59756","inReplyTo":"20230517221850.1117204b@leda.eworm.net","subject":"Re: [PATCH 1/1] imap-send: include strbuf.h","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-18T15:56:55Z","receivedAt":"2023-05-18T15:57:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Hesse writes:\n\n> Christian Hesse on Wed, 2023/05/17 22:12:\n>> > OK, so the fix seems to make sense, but the justification for the\n>> > change needs to be rewritten, I think.\n>> > \n>> > We make liberal use of the strbuf API functions and types, but\n>> > the inclusion of comes indirectly by including\n>> > , which does not happen if you build with NO_CURL.\n>> > \n>> > or something like that? \n>> \n>> Fine with me!\n>> Do you want me to re-send the patch or do you modify this on the fly?\n>\n> Found it in next branch already. Thanks a lot!\n\nWe made moderate amount of these kind of header shuffling this\ncycle, and it is inevitable to encounter a gotcha like this when\nbuilding with anything less than the most common configurations.\n\nThanks for reporting and fixing; very much appreciated.\n\n"},{"id":"477529","messageId":"xmqq7ct5vdbk.fsf@gitster.g","threadId":"59756","inReplyTo":"ZGVFnzyStiscDKh3@nand.local","subject":"Re: [PATCH 1/1] imap-send: include strbuf.h","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-18T16:01:19Z","receivedAt":"2023-05-18T16:01:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n>> My inclination is punt and simply do not to claim that we have done\n>> a good due diligence to ensure with all permutations of ifdef we are\n>> including necessary headers.\n>\n> I think that's the best course of action, too. I see that it's already\n> on 'next', thanks.\n\nYeah, I am actuall hoping that somebody clever, with time, comes up\nwith a systematic way to give us better coverage, but until then, I\nthink it is better to honestly record where we are to future\ndevelopers.\n\nThanks.\n\n\n"},{"id":"477534","messageId":"20230518182504.GA557383@coredump.intra.peff.net","threadId":"59756","inReplyTo":"xmqq7ct5vdbk.fsf@gitster.g","subject":"Re: [PATCH 1/1] imap-send: include strbuf.h","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-05-18T18:25:04Z","receivedAt":"2023-05-18T18:25:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 18, 2023 at 09:01:19AM -0700, Junio C Hamano wrote:\n\n> Taylor Blau writes:\n> \n> >> My inclination is punt and simply do not to claim that we have done\n> >> a good due diligence to ensure with all permutations of ifdef we are\n> >> including necessary headers.\n> >\n> > I think that's the best course of action, too. I see that it's already\n> > on 'next', thanks.\n> \n> Yeah, I am actuall hoping that somebody clever, with time, comes up\n> with a systematic way to give us better coverage, but until then, I\n> think it is better to honestly record where we are to future\n> developers.\n\nI faced a similar issue with the -Wunused-parameter patches. Just when I\nthought I had everything annotated, I'd find some obscure Makefile knob\nthat compiled new code (or even in one case disabled some code that used\na variable!).\n\nI never came up with a good solution, but relying on CI helped, since it\njust builds more of the combinations. Obviously that didn't catch this\ncase. We could try to hit more platforms / combinations of knobs in CI,\nbut there are diminishing returns on the compute time.\n\nBut at least in this case, the old \"if it is important, somebody will\nbuild it and report the problem\" line of thinking worked out. So maybe\nthat is enough.\n\nI guess maybe that was more philosophical than helpful. ;)\n\n-Peff\n"},{"id":"477564","messageId":"xmqq353ttlez.fsf@gitster.g","threadId":"59756","inReplyTo":"20230518182504.GA557383@coredump.intra.peff.net","subject":"Re: [PATCH 1/1] imap-send: include strbuf.h","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-18T20:49:24Z","receivedAt":"2023-05-18T20:50:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, May 18, 2023 at 09:01:19AM -0700, Junio C Hamano wrote:\n>\n>> Yeah, I am actuall hoping that somebody clever, with time, comes up\n>> with a systematic way to give us better coverage, but until then, I\n>> think it is better to honestly record where we are to future\n>> developers.\n>\n> I faced a similar issue with the -Wunused-parameter patches. Just when I\n> thought I had everything annotated, I'd find some obscure Makefile knob\n> that compiled new code (or even in one case disabled some code that used\n> a variable!).\n> ...\n> But at least in this case, the old \"if it is important, somebody will\n> build it and report the problem\" line of thinking worked out. So maybe\n> that is enough.\n\nAh, I guess the debugging situation is quite similar with that\ntopic. Thanks for your insight.\n\n"}]}