[PATCH] wt-status: move #include "pathspec.h" to the header

[PATCH] wt-status: move #include "pathspec.h" to the header

[SZEDER Gábor]

The declaration of 'struct wt_status' requires the declararion of 'struct
pathspec'.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 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 <stdio.h>
 #include "string-list.h"
 #include "color.h"
+#include "pathspec.h"
 
 enum color_wt_status {
 	WT_STATUS_HEADER = 0,
-- 
2.5.0.415.g33d64ef

Re: [PATCH] wt-status: move #include "pathspec.h" to the header

[Junio C Hamano]

SZEDER Gábor <szeder@ira.uka.de> 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 <stdio.h> 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 <szeder@ira.uka.de>
> ---
>  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 <stdio.h>
>  #include "string-list.h"
>  #include "color.h"
> +#include "pathspec.h"
>  
>  enum color_wt_status {
>  	WT_STATUS_HEADER = 0,

Re: [PATCH] wt-status: move #include "pathspec.h" to the header

[Jeff King]

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 <stdio.h> 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.

Re: [PATCH] wt-status: move #include "pathspec.h" to the header

[SZEDER Gábor]

Quoting Junio C Hamano <gitster@pobox.com>:

> SZEDER Gábor <szeder@ira.uka.de> 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 <stdio.h> 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.