threads / patch / 40141

patchwt-status: move #include "pathspec.h" to the header

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

## tl;dr

4 messages between Aug 20, 2015 and Aug 20, 2015. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

SZEDER Gábor· Aug 20, 2015, 14:06 UTC · lore

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(-)

Show changes to 2 files +1 −1

wt-status.c, wt-status.h

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

Junio C Hamano· Aug 20, 2015, 19:05 UTC · re: SZEDER Gábor · lore

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

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.

Show 28 quoted lines

> 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,

Jeff King· Aug 20, 2015, 20:38 UTC · re: Junio C Hamano · lore

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

On Thu, Aug 20, 2015 at 12:05:34PM -0700, Junio C Hamano wrote:

Show 18 quoted lines

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

SZEDER Gábor· Aug 20, 2015, 22:35 UTC · re: Junio C Hamano · lore

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

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

Show 10 quoted lines

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

Show 6 quoted lines

> 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.)

Show 15 quoted lines

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