patch, 3 partsfixes related to `make hdr-check`

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

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

Hi Denton,

On Mon, 23 Sep 2019, Denton Liu wrote:

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

Ciao, Dscho

Hi Dscho,

On Thu, Sep 26, 2019 at 02:49:55PM +0200, Johannes Schindelin wrote:

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

Hi,

On Thu, 26 Sep 2019, Denton Liu wrote:

Hahaha! ;-)

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

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

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

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

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

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

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

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.

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

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

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

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

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

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

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

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

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

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.

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

Hi Denton,

On Mon, 23 Sep 2019, Denton Liu wrote:

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

-- snipsnap --