# [PATCH] wt-status: move #include "pathspec.h" to the header 4 messages from 2015-08-20 to 2015-08-20. Participants: SZEDER Gábor, Junio C Hamano, Jeff King. Thread: https://gitlist.dev/t/40141 ## SZEDER Gábor, 2015-08-20 14:06 Subject: [PATCH] wt-status: move #include "pathspec.h" to the header Message-ID: <1440079587-26491-1-git-send-email-szeder@ira.uka.de> URL: https://gitlist.dev/e/1440079587-26491-1-git-send-email-szeder%40ira.uka.de ``` The declaration of 'struct wt_status' requires the declararion of 'struct pathspec'. Signed-off-by: SZEDER Gábor --- wt-status.c | 1 - wt-status.h | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/wt-status.c b/wt-status.c index 717fd48d13..c327fe8128 100644 --- a/wt-status.c +++ b/wt-status.c @@ -1,5 +1,4 @@ #include "cache.h" -#include "pathspec.h" #include "wt-status.h" #include "object.h" #include "dir.h" diff --git a/wt-status.h b/wt-status.h index e0a99f75c7..c9b3b744e9 100644 --- a/wt-status.h +++ b/wt-status.h @@ -4,6 +4,7 @@ #include #include "string-list.h" #include "color.h" +#include "pathspec.h" enum color_wt_status { WT_STATUS_HEADER = 0, -- 2.5.0.415.g33d64ef ``` ## Junio C Hamano, 2015-08-20 19:05 Subject: Re: [PATCH] wt-status: move #include "pathspec.h" to the header Message-ID: URL: https://gitlist.dev/e/xmqq61497pc1.fsf%40gitster.dls.corp.google.com In-Reply-To: <1440079587-26491-1-git-send-email-szeder@ira.uka.de> ``` SZEDER Gábor writes: > The declaration of 'struct wt_status' requires the declararion of 'struct > pathspec'. I think this is fine. I am guessing that you are saying it is wrong to force wt-status.c to include pathspec.h before including wt-status.h; I am fine with that. This is a tangent, but the above is different from saying that with a single liner test.c that has #include "wt-status.h" your compilation "cc -c test.c" should succeed. But for that goal, direct inclusion of to wt-status.h is also iffy. We include the system headers from git-compat-util.h because some platforms are picky about order of inclusion of system header files and definitions of feature test macros. Right now, the codebase is correct only because it is NOT our goal to guarantee that such a single-liner test.c file compiles. Instead, everybody is instructed to #include "git-compat-util.h" as the first thing, either directly or indirectly. So in that sense, we should also remove that inclusion from wt-status.h, I think. Thanks. > Signed-off-by: SZEDER Gábor > --- > wt-status.c | 1 - > wt-status.h | 1 + > 2 files changed, 1 insertion(+), 1 deletion(-) > > diff --git a/wt-status.c b/wt-status.c > index 717fd48d13..c327fe8128 100644 > --- a/wt-status.c > +++ b/wt-status.c > @@ -1,5 +1,4 @@ > #include "cache.h" > -#include "pathspec.h" > #include "wt-status.h" > #include "object.h" > #include "dir.h" > diff --git a/wt-status.h b/wt-status.h > index e0a99f75c7..c9b3b744e9 100644 > --- a/wt-status.h > +++ b/wt-status.h > @@ -4,6 +4,7 @@ > #include > #include "string-list.h" > #include "color.h" > +#include "pathspec.h" > > enum color_wt_status { > WT_STATUS_HEADER = 0, ``` ## Jeff King, 2015-08-20 20:38 Subject: Re: [PATCH] wt-status: move #include "pathspec.h" to the header Message-ID: <20150820203841.GA12386@sigill.intra.peff.net> URL: https://gitlist.dev/e/20150820203841.GA12386%40sigill.intra.peff.net In-Reply-To: ``` On Thu, Aug 20, 2015 at 12:05:34PM -0700, Junio C Hamano wrote: > This is a tangent, but the above is different from saying that with > a single liner test.c that has > > #include "wt-status.h" > > your compilation "cc -c test.c" should succeed. But for that goal, > direct inclusion of to wt-status.h is also iffy. We > include the system headers from git-compat-util.h because some > platforms are picky about order of inclusion of system header files > and definitions of feature test macros. > > Right now, the codebase is correct only because it is NOT our goal > to guarantee that such a single-liner test.c file compiles. > Instead, everybody is instructed to #include "git-compat-util.h" as > the first thing, either directly or indirectly. > > So in that sense, we should also remove that inclusion from > wt-status.h, I think. Yeah, I think that is actively wrong. Running[1]: git grep '#include <' -- '*.h' ':!git-compat-util.h' shows a few other weird ones. Things like zlib.h, pcre.h, etc, are probably OK. They are not really "system" headers, and it is probably OK as long as they come after git-compat-util (which they must, if they are in a header file). The inclusion of stdlib.h in compat/bswap.h is suspect (both are included by git-compat-util itself). But it is inside an #ifdef _MSC_VER block, so I cannot test that there is not some subtle interaction going on. A few headers include pthread.h. I would have thought that should go in git-compat-util for the usual reasons, but nobody has complained (and most files do not need it, though that has not generally stopped us from making git-compat-util a catch-all for other system files). -Peff [1] Too bad I needed the "--" separator there. Recent git nicely learned to DWIM "*.h" as a pathspec, but we do not seem to give the same treatment to "magic" pathspecs. ``` ## SZEDER Gábor, 2015-08-20 22:35 Subject: Re: [PATCH] wt-status: move #include "pathspec.h" to the header Message-ID: <20150821003506.Horde.cIQHHynKCAfMFot8jSGGhg7@webmail.informatik.kit.edu> URL: https://gitlist.dev/e/20150821003506.Horde.cIQHHynKCAfMFot8jSGGhg7%40webmail.informatik.kit.edu In-Reply-To: ``` Quoting Junio C Hamano : > SZEDER Gábor writes: > >> The declaration of 'struct wt_status' requires the declararion of 'struct >> pathspec'. > > I think this is fine. > > I am guessing that you are saying it is wrong to force wt-status.c > to include pathspec.h before including wt-status.h; I am fine with > that. > This is a tangent, but the above is different from saying that with > a single liner test.c that has > > #include "wt-status.h" > > your compilation "cc -c test.c" should succeed. Sort of, but not quite, rather a combination of both. My point is a builtin command, starting with #include "builtin.h" as it should, that wanted to use only struct wt_status_state and wt_status_get_state() from the wt-status machinery and has nothing to do with pathspecs, yet it's not sufficient to #include "wt-status.h", because the required pathspec.h is included in the .c file instead of the header itsef. (Actually, that's exactly how I noticed.) > But for that goal, > direct inclusion of to wt-status.h is also iffy. We > include the system headers from git-compat-util.h because some > platforms are picky about order of inclusion of system header files > and definitions of feature test macros. > > Right now, the codebase is correct only because it is NOT our goal > to guarantee that such a single-liner test.c file compiles. > Instead, everybody is instructed to #include "git-compat-util.h" as > the first thing, either directly or indirectly. > > So in that sense, we should also remove that inclusion from > wt-status.h, I think. > > Thanks. ```