| Message ID | adafa655432dd13d1c727522377ac9a4b515b76a.1677139521.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Header cleanups | expand |
On 2/23/2023 3:05 AM, Elijah Newren via GitGitGadget wrote: > From: Elijah Newren <newren@gmail.com> > > Since git-compat-util.h needs to be included first in C files, having it > appear in header files is unnecessary. More importantly, having it > included in header files seems to lead to folks leaving it out of C > files, which makes it harder to verify that the rule is being followed. > Remove it from header files, other than the ones that have been approved > as alternate first includes. ... > compat/fsmonitor/fsm-ipc-win32.c | 1 + > compat/fsmonitor/fsm-settings-darwin.c | 1 + It looks like you have a couple .c files here that belong in the previous patch. Thanks, -Stolee
On Thu, Feb 23, 2023 at 5:52 AM Derrick Stolee <derrickstolee@github.com> wrote: > > On 2/23/2023 3:05 AM, Elijah Newren via GitGitGadget wrote: > > From: Elijah Newren <newren@gmail.com> > > > > Since git-compat-util.h needs to be included first in C files, having it > > appear in header files is unnecessary. More importantly, having it > > included in header files seems to lead to folks leaving it out of C > > files, which makes it harder to verify that the rule is being followed. > > Remove it from header files, other than the ones that have been approved > > as alternate first includes. > ... > > compat/fsmonitor/fsm-ipc-win32.c | 1 + > > compat/fsmonitor/fsm-settings-darwin.c | 1 + > > It looks like you have a couple .c files here that belong in the > previous patch. Oops, indeed. Bad squash. (I tended to ignore files under compat/ thinking they were special, but then later found out they also needed to be fixed so that things would work right.) Thanks for catching; will fix.
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Elijah Newren <newren@gmail.com> > > Since git-compat-util.h needs to be included first in C files, having it > appear in header files is unnecessary. More importantly, having it > included in header files seems to lead to folks leaving it out of C > files, which makes it harder to verify that the rule is being followed. > Remove it from header files, other than the ones that have been approved > as alternate first includes. Hmph, doesn't this cut both ways? I like the idea that the removal of compat-util from other header files may increase the likelihood that a C file that includes such header files without including compat-util fail to compile, because it would fail to find what is defined or declared in compat-util. But from "include what you use" point of view, shouldn't a header that defines or declares its own stuff using what is defined or declared in compat-util be including compat-util itself? Or do I misunderstand what "make hdr-check" is trying to achieve? Granted that the check does not fail with this patch in place, but I suspect that it is by accident (i.e. there happens to be nobody who depends on what is defined/declared in compat-util for their own definition or declaration). Also I am not sure how to interpret the fact that "make hdr-check" succeeds with this patch. Does it mean C files that include these header files while forgetting to include compat-util may not be caught by the compiler after all? So, I dunno.
On Thu, Feb 23, 2023 at 11:35 AM Junio C Hamano <gitster@pobox.com> wrote: > > "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: Elijah Newren <newren@gmail.com> > > > > Since git-compat-util.h needs to be included first in C files, having it > > appear in header files is unnecessary. More importantly, having it > > included in header files seems to lead to folks leaving it out of C > > files, which makes it harder to verify that the rule is being followed. > > Remove it from header files, other than the ones that have been approved > > as alternate first includes. > > Hmph, doesn't this cut both ways? > > I like the idea that the removal of compat-util from other > header files may increase the likelihood that a C file that includes > such header files without including compat-util fail to compile, > because it would fail to find what is defined or declared in > compat-util. > > But from "include what you use" point of view, shouldn't a header > that defines or declares its own stuff using what is defined or > declared in compat-util be including compat-util itself? > > Or do I misunderstand what "make hdr-check" is trying to achieve? > > Granted that the check does not fail with this patch in place, but I > suspect that it is by accident (i.e. there happens to be nobody who > depends on what is defined/declared in compat-util for their own > definition or declaration). Also I am not sure how to interpret > the fact that "make hdr-check" succeeds with this patch. Does it > mean C files that include these header files while forgetting to > include compat-util may not be caught by the compiler after all? > > So, I dunno. I did something like that before, and Peff objected; see https://lore.kernel.org/git/20180811173406.GA9119@sigill.intra.peff.net/ and https://lore.kernel.org/git/20180811174301.GA9287@sigill.intra.peff.net/. I think for sanity we should do one of the following: (a) make C and header files both depend upon everything they need (b) consistently exclude git-compat-util.h from headers and require it be the first include in C files I think things get really messy if we let half the headers follow (a) and the other half are forced to do (b). I was pushed towards (b) before, but now that I've worked on this series, I think there is even more reason to go this direction: this work I did during this series shows that if we allow a mixture of (a) and (b), then empirically we end up with C files that don't include git-compat-util.h directly, and those same C files likely include some headers that don't include git-compat-util.h at all, and if the other headers are included before the indirect inclusion of git-compat-util.h then there are risks that things will break in very subtle ways (as pointed out by Peff in the above-linked emails). So, I'm inclined to go towards (b).
Elijah Newren <newren@gmail.com> writes: > I think for sanity we should do one of the following: > > (a) make C and header files both depend upon everything they need > (b) consistently exclude git-compat-util.h from headers and require it > be the first include in C files > > I think things get really messy if we let half the headers follow (a) > and the other half are forced to do (b). I was pushed towards (b) > before, but now that I've worked on this series, I think there is even > more reason to go this direction: this work I did during this series > shows that if we allow a mixture of (a) and (b), then empirically we > end up with C files that don't include git-compat-util.h directly, and > those same C files likely include some headers that don't include > git-compat-util.h at all, and if the other headers are included before > the indirect inclusion of git-compat-util.h then there are risks that > things will break in very subtle ways (as pointed out by Peff in the > above-linked emails). So, I'm inclined to go towards (b). Perfect. Can some of that reasoning be captured in the proposed log message of [02/16] to help future developers? Thanks.
On Thu, Feb 23, 2023 at 2:07 PM Junio C Hamano <gitster@pobox.com> wrote: > > Elijah Newren <newren@gmail.com> writes: > > > I think for sanity we should do one of the following: > > > > (a) make C and header files both depend upon everything they need > > (b) consistently exclude git-compat-util.h from headers and require it > > be the first include in C files > > > > I think things get really messy if we let half the headers follow (a) > > and the other half are forced to do (b). I was pushed towards (b) > > before, but now that I've worked on this series, I think there is even > > more reason to go this direction: this work I did during this series > > shows that if we allow a mixture of (a) and (b), then empirically we > > end up with C files that don't include git-compat-util.h directly, and > > those same C files likely include some headers that don't include > > git-compat-util.h at all, and if the other headers are included before > > the indirect inclusion of git-compat-util.h then there are risks that > > things will break in very subtle ways (as pointed out by Peff in the > > above-linked emails). So, I'm inclined to go towards (b). > > Perfect. Can some of that reasoning be captured in the proposed log > message of [02/16] to help future developers? Yep, will do.
diff --git a/advice.h b/advice.h index 07e0f76833e..3e1b48bf68d 100644 --- a/advice.h +++ b/advice.h @@ -1,8 +1,6 @@ #ifndef ADVICE_H #define ADVICE_H -#include "git-compat-util.h" - struct string_list; /* diff --git a/cbtree.h b/cbtree.h index 0be14fb7ee4..43193abdda2 100644 --- a/cbtree.h +++ b/cbtree.h @@ -14,8 +14,6 @@ #ifndef CBTREE_H #define CBTREE_H -#include "git-compat-util.h" - struct cb_node; struct cb_node { struct cb_node *child[2]; diff --git a/chunk-format.h b/chunk-format.h index 7885aa08487..025c38f938e 100644 --- a/chunk-format.h +++ b/chunk-format.h @@ -1,7 +1,6 @@ #ifndef CHUNK_FORMAT_H #define CHUNK_FORMAT_H -#include "git-compat-util.h" #include "hash.h" struct hashfile; diff --git a/commit-graph.h b/commit-graph.h index 37faee6b66d..bb88bec7aa3 100644 --- a/commit-graph.h +++ b/commit-graph.h @@ -1,7 +1,6 @@ #ifndef COMMIT_GRAPH_H #define COMMIT_GRAPH_H -#include "git-compat-util.h" #include "object-store.h" #include "oidset.h" diff --git a/commit-slab-impl.h b/commit-slab-impl.h index 557738df271..4a414ee905d 100644 --- a/commit-slab-impl.h +++ b/commit-slab-impl.h @@ -1,8 +1,6 @@ #ifndef COMMIT_SLAB_IMPL_H #define COMMIT_SLAB_IMPL_H -#include "git-compat-util.h" - #define implement_static_commit_slab(slabname, elemtype) \ implement_commit_slab(slabname, elemtype, MAYBE_UNUSED static) diff --git a/compat/fsmonitor/fsm-ipc-win32.c b/compat/fsmonitor/fsm-ipc-win32.c index e08c505c148..c9536dfb666 100644 --- a/compat/fsmonitor/fsm-ipc-win32.c +++ b/compat/fsmonitor/fsm-ipc-win32.c @@ -1,3 +1,4 @@ +#include "git-compat-util.h" #include "config.h" #include "fsmonitor-ipc.h" diff --git a/compat/fsmonitor/fsm-settings-darwin.c b/compat/fsmonitor/fsm-settings-darwin.c index 6abbc7af3ab..58b623fbb9a 100644 --- a/compat/fsmonitor/fsm-settings-darwin.c +++ b/compat/fsmonitor/fsm-settings-darwin.c @@ -1,3 +1,4 @@ +#include "git-compat-util.h" #include "config.h" #include "fsmonitor.h" #include "fsmonitor-ipc.h" diff --git a/hash.h b/hash.h index 36b64165fc9..351afc2ce3b 100644 --- a/hash.h +++ b/hash.h @@ -1,7 +1,6 @@ #ifndef HASH_H #define HASH_H -#include "git-compat-util.h" #include "repository.h" #if defined(SHA1_APPLE) diff --git a/pack-mtimes.h b/pack-mtimes.h index cc957b3e852..107327cec0b 100644 --- a/pack-mtimes.h +++ b/pack-mtimes.h @@ -1,8 +1,6 @@ #ifndef PACK_MTIMES_H #define PACK_MTIMES_H -#include "git-compat-util.h" - #define MTIMES_SIGNATURE 0x4d544d45 /* "MTME" */ #define MTIMES_VERSION 1 diff --git a/pkt-line.h b/pkt-line.h index 79c538b99e4..8e9846f3151 100644 --- a/pkt-line.h +++ b/pkt-line.h @@ -1,7 +1,6 @@ #ifndef PKTLINE_H #define PKTLINE_H -#include "git-compat-util.h" #include "strbuf.h" #include "sideband.h" diff --git a/repository.h b/repository.h index e8c67ffe165..15a8afc5fb5 100644 --- a/repository.h +++ b/repository.h @@ -1,7 +1,6 @@ #ifndef REPOSITORY_H #define REPOSITORY_H -#include "git-compat-util.h" #include "path.h" struct config_set; diff --git a/sub-process.h b/sub-process.h index e85f21fa1a7..6a61638a8ac 100644 --- a/sub-process.h +++ b/sub-process.h @@ -1,7 +1,6 @@ #ifndef SUBPROCESS_H #define SUBPROCESS_H -#include "git-compat-util.h" #include "hashmap.h" #include "run-command.h" diff --git a/trace.h b/trace.h index 4e771f86ac2..1a75824b15e 100644 --- a/trace.h +++ b/trace.h @@ -1,7 +1,6 @@ #ifndef TRACE_H #define TRACE_H -#include "git-compat-util.h" #include "strbuf.h" /**