* [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: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: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: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 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: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 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 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 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 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).