{"thread":{"id":"51904","subject":"[PATCH 0/3] fixes related to `make hdr-check`","startedAt":"2019-09-23T18:34:55Z","lastAt":"2019-10-02T15:41:33Z","messageCount":18,"participants":["Denton Liu","Jeff King","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"382780","messageId":"cover.1569263631.git.liu.denton@gmail.com","threadId":"51904","inReplyTo":null,"subject":"[PATCH 0/3] fixes related to `make hdr-check`","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-09-23T18:34:51Z","receivedAt":"2019-09-23T18:34:55Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"This patchset relates to `make hdr-check`. The first patch addresses\ngetting it to run on platforms which require custom CFLAGS.\n\nThe other two patches address errors/warnings caught by actually running\n`make hdr-check`.\n\n\nDenton Liu (3):\n Makefile: use $(ALL_CFLAGS) in $(HCO) target\n apply.h: include missing header\n promisor-remote.h: include missing header\n\n Makefile | 2 +-\n apply.h | 1 +\n promisor-remote.h | 2 ++\n 3 files changed, 4 insertions(+), 1 deletion(-)\n\n-- \n2.23.0.565.g1cc52d20df\n\n"},{"id":"382781","messageId":"0336d1345abfef563d9228e250316cc58a01f2b3.1569263631.git.liu.denton@gmail.com","threadId":"51904","inReplyTo":"cover.1569263631.git.liu.denton@gmail.com","subject":"[PATCH 1/3] Makefile: use $(ALL_CFLAGS) in $(HCO) target","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-09-23T18:34:53Z","receivedAt":"2019-09-23T18:34:58Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"On platforms that can run `make hdr-check` but require custom flags,\nthis target was failing because none of them were being passed to the\ncompiler. For example, on MacOS, the NO_OPENSSL flag was being set but\nit was not being passed into compiler so the check was failing.\n\nPass $(ALL_CFLAGS) into the compiler for the $(HCO) target so that these\ncustom flags can be used.\n\nSigned-off-by: Denton Liu \n---\n Makefile | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex f879697ea3..d8df4e316b 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2773,7 +2773,7 @@ CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H)))\n HCO = $(patsubst %.h,%.hco,$(CHK_HDRS))\n \n $(HCO): %.hco: %.h FORCE\n-\t$(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $<\n+\t$(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $(ALL_CFLAGS) $<\n \n .PHONY: hdr-check $(HCO)\n hdr-check: $(HCO)\n-- \n2.23.0.565.g1cc52d20df\n\n"},{"id":"382782","messageId":"1fc6dfc5fa45eec6ed20a06efb445c344b8a64af.1569263631.git.liu.denton@gmail.com","threadId":"51904","inReplyTo":"cover.1569263631.git.liu.denton@gmail.com","subject":"[PATCH 2/3] apply.h: include missing header","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-09-23T18:34:55Z","receivedAt":"2019-09-23T18:34:59Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"When running `make hdr-check`, we got the following error messages:\n\n\tapply.h:146:22: error: use of undeclared identifier 'GIT_MAX_HEXSZ'\n\t\tchar old_oid_prefix[GIT_MAX_HEXSZ + 1];\n\t\t\t\t ^\n\tapply.h:147:22: error: use of undeclared identifier 'GIT_MAX_HEXSZ'\n\t\tchar new_oid_prefix[GIT_MAX_HEXSZ + 1];\n\t\t\t\t ^\n\tapply.h:151:33: error: array has incomplete element type 'struct object_id'\n\t\tstruct object_id threeway_stage[3];\n\t\t\t\t\t ^\n\t./strbuf.h:79:8: note: forward declaration of 'struct object_id'\n\tstruct object_id;\n\t ^\n\t3 errors generated.\n\tmake: *** [apply.hco] Error 1\n\nInclude the missing \"hash.h\" header to fix these errors.\n\nSigned-off-by: Denton Liu \n---\n apply.h | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/apply.h b/apply.h\nindex a795193435..da3d95fa50 100644\n--- a/apply.h\n+++ b/apply.h\n@@ -1,6 +1,7 @@\n #ifndef APPLY_H\n #define APPLY_H\n \n+#include \"hash.h\"\n #include \"lockfile.h\"\n #include \"string-list.h\"\n \n-- \n2.23.0.565.g1cc52d20df\n\n"},{"id":"382783","messageId":"8ccbd81673dc83e3292e7b1227b5acf536bcd2b3.1569263631.git.liu.denton@gmail.com","threadId":"51904","inReplyTo":"cover.1569263631.git.liu.denton@gmail.com","subject":"[PATCH 3/3] promisor-remote.h: include missing header","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-09-23T18:34:58Z","receivedAt":"2019-09-23T18:35:02Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"When we ran `make hdr-check`, we got the following warning message:\n\n\tpromisor-remote.h:21:46: warning: declaration of 'struct repository' will not be visible outside of this function [-Wvisibility]\n\textern int promisor_remote_get_direct(struct repository *repo,\n\t\t\t\t\t\t ^\n\t1 warning generated.\n\nThis was caused by a missing reference to `struct repository`, which is\ndefined in \"repository.h\".\n\nInclude this missing header to fix this warning.\n\nSigned-off-by: Denton Liu \n---\n promisor-remote.h | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/promisor-remote.h b/promisor-remote.h\nindex 8200dfc940..76e8d86c7c 100644\n--- a/promisor-remote.h\n+++ b/promisor-remote.h\n@@ -1,6 +1,8 @@\n #ifndef PROMISOR_REMOTE_H\n #define PROMISOR_REMOTE_H\n \n+#include \"repository.h\"\n+\n struct object_id;\n \n /*\n-- \n2.23.0.565.g1cc52d20df\n\n"},{"id":"382839","messageId":"a3a3357925cb144deb35b298cbe2796934fa0673.1569315905.git.liu.denton@gmail.com","threadId":"51904","inReplyTo":"cover.1569263631.git.liu.denton@gmail.com","subject":"[PATCH 4/3] pack-bitmap.h: fix unused variable warning","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-09-24T09:08:53Z","receivedAt":"2019-09-24T09:08:59Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"When we ran `make hdr-check`, we got the following warning on Arch Linux:\n\n\tpack-bitmap.h:20:19: error: ‘BITMAP_IDX_SIGNATURE’ defined but not used [-Werror=unused-const-variable=]\n\t 20 | static const char BITMAP_IDX_SIGNATURE[] = {'B', 'I', 'T', 'M'};\n\t | ^~~~~~~~~~~~~~~~~~~~\n\tcc1: all warnings being treated as errors\n\n\"Use\" the BITMAP_IDX_SIGNATURE variable by making the size of\nbitmap_disk_header.magic equal to the size of BITMAP_IDX_SIGNATURE. An\nalternative was to simply add MAYBE_UNUSED. However, this design was\nchosen because we eliminate the magic number (4) in the process.\n\nSigned-off-by: Denton Liu \n---\nI'm tacking this patch on since this warning didn't show up until I\ncompiled it on gcc 9.1.0.\n\n pack-bitmap.h | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/pack-bitmap.h b/pack-bitmap.h\nindex 00de3ec8e4..466c5afa09 100644\n--- a/pack-bitmap.h\n+++ b/pack-bitmap.h\n@@ -9,16 +9,16 @@ struct commit;\n struct repository;\n struct rev_info;\n \n+static const char BITMAP_IDX_SIGNATURE[] = {'B', 'I', 'T', 'M'};\n+\n struct bitmap_disk_header {\n-\tchar magic[4];\n+\tchar magic[ARRAY_SIZE(BITMAP_IDX_SIGNATURE)];\n \tuint16_t version;\n \tuint16_t options;\n \tuint32_t entry_count;\n \tunsigned char checksum[GIT_MAX_RAWSZ];\n };\n \n-static const char BITMAP_IDX_SIGNATURE[] = {'B', 'I', 'T', 'M'};\n-\n #define NEEDS_BITMAP (1u<<22)\n \n enum pack_bitmap_opts {\n-- \n2.23.0.248.g3a9dd8fb08\n\n"},{"id":"382875","messageId":"20190924213408.GD20858@sigill.intra.peff.net","threadId":"51904","inReplyTo":"a3a3357925cb144deb35b298cbe2796934fa0673.1569315905.git.liu.denton@gmail.com","subject":"Re: [PATCH 4/3] pack-bitmap.h: fix unused variable warning","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-24T21:34:08Z","receivedAt":"2019-09-24T21:34:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 24, 2019 at 02:08:53AM -0700, Denton Liu wrote:\n\n> When we ran `make hdr-check`, we got the following warning on Arch Linux:\n> \n> \tpack-bitmap.h:20:19: error: ‘BITMAP_IDX_SIGNATURE’ defined but not used [-Werror=unused-const-variable=]\n> \t 20 | static const char BITMAP_IDX_SIGNATURE[] = {'B', 'I', 'T', 'M'};\n> \t | ^~~~~~~~~~~~~~~~~~~~\n> \tcc1: all warnings being treated as errors\n> \n> \"Use\" the BITMAP_IDX_SIGNATURE variable by making the size of\n> bitmap_disk_header.magic equal to the size of BITMAP_IDX_SIGNATURE. An\n> alternative was to simply add MAYBE_UNUSED. However, this design was\n> chosen because we eliminate the magic number (4) in the process.\n\nYeah, I think this is a fine fix. Since it's needed in only a few files,\nthe more usual fix would be to not define it in the header in the first\nplace. Something like:\n\n extern const char BITMAP_IDX_SIGNATURE[4];\n\nor whatever. But I think it's nice to keep it all together.\n\n> I'm tacking this patch on since this warning didn't show up until I\n> compiled it on gcc 9.1.0.\n\nCuriously, I _don't_ see the warning with gcc 9.2.1. By my reading of\nthe manpage, this should be triggered by -Wunused-const-variable=2, but\nnot by \"1\" (the difference being whether it triggers for stuff in header\nfiles). And only the latter is triggered by -Wall or -Wextra.\n\nBut another weirdness is that hdr-check is directly compiling the header\nfiles. So I guess that fools it. But we don't pass any of the extra\ndiagnostic options there. Have you put \"-Wall\" into your $(CC)?\n\nPerhaps a more realistic hdr-check would be:\n\n {\n echo '#include \"git-compat-util.h\"'\n echo '#include \"$<\"'\n } >$*.hcc\n $(QUIET_HDR)$(CC) $(ALL_CFLAGS) -o /dev/null -c -xc $*.hcc\n\n-Peff\n"},{"id":"382877","messageId":"20190924213847.GF20858@sigill.intra.peff.net","threadId":"51904","inReplyTo":"20190924213408.GD20858@sigill.intra.peff.net","subject":"Re: [PATCH 4/3] pack-bitmap.h: fix unused variable warning","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-24T21:38:48Z","receivedAt":"2019-09-24T21:38:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 24, 2019 at 05:34:08PM -0400, Jeff King wrote:\n\n> > I'm tacking this patch on since this warning didn't show up until I\n> > compiled it on gcc 9.1.0.\n> \n> Curiously, I _don't_ see the warning with gcc 9.2.1. By my reading of\n> the manpage, this should be triggered by -Wunused-const-variable=2, but\n> not by \"1\" (the difference being whether it triggers for stuff in header\n> files). And only the latter is triggered by -Wall or -Wextra.\n> \n> But another weirdness is that hdr-check is directly compiling the header\n> files. So I guess that fools it. But we don't pass any of the extra\n> diagnostic options there. Have you put \"-Wall\" into your $(CC)?\n> \n> Perhaps a more realistic hdr-check would be:\n> \n> {\n> echo '#include \"git-compat-util.h\"'\n> echo '#include \"$<\"'\n> } >$*.hcc\n> $(QUIET_HDR)$(CC) $(ALL_CFLAGS) -o /dev/null -c -xc $*.hcc\n\nOh, heh. I should have read your whole series (the mention of\npack-bitmaps in the subject got my attention).\n\nSo yeah, I think the problem is that you're using $(ALL_CFLAGS) with our\nfake \"compile the header\" check, which does not reflect how our code is\nreally compiled. I think we should use a more accurate simulation, like\nwhat I wrote above.\n\n-Peff\n"},{"id":"382897","messageId":"cover.1569398897.git.liu.denton@gmail.com","threadId":"51904","inReplyTo":"cover.1569263631.git.liu.denton@gmail.com","subject":"[PATCH v2 0/4] fixes related to `make hdr-check`","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-09-25T08:20:49Z","receivedAt":"2019-09-25T08:20:55Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The first two patches fix errors causing `make hdr-check` to fail. The third is\nlegacy from v1 but provides code cleanup so we leave it. Finally, the last\npatch is a patch which improves the portability and correctness of `hdr-check`.\n\nChanges since v1:\n\n* Reordered patches to put fixes first\n\n* Took Peff's improvement suggestions for the $(HCO) target\n\n* Added \"pack-bitmap.h: fix unused variable warning\"\n\n\nDenton Liu (4):\n apply.h: include missing header\n promisor-remote.h: include missing header\n pack-bitmap.h: remove magic number\n Makefile: emulate compile in $(HCO) target better\n\n .gitignore | 1 +\n Makefile | 12 +++++++++---\n apply.h | 1 +\n pack-bitmap.h | 6 +++---\n promisor-remote.h | 2 ++\n 5 files changed, 16 insertions(+), 6 deletions(-)\n\nRange-diff against v1:\n1: 0336d1345a < -: ---------- Makefile: use $(ALL_CFLAGS) in $(HCO) target\n2: 1fc6dfc5fa = 1: 74efb6c04c apply.h: include missing header\n3: 8ccbd81673 = 2: 2befc450fb promisor-remote.h: include missing header\n4: a3a3357925 ! 3: 50e37c16f9 pack-bitmap.h: fix unused variable warning\n @@ Metadata\n Author: Denton Liu \n \n ## Commit message ##\n - pack-bitmap.h: fix unused variable warning\n + pack-bitmap.h: remove magic number\n \n - When we ran `make hdr-check`, we got the following warning on Arch Linux:\n + When we ran `make hdr-check` with the following patch\n +\n + diff --git a/Makefile b/Makefile\n + index f879697ea3..d8df4e316b 100644\n + --- a/Makefile\n + +++ b/Makefile\n + @@ -2773,7 +2773,7 @@ CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H)))\n + HCO = $(patsubst %.h,%.hco,$(CHK_HDRS))\n +\n + $(HCO): %.hco: %.h FORCE\n + - $(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $<\n + + $(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $(ALL_CFLAGS) $<\n +\n + .PHONY: hdr-check $(HCO)\n + hdr-check: $(HCO)\n +\n + and with `DEVELOPER=1`, we got the following warning on Arch Linux:\n \n pack-bitmap.h:20:19: error: ‘BITMAP_IDX_SIGNATURE’ defined but not used [-Werror=unused-const-variable=]\n 20 | static const char BITMAP_IDX_SIGNATURE[] = {'B', 'I', 'T', 'M'};\n @@ Commit message\n cc1: all warnings being treated as errors\n \n \"Use\" the BITMAP_IDX_SIGNATURE variable by making the size of\n - bitmap_disk_header.magic equal to the size of BITMAP_IDX_SIGNATURE. An\n - alternative was to simply add MAYBE_UNUSED. However, this design was\n - chosen because we eliminate the magic number (4) in the process.\n + bitmap_disk_header.magic equal to the size of BITMAP_IDX_SIGNATURE,\n + thereby eliminating the magic number (4).\n +\n + An alternative was to simply add MAYBE_UNUSED, however that does not\n + eliminate the magic number.\n +\n + Another alternative was to change the definition to\n +\n + extern const char BITMAP_IDX_SIGNATURE[4];\n +\n + However, this design was also not chosen as the static definition allows\n + us to keep the declaration together for readability along with removing\n + the magic number.\n \n ## pack-bitmap.h ##\n @@ pack-bitmap.h: struct commit;\n-: ---------- > 4: 14def72319 Makefile: emulate compile in $(HCO) target better\n-- \n2.23.0.248.g3a9dd8fb08\n\n"},{"id":"382898","messageId":"74efb6c04c1081d73be4c2666fa45ff2807464c3.1569398897.git.liu.denton@gmail.com","threadId":"51904","inReplyTo":"cover.1569398897.git.liu.denton@gmail.com","subject":"[PATCH v2 1/4] apply.h: include missing header","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-09-25T08:20:53Z","receivedAt":"2019-09-25T08:20:59Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"When running `make hdr-check`, we got the following error messages:\n\n\tapply.h:146:22: error: use of undeclared identifier 'GIT_MAX_HEXSZ'\n\t\tchar old_oid_prefix[GIT_MAX_HEXSZ + 1];\n\t\t\t\t ^\n\tapply.h:147:22: error: use of undeclared identifier 'GIT_MAX_HEXSZ'\n\t\tchar new_oid_prefix[GIT_MAX_HEXSZ + 1];\n\t\t\t\t ^\n\tapply.h:151:33: error: array has incomplete element type 'struct object_id'\n\t\tstruct object_id threeway_stage[3];\n\t\t\t\t\t ^\n\t./strbuf.h:79:8: note: forward declaration of 'struct object_id'\n\tstruct object_id;\n\t ^\n\t3 errors generated.\n\tmake: *** [apply.hco] Error 1\n\nInclude the missing \"hash.h\" header to fix these errors.\n\nSigned-off-by: Denton Liu \n---\n apply.h | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/apply.h b/apply.h\nindex a795193435..da3d95fa50 100644\n--- a/apply.h\n+++ b/apply.h\n@@ -1,6 +1,7 @@\n #ifndef APPLY_H\n #define APPLY_H\n \n+#include \"hash.h\"\n #include \"lockfile.h\"\n #include \"string-list.h\"\n \n-- \n2.23.0.248.g3a9dd8fb08\n\n"},{"id":"382899","messageId":"2befc450fb04dde1f676ec9d0cf04da9b1a1ca70.1569398897.git.liu.denton@gmail.com","threadId":"51904","inReplyTo":"cover.1569398897.git.liu.denton@gmail.com","subject":"[PATCH v2 2/4] promisor-remote.h: include missing header","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-09-25T08:20:56Z","receivedAt":"2019-09-25T08:21:01Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"When we ran `make hdr-check`, we got the following warning message:\n\n\tpromisor-remote.h:21:46: warning: declaration of 'struct repository' will not be visible outside of this function [-Wvisibility]\n\textern int promisor_remote_get_direct(struct repository *repo,\n\t\t\t\t\t\t ^\n\t1 warning generated.\n\nThis was caused by a missing reference to `struct repository`, which is\ndefined in \"repository.h\".\n\nInclude this missing header to fix this warning.\n\nSigned-off-by: Denton Liu \n---\n promisor-remote.h | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/promisor-remote.h b/promisor-remote.h\nindex 8200dfc940..76e8d86c7c 100644\n--- a/promisor-remote.h\n+++ b/promisor-remote.h\n@@ -1,6 +1,8 @@\n #ifndef PROMISOR_REMOTE_H\n #define PROMISOR_REMOTE_H\n \n+#include \"repository.h\"\n+\n struct object_id;\n \n /*\n-- \n2.23.0.248.g3a9dd8fb08\n\n"},{"id":"382900","messageId":"50e37c16f9715bb6bc41940545c779f6aa9f6be4.1569398897.git.liu.denton@gmail.com","threadId":"51904","inReplyTo":"cover.1569398897.git.liu.denton@gmail.com","subject":"[PATCH v2 3/4] pack-bitmap.h: remove magic number","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-09-25T08:20:59Z","receivedAt":"2019-09-25T08:21:03Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"When we ran `make hdr-check` with the following patch\n\n\tdiff --git a/Makefile b/Makefile\n\tindex f879697ea3..d8df4e316b 100644\n\t--- a/Makefile\n\t+++ b/Makefile\n\t@@ -2773,7 +2773,7 @@ CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H)))\n\tHCO = $(patsubst %.h,%.hco,$(CHK_HDRS))\n\n\t$(HCO): %.hco: %.h FORCE\n\t-\t$(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $<\n\t+\t$(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $(ALL_CFLAGS) $<\n\n\t.PHONY: hdr-check $(HCO)\n\thdr-check: $(HCO)\n\nand with `DEVELOPER=1`, we got the following warning on Arch Linux:\n\n\tpack-bitmap.h:20:19: error: ‘BITMAP_IDX_SIGNATURE’ defined but not used [-Werror=unused-const-variable=]\n\t 20 | static const char BITMAP_IDX_SIGNATURE[] = {'B', 'I', 'T', 'M'};\n\t | ^~~~~~~~~~~~~~~~~~~~\n\tcc1: all warnings being treated as errors\n\n\"Use\" the BITMAP_IDX_SIGNATURE variable by making the size of\nbitmap_disk_header.magic equal to the size of BITMAP_IDX_SIGNATURE,\nthereby eliminating the magic number (4).\n\nAn alternative was to simply add MAYBE_UNUSED, however that does not\neliminate the magic number.\n\nAnother alternative was to change the definition to\n\n\textern const char BITMAP_IDX_SIGNATURE[4];\n\nHowever, this design was also not chosen as the static definition allows\nus to keep the declaration together for readability along with removing\nthe magic number.\n\nSigned-off-by: Denton Liu \n---\n pack-bitmap.h | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/pack-bitmap.h b/pack-bitmap.h\nindex 00de3ec8e4..466c5afa09 100644\n--- a/pack-bitmap.h\n+++ b/pack-bitmap.h\n@@ -9,16 +9,16 @@ struct commit;\n struct repository;\n struct rev_info;\n \n+static const char BITMAP_IDX_SIGNATURE[] = {'B', 'I', 'T', 'M'};\n+\n struct bitmap_disk_header {\n-\tchar magic[4];\n+\tchar magic[ARRAY_SIZE(BITMAP_IDX_SIGNATURE)];\n \tuint16_t version;\n \tuint16_t options;\n \tuint32_t entry_count;\n \tunsigned char checksum[GIT_MAX_RAWSZ];\n };\n \n-static const char BITMAP_IDX_SIGNATURE[] = {'B', 'I', 'T', 'M'};\n-\n #define NEEDS_BITMAP (1u<<22)\n \n enum pack_bitmap_opts {\n-- \n2.23.0.248.g3a9dd8fb08\n\n"},{"id":"382901","messageId":"14def72319521d7380fb6a8ec570d014c0f5361b.1569398897.git.liu.denton@gmail.com","threadId":"51904","inReplyTo":"cover.1569398897.git.liu.denton@gmail.com","subject":"[PATCH v2 4/4] Makefile: emulate compile in $(HCO) target better","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-09-25T08:21:01Z","receivedAt":"2019-09-25T08:21:06Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Currently, when testing headers using `make hdr-check`, headers are\ndirectly compiled. Although this seems to test the headers, this is too\nstrict since we treat the headers as C sources. As a result, this will\ncause warnings to appear that would otherwise not, such as a static\nvariable definition intended for later use throwing a unused variable\nwarning.\n\nIn addition, on platforms that can run `make hdr-check` but require\ncustom flags, this target was failing because none of them were being\npassed to the compiler. For example, on MacOS, the NO_OPENSSL flag was\nbeing set but it was not being passed into compiler so the check was\nfailing.\n\nFix these problems by emulating the compile process better, including\ntest compiling dummy *.hcc C sources generated from the *.h files and\npassing $(ALL_CFLAGS) into the compiler for the $(HCO) target so that\nthese custom flags can be used.\n\nHelped-by: Jeff King \nSigned-off-by: Denton Liu \n---\n\nPeff, thanks for the suggestion! I modified it a little bit so that we\nwouldn't have to keep regenerating the *.hcc files unnecessarily.\n\nI also considered piping into the compiler's stdin directly which I know\nworks for GCC (and _probably_ Clang) but I opted against it because I'm\nnot sure it's portable for other compilers. Maybe it's alright for this\nto be less portable since it's a developer target?\n\n .gitignore | 1 +\n Makefile | 12 +++++++++---\n 2 files changed, 10 insertions(+), 3 deletions(-)\n\ndiff --git a/.gitignore b/.gitignore\nindex 521d8f4fb4..34efe125cb 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -216,6 +216,7 @@\n /tags\n /TAGS\n /cscope*\n+*.hcc\n *.obj\n *.lib\n *.res\ndiff --git a/Makefile b/Makefile\nindex f879697ea3..581cc617e3 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1872,7 +1872,7 @@ ifndef V\n \tQUIET_MSGFMT = @echo ' ' MSGFMT $@;\n \tQUIET_GCOV = @echo ' ' GCOV $@;\n \tQUIET_SP = @echo ' ' SP $<;\n-\tQUIET_HDR = @echo ' ' HDR $<;\n+\tQUIET_HDR = @echo ' ' HDR $(<:hcc=h);\n \tQUIET_RC = @echo ' ' RC $@;\n \tQUIET_SUBDIR0 = +@subdir=\n \tQUIET_SUBDIR1 = ;$(NO_SUBDIR) echo ' ' SUBDIR $$subdir; \\\n@@ -2771,9 +2771,14 @@ ifndef GCRYPT_SHA256\n endif\n CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H)))\n HCO = $(patsubst %.h,%.hco,$(CHK_HDRS))\n+HCC = $(HCO:hco=hcc)\n \n-$(HCO): %.hco: %.h FORCE\n-\t$(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $<\n+%.hcc: %.h\n+\t@echo '#include \"git-compat-util.h\"' >$@\n+\t@echo '#include \"$<\"' >>$@\n+\n+$(HCO): %.hco: %.hcc FORCE\n+\t$(QUIET_HDR)$(CC) $(ALL_CFLAGS) -o /dev/null -c -xc $<\n \n .PHONY: hdr-check $(HCO)\n hdr-check: $(HCO)\n@@ -3082,6 +3087,7 @@ clean: profile-clean coverage-clean cocciclean\n \t$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) git$X\n \t$(RM) $(TEST_PROGRAMS)\n \t$(RM) $(FUZZ_PROGRAMS)\n+\t$(RM) $(HCC)\n \t$(RM) -r bin-wrappers $(dep_dirs)\n \t$(RM) -r po/build/\n \t$(RM) *.pyc *.pyo */*.pyc */*.pyo command-list.h $(ETAGS_TARGET) tags cscope*\n-- \n2.23.0.248.g3a9dd8fb08\n\n"},{"id":"382979","messageId":"nycvar.QRO.7.76.6.1909261449230.15067@tvgsbejvaqbjf.bet","threadId":"51904","inReplyTo":"0336d1345abfef563d9228e250316cc58a01f2b3.1569263631.git.liu.denton@gmail.com","subject":"Re: [PATCH 1/3] Makefile: use $(ALL_CFLAGS) in $(HCO) target","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-09-26T12:49:55Z","receivedAt":"2019-09-26T12:50:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Denton,\n\nOn Mon, 23 Sep 2019, Denton Liu wrote:\n\n> On platforms that can run `make hdr-check` but require custom flags,\n> this target was failing because none of them were being passed to the\n> compiler. For example, on MacOS, the NO_OPENSSL flag was being set but\n> it was not being passed into compiler so the check was failing.\n>\n> Pass $(ALL_CFLAGS) into the compiler for the $(HCO) target so that these\n> custom flags can be used.\n>\n> Signed-off-by: Denton Liu \n> ---\n> Makefile | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/Makefile b/Makefile\n> index f879697ea3..d8df4e316b 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -2773,7 +2773,7 @@ CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H)))\n> HCO = $(patsubst %.h,%.hco,$(CHK_HDRS))\n>\n> $(HCO): %.hco: %.h FORCE\n> -\t$(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $<\n> +\t$(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $(ALL_CFLAGS) $<\n\nIsn't this essentially identical to what I sent in\nhttps://public-inbox.org/git/pull.129.git.gitgitgadget@gmail.com/\n?\n\nCiao,\nDscho\n\n>\n> .PHONY: hdr-check $(HCO)\n> hdr-check: $(HCO)\n> --\n> 2.23.0.565.g1cc52d20df\n>\n>\n"},{"id":"382980","messageId":"nycvar.QRO.7.76.6.1909261452340.15067@tvgsbejvaqbjf.bet","threadId":"51904","inReplyTo":"cover.1569263631.git.liu.denton@gmail.com","subject":"Re: [PATCH 0/3] fixes related to `make hdr-check`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-09-26T12:57:27Z","receivedAt":"2019-09-26T12:57:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Denton,\n\nOn Mon, 23 Sep 2019, Denton Liu wrote:\n\n> This patchset relates to `make hdr-check`. The first patch addresses\n> getting it to run on platforms which require custom CFLAGS.\n>\n> The other two patches address errors/warnings caught by actually running\n> `make hdr-check`.\n>\n>\n> Denton Liu (3):\n> Makefile: use $(ALL_CFLAGS) in $(HCO) target\n> apply.h: include missing header\n> promisor-remote.h: include missing header\n\nThese all make sense to me (including 4/3). I wonder whether we would\nwant a 5/3 that essentially does this:\n\n-- snipsnap --\ndiff --git a/ci/run-static-analysis.sh b/ci/run-static-analysis.sh\nindex a19aa7ebbc0..65bcebda41a 100755\n--- a/ci/run-static-analysis.sh\n+++ b/ci/run-static-analysis.sh\n@@ -26,4 +26,7 @@ then\n \texit 1\n fi\n\n+make hdr-check ||\n+exit 1\n+\n save_good_tree\n"},{"id":"383001","messageId":"20190926173801.GA42456@generichostname","threadId":"51904","inReplyTo":"nycvar.QRO.7.76.6.1909261449230.15067@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/3] Makefile: use $(ALL_CFLAGS) in $(HCO) target","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-09-26T17:38:01Z","receivedAt":"2019-09-26T17:38:06Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Dscho,\n\nOn Thu, Sep 26, 2019 at 02:49:55PM +0200, Johannes Schindelin wrote:\n> Hi Denton,\n> \n> On Mon, 23 Sep 2019, Denton Liu wrote:\n> \n> > On platforms that can run `make hdr-check` but require custom flags,\n> > this target was failing because none of them were being passed to the\n> > compiler. For example, on MacOS, the NO_OPENSSL flag was being set but\n> > it was not being passed into compiler so the check was failing.\n> >\n> > Pass $(ALL_CFLAGS) into the compiler for the $(HCO) target so that these\n> > custom flags can be used.\n> >\n> > Signed-off-by: Denton Liu \n> > ---\n> > Makefile | 2 +-\n> > 1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/Makefile b/Makefile\n> > index f879697ea3..d8df4e316b 100644\n> > --- a/Makefile\n> > +++ b/Makefile\n> > @@ -2773,7 +2773,7 @@ CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H)))\n> > HCO = $(patsubst %.h,%.hco,$(CHK_HDRS))\n> >\n> > $(HCO): %.hco: %.h FORCE\n> > -\t$(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $<\n> > +\t$(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $(ALL_CFLAGS) $<\n> \n> Isn't this essentially identical to what I sent in\n> https://public-inbox.org/git/pull.129.git.gitgitgadget@gmail.com/\n> ?\n\nWhoops, didn't notice this. I heard there's some quote about great minds\nor something ;).\n\nAnyway, this patch is superceded by v2 4/4 which should emulate the\ncompilation process even better, as suggested by Peff. The only problem\nwith that patch is that it splashes temporary *.hcc files everywhere but\nI think that if it's only a developer-run target, it should be fine.\n\nThanks,\n\nDenton\n\n> \n> Ciao,\n> Dscho\n> \n> >\n> > .PHONY: hdr-check $(HCO)\n> > hdr-check: $(HCO)\n> > --\n> > 2.23.0.565.g1cc52d20df\n> >\n> >\n"},{"id":"383022","messageId":"nycvar.QRO.7.76.6.1909262144060.15067@tvgsbejvaqbjf.bet","threadId":"51904","inReplyTo":"20190926173801.GA42456@generichostname","subject":"Re: [PATCH 1/3] Makefile: use $(ALL_CFLAGS) in $(HCO) target","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-09-26T19:47:43Z","receivedAt":"2019-09-26T19:48:04Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 26 Sep 2019, Denton Liu wrote:\n\n> On Thu, Sep 26, 2019 at 02:49:55PM +0200, Johannes Schindelin wrote:\n> >\n> > On Mon, 23 Sep 2019, Denton Liu wrote:\n> >\n> > > On platforms that can run `make hdr-check` but require custom flags,\n> > > this target was failing because none of them were being passed to the\n> > > compiler. For example, on MacOS, the NO_OPENSSL flag was being set but\n> > > it was not being passed into compiler so the check was failing.\n> > >\n> > > Pass $(ALL_CFLAGS) into the compiler for the $(HCO) target so that these\n> > > custom flags can be used.\n> > >\n> > > Signed-off-by: Denton Liu \n> > > ---\n> > > Makefile | 2 +-\n> > > 1 file changed, 1 insertion(+), 1 deletion(-)\n> > >\n> > > diff --git a/Makefile b/Makefile\n> > > index f879697ea3..d8df4e316b 100644\n> > > --- a/Makefile\n> > > +++ b/Makefile\n> > > @@ -2773,7 +2773,7 @@ CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H)))\n> > > HCO = $(patsubst %.h,%.hco,$(CHK_HDRS))\n> > >\n> > > $(HCO): %.hco: %.h FORCE\n> > > -\t$(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $<\n> > > +\t$(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $(ALL_CFLAGS) $<\n> >\n> > Isn't this essentially identical to what I sent in\n> > https://public-inbox.org/git/pull.129.git.gitgitgadget@gmail.com/\n> > ?\n>\n> Whoops, didn't notice this. I heard there's some quote about great minds\n> or something ;).\n\nHahaha! ;-)\n\n> Anyway, this patch is superceded by v2 4/4 which should emulate the\n> compilation process even better, as suggested by Peff. The only\n> problem with that patch is that it splashes temporary *.hcc files\n> everywhere but I think that if it's only a developer-run target, it\n> should be fine.\n\nI agree that Peff's improvement is nice.\n\nThe only problem is that my patch made it into `next` already, according\nto https://github.com/gitgitgadget/git/pull/129 (look for the labels on\nthe right side, or for the comments on the bottom), and\nhttps://github.com/gitster/git/commit/a3f332f4fb10 agrees (look for\nnames of the branches containing that commit, under the commit message).\n\nSo we should explicitly as for my patch to be backed out from `next` and\nbe stopped from advancing to `master`.\n\nJunio, would you kindly do that? According to the PR, you gave this\nbranch the name `js/honor-cflags-in-hdr-check`.\n\nThanks,\nDscho\n"},{"id":"383084","messageId":"xmqqr241j9so.fsf@gitster-ct.c.googlers.com","threadId":"51904","inReplyTo":"nycvar.QRO.7.76.6.1909262144060.15067@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/3] Makefile: use $(ALL_CFLAGS) in $(HCO) target","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-28T04:59:19Z","receivedAt":"2019-09-28T04:59:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> The only problem is that my patch made it into `next` already, according\n> to https://github.com/gitgitgadget/git/pull/129 (look for the labels on\n> the right side, or for the comments on the bottom), and\n> https://github.com/gitster/git/commit/a3f332f4fb10 agrees (look for\n> names of the branches containing that commit, under the commit message).\n>\n> So we should explicitly as for my patch to be backed out from `next` and\n> be stopped from advancing to `master`.\n>\n> Junio, would you kindly do that? According to the PR, you gave this\n> branch the name `js/honor-cflags-in-hdr-check`.\n\nOK, so the topic should be dropped and replaced by the Denton's\nseries. Thanks, will do.\n"},{"id":"383278","messageId":"20191002154131.GB6116@sigill.intra.peff.net","threadId":"51904","inReplyTo":"14def72319521d7380fb6a8ec570d014c0f5361b.1569398897.git.liu.denton@gmail.com","subject":"Re: [PATCH v2 4/4] Makefile: emulate compile in $(HCO) target better","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-02T15:41:31Z","receivedAt":"2019-10-02T15:41:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 25, 2019 at 01:21:01AM -0700, Denton Liu wrote:\n\n> Fix these problems by emulating the compile process better, including\n> test compiling dummy *.hcc C sources generated from the *.h files and\n> passing $(ALL_CFLAGS) into the compiler for the $(HCO) target so that\n> these custom flags can be used.\n> \n> Helped-by: Jeff King \n> Signed-off-by: Denton Liu \n> ---\n> \n> Peff, thanks for the suggestion! I modified it a little bit so that we\n> wouldn't have to keep regenerating the *.hcc files unnecessarily.\n\nWhat you have here looks good. I doubt it matters too much compared to\nthe cost of the compiler, but having them in their own rule makes it\neasier to follow, I think.\n\n> I also considered piping into the compiler's stdin directly which I know\n> works for GCC (and _probably_ Clang) but I opted against it because I'm\n> not sure it's portable for other compilers. Maybe it's alright for this\n> to be less portable since it's a developer target?\n\nYeah, I'd worry slightly about portability. But it would be nice to\navoid generating more cruft. And I agree that most people running this\nwould be developers. Maybe it would make sense to float a separate patch\non top (that would make it easy to revert if somebody runs into a\nproblem).\n\nThanks for working on this (and the whole series looks good, including\nthe commit message changes you made regarding the bitmap code).\n\n-Peff\n"}]}