[PATCH 0/3] fixes related to `make hdr-check`

[PATCH 0/3] fixes related to `make hdr-check`

[Denton Liu]

This patchset relates to `make hdr-check`. The first patch addresses
getting it to run on platforms which require custom CFLAGS.

The other two patches address errors/warnings caught by actually running
`make hdr-check`.


Denton Liu (3):
  Makefile: use $(ALL_CFLAGS) in $(HCO) target
  apply.h: include missing header
  promisor-remote.h: include missing header

 Makefile          | 2 +-
 apply.h           | 1 +
 promisor-remote.h | 2 ++
 3 files changed, 4 insertions(+), 1 deletion(-)

-- 
2.23.0.565.g1cc52d20df

[PATCH 1/3] Makefile: use $(ALL_CFLAGS) in $(HCO) target

[Denton Liu]

On platforms that can run `make hdr-check` but require custom flags,
this target was failing because none of them were being passed to the
compiler. For example, on MacOS, the NO_OPENSSL flag was being set but
it was not being passed into compiler so the check was failing.

Pass $(ALL_CFLAGS) into the compiler for the $(HCO) target so that these
custom flags can be used.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index f879697ea3..d8df4e316b 100644
--- a/Makefile
+++ b/Makefile
@@ -2773,7 +2773,7 @@ CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H)))
 HCO = $(patsubst %.h,%.hco,$(CHK_HDRS))
 
 $(HCO): %.hco: %.h FORCE
-	$(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $<
+	$(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $(ALL_CFLAGS) $<
 
 .PHONY: hdr-check $(HCO)
 hdr-check: $(HCO)
-- 
2.23.0.565.g1cc52d20df

[PATCH 2/3] apply.h: include missing header

[Denton Liu]

When running `make hdr-check`, we got the following error messages:

	apply.h:146:22: error: use of undeclared identifier 'GIT_MAX_HEXSZ'
		char old_oid_prefix[GIT_MAX_HEXSZ + 1];
				    ^
	apply.h:147:22: error: use of undeclared identifier 'GIT_MAX_HEXSZ'
		char new_oid_prefix[GIT_MAX_HEXSZ + 1];
				    ^
	apply.h:151:33: error: array has incomplete element type 'struct object_id'
		struct object_id threeway_stage[3];
					       ^
	./strbuf.h:79:8: note: forward declaration of 'struct object_id'
	struct object_id;
	       ^
	3 errors generated.
	make: *** [apply.hco] Error 1

Include the missing "hash.h" header to fix these errors.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 apply.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/apply.h b/apply.h
index a795193435..da3d95fa50 100644
--- a/apply.h
+++ b/apply.h
@@ -1,6 +1,7 @@
 #ifndef APPLY_H
 #define APPLY_H
 
+#include "hash.h"
 #include "lockfile.h"
 #include "string-list.h"
 
-- 
2.23.0.565.g1cc52d20df

[PATCH 3/3] promisor-remote.h: include missing header

[Denton Liu]

When we ran `make hdr-check`, we got the following warning message:

	promisor-remote.h:21:46: warning: declaration of 'struct repository' will not be visible outside of this function [-Wvisibility]
	extern int promisor_remote_get_direct(struct repository *repo,
						     ^
	1 warning generated.

This was caused by a missing reference to `struct repository`, which is
defined in "repository.h".

Include this missing header to fix this warning.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 promisor-remote.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/promisor-remote.h b/promisor-remote.h
index 8200dfc940..76e8d86c7c 100644
--- a/promisor-remote.h
+++ b/promisor-remote.h
@@ -1,6 +1,8 @@
 #ifndef PROMISOR_REMOTE_H
 #define PROMISOR_REMOTE_H
 
+#include "repository.h"
+
 struct object_id;
 
 /*
-- 
2.23.0.565.g1cc52d20df

[PATCH 4/3] pack-bitmap.h: fix unused variable warning

[Denton Liu]

When we ran `make hdr-check`, we got the following warning on Arch Linux:

	pack-bitmap.h:20:19: error: ‘BITMAP_IDX_SIGNATURE’ defined but not used [-Werror=unused-const-variable=]
	   20 | static const char BITMAP_IDX_SIGNATURE[] = {'B', 'I', 'T', 'M'};
	      |                   ^~~~~~~~~~~~~~~~~~~~
	cc1: all warnings being treated as errors

"Use" the BITMAP_IDX_SIGNATURE variable by making the size of
bitmap_disk_header.magic equal to the size of BITMAP_IDX_SIGNATURE. An
alternative was to simply add MAYBE_UNUSED. However, this design was
chosen because we eliminate the magic number (4) in the process.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
I'm tacking this patch on since this warning didn't show up until I
compiled it on gcc 9.1.0.

 pack-bitmap.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/pack-bitmap.h b/pack-bitmap.h
index 00de3ec8e4..466c5afa09 100644
--- a/pack-bitmap.h
+++ b/pack-bitmap.h
@@ -9,16 +9,16 @@ struct commit;
 struct repository;
 struct rev_info;
 
+static const char BITMAP_IDX_SIGNATURE[] = {'B', 'I', 'T', 'M'};
+
 struct bitmap_disk_header {
-	char magic[4];
+	char magic[ARRAY_SIZE(BITMAP_IDX_SIGNATURE)];
 	uint16_t version;
 	uint16_t options;
 	uint32_t entry_count;
 	unsigned char checksum[GIT_MAX_RAWSZ];
 };
 
-static const char BITMAP_IDX_SIGNATURE[] = {'B', 'I', 'T', 'M'};
-
 #define NEEDS_BITMAP (1u<<22)
 
 enum pack_bitmap_opts {
-- 
2.23.0.248.g3a9dd8fb08

Re: [PATCH 4/3] pack-bitmap.h: fix unused variable warning

[Jeff King]

On Tue, Sep 24, 2019 at 02:08:53AM -0700, Denton Liu wrote:

> When we ran `make hdr-check`, we got the following warning on Arch Linux:
> 
> 	pack-bitmap.h:20:19: error: ‘BITMAP_IDX_SIGNATURE’ defined but not used [-Werror=unused-const-variable=]
> 	   20 | static const char BITMAP_IDX_SIGNATURE[] = {'B', 'I', 'T', 'M'};
> 	      |                   ^~~~~~~~~~~~~~~~~~~~
> 	cc1: all warnings being treated as errors
> 
> "Use" the BITMAP_IDX_SIGNATURE variable by making the size of
> bitmap_disk_header.magic equal to the size of BITMAP_IDX_SIGNATURE. An
> alternative was to simply add MAYBE_UNUSED. However, this design was
> chosen because we eliminate the magic number (4) in the process.

Yeah, I think this is a fine fix. Since it's needed in only a few files,
the more usual fix would be to not define it in the header in the first
place. Something like:

  extern const char BITMAP_IDX_SIGNATURE[4];

or whatever. But I think it's nice to keep it all together.

> I'm tacking this patch on since this warning didn't show up until I
> compiled it on gcc 9.1.0.

Curiously, I _don't_ see the warning with gcc 9.2.1. By my reading of
the manpage, this should be triggered by -Wunused-const-variable=2, but
not by "1" (the difference being whether it triggers for stuff in header
files). And only the latter is triggered by -Wall or -Wextra.

But another weirdness is that hdr-check is directly compiling the header
files. So I guess that fools it. But we don't pass any of the extra
diagnostic options there.  Have you put "-Wall" into your $(CC)?

Perhaps a more realistic hdr-check would be:

  {
    echo '#include "git-compat-util.h"'
    echo '#include "$<"'
  } >$*.hcc
  $(QUIET_HDR)$(CC) $(ALL_CFLAGS) -o /dev/null -c -xc $*.hcc

-Peff

Re: [PATCH 4/3] pack-bitmap.h: fix unused variable warning

[Jeff King]

On Tue, Sep 24, 2019 at 05:34:08PM -0400, Jeff King wrote:

> > I'm tacking this patch on since this warning didn't show up until I
> > compiled it on gcc 9.1.0.
> 
> Curiously, I _don't_ see the warning with gcc 9.2.1. By my reading of
> the manpage, this should be triggered by -Wunused-const-variable=2, but
> not by "1" (the difference being whether it triggers for stuff in header
> files). And only the latter is triggered by -Wall or -Wextra.
> 
> But another weirdness is that hdr-check is directly compiling the header
> files. So I guess that fools it. But we don't pass any of the extra
> diagnostic options there.  Have you put "-Wall" into your $(CC)?
> 
> Perhaps a more realistic hdr-check would be:
> 
>   {
>     echo '#include "git-compat-util.h"'
>     echo '#include "$<"'
>   } >$*.hcc
>   $(QUIET_HDR)$(CC) $(ALL_CFLAGS) -o /dev/null -c -xc $*.hcc

Oh, heh. I should have read your whole series (the mention of
pack-bitmaps in the subject got my attention).

So yeah, I think the problem is that you're using $(ALL_CFLAGS) with our
fake "compile the header" check, which does not reflect how our code is
really compiled. I think we should use a more accurate simulation, like
what I wrote above.

-Peff

[PATCH v2 0/4] fixes related to `make hdr-check`

[Denton Liu]

The first two patches fix errors causing `make hdr-check` to fail. The third is
legacy from v1 but provides code cleanup so we leave it. Finally, the last
patch is a patch which improves the portability and correctness of `hdr-check`.

Changes since v1:

* Reordered patches to put fixes first

* Took Peff's improvement suggestions for the $(HCO) target

* Added "pack-bitmap.h: fix unused variable warning"


Denton Liu (4):
  apply.h: include missing header
  promisor-remote.h: include missing header
  pack-bitmap.h: remove magic number
  Makefile: emulate compile in $(HCO) target better

 .gitignore        |  1 +
 Makefile          | 12 +++++++++---
 apply.h           |  1 +
 pack-bitmap.h     |  6 +++---
 promisor-remote.h |  2 ++
 5 files changed, 16 insertions(+), 6 deletions(-)

Range-diff against v1:
1:  0336d1345a < -:  ---------- Makefile: use $(ALL_CFLAGS) in $(HCO) target
2:  1fc6dfc5fa = 1:  74efb6c04c apply.h: include missing header
3:  8ccbd81673 = 2:  2befc450fb promisor-remote.h: include missing header
4:  a3a3357925 ! 3:  50e37c16f9 pack-bitmap.h: fix unused variable warning
    @@ Metadata
     Author: Denton Liu <liu.denton@gmail.com>
     
      ## Commit message ##
    -    pack-bitmap.h: fix unused variable warning
    +    pack-bitmap.h: remove magic number
     
    -    When we ran `make hdr-check`, we got the following warning on Arch Linux:
    +    When we ran `make hdr-check` with the following patch
    +
    +            diff --git a/Makefile b/Makefile
    +            index f879697ea3..d8df4e316b 100644
    +            --- a/Makefile
    +            +++ b/Makefile
    +            @@ -2773,7 +2773,7 @@ CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H)))
    +            HCO = $(patsubst %.h,%.hco,$(CHK_HDRS))
    +
    +            $(HCO): %.hco: %.h FORCE
    +            -       $(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $<
    +            +       $(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $(ALL_CFLAGS) $<
    +
    +            .PHONY: hdr-check $(HCO)
    +            hdr-check: $(HCO)
    +
    +    and with `DEVELOPER=1`, we got the following warning on Arch Linux:
     
                 pack-bitmap.h:20:19: error: ‘BITMAP_IDX_SIGNATURE’ defined but not used [-Werror=unused-const-variable=]
                    20 | static const char BITMAP_IDX_SIGNATURE[] = {'B', 'I', 'T', 'M'};
    @@ Commit message
                 cc1: all warnings being treated as errors
     
         "Use" the BITMAP_IDX_SIGNATURE variable by making the size of
    -    bitmap_disk_header.magic equal to the size of BITMAP_IDX_SIGNATURE. An
    -    alternative was to simply add MAYBE_UNUSED. However, this design was
    -    chosen because we eliminate the magic number (4) in the process.
    +    bitmap_disk_header.magic equal to the size of BITMAP_IDX_SIGNATURE,
    +    thereby eliminating the magic number (4).
    +
    +    An alternative was to simply add MAYBE_UNUSED, however that does not
    +    eliminate the magic number.
    +
    +    Another alternative was to change the definition to
    +
    +            extern const char BITMAP_IDX_SIGNATURE[4];
    +
    +    However, this design was also not chosen as the static definition allows
    +    us to keep the declaration together for readability along with removing
    +    the magic number.
     
      ## pack-bitmap.h ##
     @@ pack-bitmap.h: struct commit;
-:  ---------- > 4:  14def72319 Makefile: emulate compile in $(HCO) target better
-- 
2.23.0.248.g3a9dd8fb08

[PATCH v2 1/4] apply.h: include missing header

[Denton Liu]

When running `make hdr-check`, we got the following error messages:

	apply.h:146:22: error: use of undeclared identifier 'GIT_MAX_HEXSZ'
		char old_oid_prefix[GIT_MAX_HEXSZ + 1];
				    ^
	apply.h:147:22: error: use of undeclared identifier 'GIT_MAX_HEXSZ'
		char new_oid_prefix[GIT_MAX_HEXSZ + 1];
				    ^
	apply.h:151:33: error: array has incomplete element type 'struct object_id'
		struct object_id threeway_stage[3];
					       ^
	./strbuf.h:79:8: note: forward declaration of 'struct object_id'
	struct object_id;
	       ^
	3 errors generated.
	make: *** [apply.hco] Error 1

Include the missing "hash.h" header to fix these errors.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 apply.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/apply.h b/apply.h
index a795193435..da3d95fa50 100644
--- a/apply.h
+++ b/apply.h
@@ -1,6 +1,7 @@
 #ifndef APPLY_H
 #define APPLY_H
 
+#include "hash.h"
 #include "lockfile.h"
 #include "string-list.h"
 
-- 
2.23.0.248.g3a9dd8fb08

[PATCH v2 2/4] promisor-remote.h: include missing header

[Denton Liu]

When we ran `make hdr-check`, we got the following warning message:

	promisor-remote.h:21:46: warning: declaration of 'struct repository' will not be visible outside of this function [-Wvisibility]
	extern int promisor_remote_get_direct(struct repository *repo,
						     ^
	1 warning generated.

This was caused by a missing reference to `struct repository`, which is
defined in "repository.h".

Include this missing header to fix this warning.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 promisor-remote.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/promisor-remote.h b/promisor-remote.h
index 8200dfc940..76e8d86c7c 100644
--- a/promisor-remote.h
+++ b/promisor-remote.h
@@ -1,6 +1,8 @@
 #ifndef PROMISOR_REMOTE_H
 #define PROMISOR_REMOTE_H
 
+#include "repository.h"
+
 struct object_id;
 
 /*
-- 
2.23.0.248.g3a9dd8fb08

[PATCH v2 3/4] pack-bitmap.h: remove magic number

[Denton Liu]

When we ran `make hdr-check` with the following patch

	diff --git a/Makefile b/Makefile
	index f879697ea3..d8df4e316b 100644
	--- a/Makefile
	+++ b/Makefile
	@@ -2773,7 +2773,7 @@ CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H)))
	HCO = $(patsubst %.h,%.hco,$(CHK_HDRS))

	$(HCO): %.hco: %.h FORCE
	-	$(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $<
	+	$(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $(ALL_CFLAGS) $<

	.PHONY: hdr-check $(HCO)
	hdr-check: $(HCO)

and with `DEVELOPER=1`, we got the following warning on Arch Linux:

	pack-bitmap.h:20:19: error: ‘BITMAP_IDX_SIGNATURE’ defined but not used [-Werror=unused-const-variable=]
	   20 | static const char BITMAP_IDX_SIGNATURE[] = {'B', 'I', 'T', 'M'};
	      |                   ^~~~~~~~~~~~~~~~~~~~
	cc1: all warnings being treated as errors

"Use" the BITMAP_IDX_SIGNATURE variable by making the size of
bitmap_disk_header.magic equal to the size of BITMAP_IDX_SIGNATURE,
thereby eliminating the magic number (4).

An alternative was to simply add MAYBE_UNUSED, however that does not
eliminate the magic number.

Another alternative was to change the definition to

	extern const char BITMAP_IDX_SIGNATURE[4];

However, this design was also not chosen as the static definition allows
us to keep the declaration together for readability along with removing
the magic number.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 pack-bitmap.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/pack-bitmap.h b/pack-bitmap.h
index 00de3ec8e4..466c5afa09 100644
--- a/pack-bitmap.h
+++ b/pack-bitmap.h
@@ -9,16 +9,16 @@ struct commit;
 struct repository;
 struct rev_info;
 
+static const char BITMAP_IDX_SIGNATURE[] = {'B', 'I', 'T', 'M'};
+
 struct bitmap_disk_header {
-	char magic[4];
+	char magic[ARRAY_SIZE(BITMAP_IDX_SIGNATURE)];
 	uint16_t version;
 	uint16_t options;
 	uint32_t entry_count;
 	unsigned char checksum[GIT_MAX_RAWSZ];
 };
 
-static const char BITMAP_IDX_SIGNATURE[] = {'B', 'I', 'T', 'M'};
-
 #define NEEDS_BITMAP (1u<<22)
 
 enum pack_bitmap_opts {
-- 
2.23.0.248.g3a9dd8fb08

[PATCH v2 4/4] Makefile: emulate compile in $(HCO) target better

[Denton Liu]

Currently, when testing headers using `make hdr-check`, headers are
directly compiled. Although this seems to test the headers, this is too
strict since we treat the headers as C sources. As a result, this will
cause warnings to appear that would otherwise not, such as a static
variable definition intended for later use throwing a unused variable
warning.

In addition, on platforms that can run `make hdr-check` but require
custom flags, this target was failing because none of them were being
passed to the compiler. For example, on MacOS, the NO_OPENSSL flag was
being set but it was not being passed into compiler so the check was
failing.

Fix these problems by emulating the compile process better, including
test compiling dummy *.hcc C sources generated from the *.h files and
passing $(ALL_CFLAGS) into the compiler for the $(HCO) target so that
these custom flags can be used.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---

Peff, thanks for the suggestion! I modified it a little bit so that we
wouldn't have to keep regenerating the *.hcc files unnecessarily.

I also considered piping into the compiler's stdin directly which I know
works for GCC (and _probably_ Clang) but I opted against it because I'm
not sure it's portable for other compilers. Maybe it's alright for this
to be less portable since it's a developer target?

 .gitignore |  1 +
 Makefile   | 12 +++++++++---
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/.gitignore b/.gitignore
index 521d8f4fb4..34efe125cb 100644
--- a/.gitignore
+++ b/.gitignore
@@ -216,6 +216,7 @@
 /tags
 /TAGS
 /cscope*
+*.hcc
 *.obj
 *.lib
 *.res
diff --git a/Makefile b/Makefile
index f879697ea3..581cc617e3 100644
--- a/Makefile
+++ b/Makefile
@@ -1872,7 +1872,7 @@ ifndef V
 	QUIET_MSGFMT   = @echo '   ' MSGFMT $@;
 	QUIET_GCOV     = @echo '   ' GCOV $@;
 	QUIET_SP       = @echo '   ' SP $<;
-	QUIET_HDR      = @echo '   ' HDR $<;
+	QUIET_HDR      = @echo '   ' HDR $(<:hcc=h);
 	QUIET_RC       = @echo '   ' RC $@;
 	QUIET_SUBDIR0  = +@subdir=
 	QUIET_SUBDIR1  = ;$(NO_SUBDIR) echo '   ' SUBDIR $$subdir; \
@@ -2771,9 +2771,14 @@ ifndef GCRYPT_SHA256
 endif
 CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H)))
 HCO = $(patsubst %.h,%.hco,$(CHK_HDRS))
+HCC = $(HCO:hco=hcc)
 
-$(HCO): %.hco: %.h FORCE
-	$(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $<
+%.hcc: %.h
+	@echo '#include "git-compat-util.h"' >$@
+	@echo '#include "$<"' >>$@
+
+$(HCO): %.hco: %.hcc FORCE
+	$(QUIET_HDR)$(CC) $(ALL_CFLAGS) -o /dev/null -c -xc $<
 
 .PHONY: hdr-check $(HCO)
 hdr-check: $(HCO)
@@ -3082,6 +3087,7 @@ clean: profile-clean coverage-clean cocciclean
 	$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) git$X
 	$(RM) $(TEST_PROGRAMS)
 	$(RM) $(FUZZ_PROGRAMS)
+	$(RM) $(HCC)
 	$(RM) -r bin-wrappers $(dep_dirs)
 	$(RM) -r po/build/
 	$(RM) *.pyc *.pyo */*.pyc */*.pyo command-list.h $(ETAGS_TARGET) tags cscope*
-- 
2.23.0.248.g3a9dd8fb08

Re: [PATCH 1/3] Makefile: use $(ALL_CFLAGS) in $(HCO) target

[Johannes Schindelin]

Hi Denton,

On Mon, 23 Sep 2019, Denton Liu wrote:

> On platforms that can run `make hdr-check` but require custom flags,
> this target was failing because none of them were being passed to the
> compiler. For example, on MacOS, the NO_OPENSSL flag was being set but
> it was not being passed into compiler so the check was failing.
>
> Pass $(ALL_CFLAGS) into the compiler for the $(HCO) target so that these
> custom flags can be used.
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index f879697ea3..d8df4e316b 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2773,7 +2773,7 @@ CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H)))
>  HCO = $(patsubst %.h,%.hco,$(CHK_HDRS))
>
>  $(HCO): %.hco: %.h FORCE
> -	$(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $<
> +	$(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $(ALL_CFLAGS) $<

Isn't this essentially identical to what I sent in
https://public-inbox.org/git/pull.129.git.gitgitgadget@gmail.com/
?

Ciao,
Dscho

>
>  .PHONY: hdr-check $(HCO)
>  hdr-check: $(HCO)
> --
> 2.23.0.565.g1cc52d20df
>
>

Re: [PATCH 0/3] fixes related to `make hdr-check`

[Johannes Schindelin]

Hi Denton,

On Mon, 23 Sep 2019, Denton Liu wrote:

> This patchset relates to `make hdr-check`. The first patch addresses
> getting it to run on platforms which require custom CFLAGS.
>
> The other two patches address errors/warnings caught by actually running
> `make hdr-check`.
>
>
> Denton Liu (3):
>   Makefile: use $(ALL_CFLAGS) in $(HCO) target
>   apply.h: include missing header
>   promisor-remote.h: include missing header

These all make sense to me (including 4/3). I wonder whether we would
want a 5/3 that essentially does this:

-- snipsnap --
diff --git a/ci/run-static-analysis.sh b/ci/run-static-analysis.sh
index a19aa7ebbc0..65bcebda41a 100755
--- a/ci/run-static-analysis.sh
+++ b/ci/run-static-analysis.sh
@@ -26,4 +26,7 @@ then
 	exit 1
 fi

+make hdr-check ||
+exit 1
+
 save_good_tree

Re: [PATCH 1/3] Makefile: use $(ALL_CFLAGS) in $(HCO) target

[Denton Liu]

Hi Dscho,

On Thu, Sep 26, 2019 at 02:49:55PM +0200, Johannes Schindelin wrote:
> Hi Denton,
> 
> On Mon, 23 Sep 2019, Denton Liu wrote:
> 
> > On platforms that can run `make hdr-check` but require custom flags,
> > this target was failing because none of them were being passed to the
> > compiler. For example, on MacOS, the NO_OPENSSL flag was being set but
> > it was not being passed into compiler so the check was failing.
> >
> > Pass $(ALL_CFLAGS) into the compiler for the $(HCO) target so that these
> > custom flags can be used.
> >
> > Signed-off-by: Denton Liu <liu.denton@gmail.com>
> > ---
> >  Makefile | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/Makefile b/Makefile
> > index f879697ea3..d8df4e316b 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -2773,7 +2773,7 @@ CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H)))
> >  HCO = $(patsubst %.h,%.hco,$(CHK_HDRS))
> >
> >  $(HCO): %.hco: %.h FORCE
> > -	$(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $<
> > +	$(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $(ALL_CFLAGS) $<
> 
> Isn't this essentially identical to what I sent in
> https://public-inbox.org/git/pull.129.git.gitgitgadget@gmail.com/
> ?

Whoops, didn't notice this. I heard there's some quote about great minds
or something ;).

Anyway, this patch is superceded by v2 4/4 which should emulate the
compilation process even better, as suggested by Peff. The only problem
with that patch is that it splashes temporary *.hcc files everywhere but
I think that if it's only a developer-run target, it should be fine.

Thanks,

Denton

> 
> Ciao,
> Dscho
> 
> >
> >  .PHONY: hdr-check $(HCO)
> >  hdr-check: $(HCO)
> > --
> > 2.23.0.565.g1cc52d20df
> >
> >

Re: [PATCH 1/3] Makefile: use $(ALL_CFLAGS) in $(HCO) target

[Johannes Schindelin]

Hi,

On Thu, 26 Sep 2019, Denton Liu wrote:

> On Thu, Sep 26, 2019 at 02:49:55PM +0200, Johannes Schindelin wrote:
> >
> > On Mon, 23 Sep 2019, Denton Liu wrote:
> >
> > > On platforms that can run `make hdr-check` but require custom flags,
> > > this target was failing because none of them were being passed to the
> > > compiler. For example, on MacOS, the NO_OPENSSL flag was being set but
> > > it was not being passed into compiler so the check was failing.
> > >
> > > Pass $(ALL_CFLAGS) into the compiler for the $(HCO) target so that these
> > > custom flags can be used.
> > >
> > > Signed-off-by: Denton Liu <liu.denton@gmail.com>
> > > ---
> > >  Makefile | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/Makefile b/Makefile
> > > index f879697ea3..d8df4e316b 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -2773,7 +2773,7 @@ CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H)))
> > >  HCO = $(patsubst %.h,%.hco,$(CHK_HDRS))
> > >
> > >  $(HCO): %.hco: %.h FORCE
> > > -	$(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $<
> > > +	$(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $(ALL_CFLAGS) $<
> >
> > Isn't this essentially identical to what I sent in
> > https://public-inbox.org/git/pull.129.git.gitgitgadget@gmail.com/
> > ?
>
> Whoops, didn't notice this. I heard there's some quote about great minds
> or something ;).

Hahaha! ;-)

> Anyway, this patch is superceded by v2 4/4 which should emulate the
> compilation process even better, as suggested by Peff. The only
> problem with that patch is that it splashes temporary *.hcc files
> everywhere but I think that if it's only a developer-run target, it
> should be fine.

I agree that Peff's improvement is nice.

The only problem is that my patch made it into `next` already, according
to https://github.com/gitgitgadget/git/pull/129 (look for the labels on
the right side, or for the comments on the bottom), and
https://github.com/gitster/git/commit/a3f332f4fb10 agrees (look for
names of the branches containing that commit, under the commit message).

So we should explicitly as for my patch to be backed out from `next` and
be stopped from advancing to `master`.

Junio, would you kindly do that? According to the PR, you gave this
branch the name `js/honor-cflags-in-hdr-check`.

Thanks,
Dscho

Re: [PATCH 1/3] Makefile: use $(ALL_CFLAGS) in $(HCO) target

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> The only problem is that my patch made it into `next` already, according
> to https://github.com/gitgitgadget/git/pull/129 (look for the labels on
> the right side, or for the comments on the bottom), and
> https://github.com/gitster/git/commit/a3f332f4fb10 agrees (look for
> names of the branches containing that commit, under the commit message).
>
> So we should explicitly as for my patch to be backed out from `next` and
> be stopped from advancing to `master`.
>
> Junio, would you kindly do that? According to the PR, you gave this
> branch the name `js/honor-cflags-in-hdr-check`.

OK, so the topic should be dropped and replaced by the Denton's
series.  Thanks, will do.

Re: [PATCH v2 4/4] Makefile: emulate compile in $(HCO) target better

[Jeff King]

On Wed, Sep 25, 2019 at 01:21:01AM -0700, Denton Liu wrote:

> Fix these problems by emulating the compile process better, including
> test compiling dummy *.hcc C sources generated from the *.h files and
> passing $(ALL_CFLAGS) into the compiler for the $(HCO) target so that
> these custom flags can be used.
> 
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> 
> Peff, thanks for the suggestion! I modified it a little bit so that we
> wouldn't have to keep regenerating the *.hcc files unnecessarily.

What you have here looks good. I doubt it matters too much compared to
the cost of the compiler, but having them in their own rule makes it
easier to follow, I think.

> I also considered piping into the compiler's stdin directly which I know
> works for GCC (and _probably_ Clang) but I opted against it because I'm
> not sure it's portable for other compilers. Maybe it's alright for this
> to be less portable since it's a developer target?

Yeah, I'd worry slightly about portability. But it would be nice to
avoid generating more cruft. And I agree that most people running this
would be developers. Maybe it would make sense to float a separate patch
on top (that would make it easy to revert if somebody runs into a
problem).

Thanks for working on this (and the whole series looks good, including
the commit message changes you made regarding the bitmap code).

-Peff