Re: [PATCH 0/9] Add missing includes and forward declares
Elijah Newren writes: >> >> But please remind me not to merge this round down to 'next', for the >> "enum" forward decl gotcha. > > I'll send out a new round shortly. Would you like me to squash the > last patch (the one that had two hunks with minor conflicts with other > topics in next and pu) into the first patch, or would you rather I > dropped that patch and waited to submit it until later? Either way is fine, especially if you looked at the intergration result from yesterday and resolution of these conflicts looked reasonable to you.
Re: [PATCH 0/9] Add missing includes and forward declares
On Wed, Aug 15, 2018 at 8:43 AM Junio C Hamano wrote: > Elijah Newren writes: > > > On Tue, Aug 14, 2018 at 10:45 PM Junio C Hamano wrote: > >> Elijah Newren writes: > >> > >> > On Mon, Aug 13, 2018 at 11:24 AM Junio C Hamano > >> > wrote: > >> >> Jeff King writes: > >> > > >> >> As things are slowly moving out of the so-far kitchen-sink "cache.h" > >> >> into more specific subsystem headers (like object-store.h), we may > >> >> actually want to tighten the "header that includes it first" part a > >> >> bit in the future, so that 'git grep cache.h' would give us a more > >> >> explicit and a better picture of what really depends on knowing what > >> >> the lowest level plumbing API are built around. > >> >> > >> >> > So I think the better test is a two-line .c file with: > >> >> > > >> >> > #include "git-compat-util.h" > >> >> > #include $header_to_check > >> >> > >> >> But until that tightening happens, I do not actually mind the > >> >> two-line .c file started with inclusion of cache.h instead of > >> >> git-compat-util.h. That would limit the scope of this series > >> >> further. > >> > > >> > Yes, this removes about 2/3 of patch #1. > >> > >> Sorry for making a misleading comment. I should have phrased "I > >> would not have minded if the series were looser by assuming > >> cache.h", implying that "but now the actual patch went extra mile to > >> be more complete, what we have is even better ;-)". > > > > Ah, gotcha. Thanks for the clarification. > > But please remind me not to merge this round down to 'next', for the > "enum" forward decl gotcha. I'll send out a new round shortly. Would you like me to squash the last patch (the one that had two hunks with minor conflicts with other topics in next and pu) into the first patch, or would you rather I dropped that patch and waited to submit it until later?
Re: [PATCH 0/9] Add missing includes and forward declares
Elijah Newren writes: > On Tue, Aug 14, 2018 at 10:45 PM Junio C Hamano wrote: >> Elijah Newren writes: >> >> > On Mon, Aug 13, 2018 at 11:24 AM Junio C Hamano wrote: >> >> Jeff King writes: >> > >> >> As things are slowly moving out of the so-far kitchen-sink "cache.h" >> >> into more specific subsystem headers (like object-store.h), we may >> >> actually want to tighten the "header that includes it first" part a >> >> bit in the future, so that 'git grep cache.h' would give us a more >> >> explicit and a better picture of what really depends on knowing what >> >> the lowest level plumbing API are built around. >> >> >> >> > So I think the better test is a two-line .c file with: >> >> > >> >> > #include "git-compat-util.h" >> >> > #include $header_to_check >> >> >> >> But until that tightening happens, I do not actually mind the >> >> two-line .c file started with inclusion of cache.h instead of >> >> git-compat-util.h. That would limit the scope of this series >> >> further. >> > >> > Yes, this removes about 2/3 of patch #1. >> >> Sorry for making a misleading comment. I should have phrased "I >> would not have minded if the series were looser by assuming >> cache.h", implying that "but now the actual patch went extra mile to >> be more complete, what we have is even better ;-)". > > Ah, gotcha. Thanks for the clarification. But please remind me not to merge this round down to 'next', for the "enum" forward decl gotcha.
Re: [PATCH 0/9] Add missing includes and forward declares
On Tue, Aug 14, 2018 at 10:45 PM Junio C Hamano wrote: > Elijah Newren writes: > > > On Mon, Aug 13, 2018 at 11:24 AM Junio C Hamano wrote: > >> Jeff King writes: > > > >> As things are slowly moving out of the so-far kitchen-sink "cache.h" > >> into more specific subsystem headers (like object-store.h), we may > >> actually want to tighten the "header that includes it first" part a > >> bit in the future, so that 'git grep cache.h' would give us a more > >> explicit and a better picture of what really depends on knowing what > >> the lowest level plumbing API are built around. > >> > >> > So I think the better test is a two-line .c file with: > >> > > >> > #include "git-compat-util.h" > >> > #include $header_to_check > >> > >> But until that tightening happens, I do not actually mind the > >> two-line .c file started with inclusion of cache.h instead of > >> git-compat-util.h. That would limit the scope of this series > >> further. > > > > Yes, this removes about 2/3 of patch #1. > > Sorry for making a misleading comment. I should have phrased "I > would not have minded if the series were looser by assuming > cache.h", implying that "but now the actual patch went extra mile to > be more complete, what we have is even better ;-)". Ah, gotcha. Thanks for the clarification.
Re: [PATCH 0/9] Add missing includes and forward declares
Elijah Newren writes: > On Mon, Aug 13, 2018 at 11:24 AM Junio C Hamano wrote: >> Jeff King writes: > >> As things are slowly moving out of the so-far kitchen-sink "cache.h" >> into more specific subsystem headers (like object-store.h), we may >> actually want to tighten the "header that includes it first" part a >> bit in the future, so that 'git grep cache.h' would give us a more >> explicit and a better picture of what really depends on knowing what >> the lowest level plumbing API are built around. >> >> > So I think the better test is a two-line .c file with: >> > >> > #include "git-compat-util.h" >> > #include $header_to_check >> >> But until that tightening happens, I do not actually mind the >> two-line .c file started with inclusion of cache.h instead of >> git-compat-util.h. That would limit the scope of this series >> further. > > Yes, this removes about 2/3 of patch #1. Sorry for making a misleading comment. I should have phrased "I would not have minded if the series were looser by assuming cache.h", implying that "but now the actual patch went extra mile to be more complete, what we have is even better ;-)".
Re: [PATCH 0/9] Add missing includes and forward declares
Jeff King wrote: > Subject: [PATCH] test-tool.h: include git-compat-util.h > > The test-tool programs include "test-tool.h" as their first > include, which breaks our CodingGuideline of "the first > include must be git-compat-util.h or an equivalent". This > isn't actually a problem, as test-tool.h doesn't define > anything tricky, but we should probably follow our own rule. > > Rather than change them all, let's instead make test-tool.h > one of those equivalents, just like we do for builtin.h > (which many of the actual git builtins include first). I wonder if it would not be simpler to change them all. It would be one less special case. That said, > Signed-off-by: Jeff King > --- > t/helper/test-tool.h | 2 ++ > 1 file changed, 2 insertions(+) Reviewed-by: Jonathan Nieder Thanks.
Re: [PATCH 0/9] Add missing includes and forward declares
Jeff King wrote: > On Mon, Aug 13, 2018 at 11:24:37AM -0700, Junio C Hamano wrote: >> Jeff King wrote: >>> So I think the better test is a two-line .c file with: >>> >>> #include "git-compat-util.h" >>> #include $header_to_check >> >> But until that tightening happens, I do not actually mind the >> two-line .c file started with inclusion of cache.h instead of >> git-compat-util.h. That would limit the scope of this series >> further. > > I can go either way on this. Using cache.h makes Elijah's current series > a bit more focused. But I think we'd eventually want to go there anyway. > I don't have a strong opinion on doing it now or later. For what it's worth, I'd been assuming that any header that is not self-contained after a #include of "git-compat-util.h" is a bug. It's true that many .c files include git-compat-util.h via cache.h, making such a bug harder to notice. But the moment you write a source file that doesn't include cache.h you'd run into that problem, so it's worth fixing ahead-of-time anyway. Thanks, Jonathan
Re: [PATCH 0/9] Add missing includes and forward declares
On Mon, Aug 13, 2018 at 11:24:37AM -0700, Junio C Hamano wrote: > As things are slowly moving out of the so-far kitchen-sink "cache.h" > into more specific subsystem headers (like object-store.h), we may > actually want to tighten the "header that includes it first" part a > bit in the future, so that 'git grep cache.h' would give us a more > explicit and a better picture of what really depends on knowing what > the lowest level plumbing API are built around. Yeah, I agree that's a good long-term goal. I think "builtin.h" is reasonable to remain as a magic header for builtins. > > So I think the better test is a two-line .c file with: > > > > #include "git-compat-util.h" > > #include $header_to_check > > But until that tightening happens, I do not actually mind the > two-line .c file started with inclusion of cache.h instead of > git-compat-util.h. That would limit the scope of this series > further. I can go either way on this. Using cache.h makes Elijah's current series a bit more focused. But I think we'd eventually want to go there anyway. I don't have a strong opinion on doing it now or later. > > I wonder if there's an easy way to check. I guess adding '-Dint=foo' to > > the command line, and then putting '#undef int' at the top of > > git-compat-util would catch just about any code the compiler sees that > > doesn't have git-compat-util included. :) > > ;-). So much to my amazement, my off-the-cuff suggestion actually worked pretty well. The only failures were xdiff (expected, since it does its own system-header stuff, though IMHO we might think about changing that) and the programs in t/helper. Here's a patch to address the latter (and you can see now why I said the above thing about "builtin.h"): -- >8 -- Subject: [PATCH] test-tool.h: include git-compat-util.h The test-tool programs include "test-tool.h" as their first include, which breaks our CodingGuideline of "the first include must be git-compat-util.h or an equivalent". This isn't actually a problem, as test-tool.h doesn't define anything tricky, but we should probably follow our own rule. Rather than change them all, let's instead make test-tool.h one of those equivalents, just like we do for builtin.h (which many of the actual git builtins include first). Signed-off-by: Jeff King --- t/helper/test-tool.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h index 80cbcf0857..75d7c782f0 100644 --- a/t/helper/test-tool.h +++ b/t/helper/test-tool.h @@ -1,6 +1,8 @@ #ifndef __TEST_TOOL_H__ #define __TEST_TOOL_H__ +#include "git-compat-util.h" + int cmd__chmtime(int argc, const char **argv); int cmd__config(int argc, const char **argv); int cmd__ctype(int argc, const char **argv); -- 2.18.0.1070.g4763fa3c01
Re: [PATCH 0/9] Add missing includes and forward declares
On Mon, Aug 13, 2018 at 11:24 AM Junio C Hamano wrote: > Jeff King writes: > As things are slowly moving out of the so-far kitchen-sink "cache.h" > into more specific subsystem headers (like object-store.h), we may > actually want to tighten the "header that includes it first" part a > bit in the future, so that 'git grep cache.h' would give us a more > explicit and a better picture of what really depends on knowing what > the lowest level plumbing API are built around. > > > So I think the better test is a two-line .c file with: > > > > #include "git-compat-util.h" > > #include $header_to_check > > But until that tightening happens, I do not actually mind the > two-line .c file started with inclusion of cache.h instead of > git-compat-util.h. That would limit the scope of this series > further. Yes, this removes about 2/3 of patch #1. But it makes things kind of odd to me; do we change the relevant paragraph of Documentation/CodingGuidelines to add an additional sentence so it reads: - The first #include in C files, except in platform specific compat/ implementations, must be either "git-compat-util.h", "cache.h" or "builtin.h". You do not have to include more than one of these. However, note that including any of a few dozen header files may result in compilation failures if cache.h is not included first. That seems annoying to state, but also annoying to leave undocumented. I'd rather have the various structs (object_id, strbuf, index_state, cache_entry, etc.) forward declared and/or relevant headers (e.g. "cache.h", "strbuf.h", or "string-list.h") #include'd. Are you sure you really want me to remove these fixes? Let me know, and if so I'll do it, even if I'd prefer to leave them in. Let me know how/if you'd like that CodingGuidline to be updated too. Also sha1dc_git.h fails to compile if cache.h is #included before it (various redefinition errors, such as for "platform_SHA_CTX"); it's possible that's expected, but it does make things a little weirder to add implicit assumptions that cache.h is included first.
Re: [PATCH 0/9] Add missing includes and forward declares
Jeff King writes: > The rule in Git has always been that your very first include must > always be "git-compat-util.h" or an equivalent header that includes it > first (like "cache.h"). This is mentioned in CodingGuidelines. Glad to see that you already have written the above so I don't have to ;-) As things are slowly moving out of the so-far kitchen-sink "cache.h" into more specific subsystem headers (like object-store.h), we may actually want to tighten the "header that includes it first" part a bit in the future, so that 'git grep cache.h' would give us a more explicit and a better picture of what really depends on knowing what the lowest level plumbing API are built around. > So I think the better test is a two-line .c file with: > > #include "git-compat-util.h" > #include $header_to_check But until that tightening happens, I do not actually mind the two-line .c file started with inclusion of cache.h instead of git-compat-util.h. That would limit the scope of this series further. > I wonder if there's an easy way to check. I guess adding '-Dint=foo' to > the command line, and then putting '#undef int' at the top of > git-compat-util would catch just about any code the compiler sees that > doesn't have git-compat-util included. :) ;-).
Re: [PATCH 0/9] Add missing includes and forward declares
On Sat, Aug 11, 2018 at 01:59:50AM -0700, Elijah Newren wrote: > The part of my story you snipped in the ellipsis is kind of important, > though: "...and decided to determine which header files were missing > their own necessary #include's and forward declarations." The way I > did so was making a simple one-line .c file that included exactly one > header, and checked to see if I could compile it (without any special > defines), fixed it up as necessary, then repeated that process for > every toplevel header. The rule in Git has always been that your very first include must always be "git-compat-util.h" or an equivalent header that includes it first (like "cache.h"). This is mentioned in CodingGuidelines. So I think the better test is a two-line .c file with: #include "git-compat-util.h" #include $header_to_check And I'd be fine with fixing any compilation failures there, either with forward declarations or recursive includes. I think the in the early days there was some resistance to having a lot of recursive includes, because it _can_ lead to confusion, but IMHO it mostly helps people. And I don't recall much discussion on it in recent years. Including "git-compat-util.h" in many more headers probably doesn't hurt (after all, it's a noop for every .c file which is following the existing rule). But I'd just as soon not sprinkle it everywhere, nor imply that that people don't need to be including it. It's really important that it comes first because it wants a clean slate and have subtle effects on other header files due to macros. Worse still, some of the effects are platform dependent, so we might not realize an ordering problem until somebody on AIX complains. I wonder if there's an easy way to check. I guess adding '-Dint=foo' to the command line, and then putting '#undef int' at the top of git-compat-util would catch just about any code the compiler sees that doesn't have git-compat-util included. :) -Peff
Re: [PATCH 0/9] Add missing includes and forward declares
On Sat, Aug 11, 2018 at 1:30 AM Ævar Arnfjörð Bjarmason wrote
> On Sat, Aug 11 2018, Elijah Newren wrote:
>
> [CC'd sha1dc maintainers, for context the relevent patch is
> https://public-inbox.org/git/20180811043218.31456-8-new...@gmail.com/T/#u]
>
> > * Patches 6-8: These patches might need to be submitted to separate
> > projects elsewhere. Let me know if so.
>
> Yes, for sha1dc (your 7/9) it's much better if we submit patches to
> upstream and then submit a patch to git.git to update from upstream, as
> my 23e37f8e9d ("sha1dc: update from upstream", 2018-08-02) sitting in
> 'next' does.
>
> When we build that library we define SHA1DC_NO_STANDARD_INCLUDES, so we
> don't use the codepath you patched, so your description of "I was bit
> yet again by compilation errors[...]" doesn't add up in that case. I
> assume you just started adding stdlib.h where you could grep for
> stdint.h.
The part of my story you snipped in the ellipsis is kind of important,
though: "...and decided to determine which header files were missing
their own necessary #include's and forward declarations." The way I
did so was making a simple one-line .c file that included exactly one
header, and checked to see if I could compile it (without any special
defines), fixed it up as necessary, then repeated that process for
every toplevel header.
Most of the stdlib.h additions were for the definition of size_t,
sha1dc/sha1.h was no different; without my patch:
In file included from nukeme.c:1:0:
sha1dc/sha1.h:96:43: error: unknown type name ‘size_t’
void SHA1DCUpdate(SHA1_CTX*, const char*, size_t);
Your point that we use SHA1DC_NO_STANDARD_INCLUDES normally when
compiling within git, though, does mean my patch #7 is kind of useless
to git.
> When I check out sha1collisiondetection.git stand-alone (which will use
> that path) and compile it, it works fine for me. This is on GCC 8.1.0 on
> Debian, so perhaps that patch isn't needed in that case.
I never said git or its subprojects failed to compile. I said when I
added another #include of some header to some existing or new files, I
sometimes saw unknown symbol errors. It's happened before, it
happened again today, and I got annoyed, so I set out to address it.
Sorry if that wasn't clear.
Re: [PATCH 0/9] Add missing includes and forward declares
On Sat, Aug 11 2018, Elijah Newren wrote:
[CC'd sha1dc maintainers, for context the relevent patch is
https://public-inbox.org/git/20180811043218.31456-8-new...@gmail.com/T/#u]
> * Patches 6-8: These patches might need to be submitted to separate
> projects elsewhere. Let me know if so.
Yes, for sha1dc (your 7/9) it's much better if we submit patches to
upstream and then submit a patch to git.git to update from upstream, as
my 23e37f8e9d ("sha1dc: update from upstream", 2018-08-02) sitting in
'next' does.
When we build that library we define SHA1DC_NO_STANDARD_INCLUDES, so we
don't use the codepath you patched, so your description of "I was bit
yet again by compilation errors[...]" doesn't add up in that case. I
assume you just started adding stdlib.h where you could grep for
stdint.h.
When I check out sha1collisiondetection.git stand-alone (which will use
that path) and compile it, it works fine for me. This is on GCC 8.1.0 on
Debian, so perhaps that patch isn't needed in that case.
