[PATCH 0/1] Avoid calling find in the Makefile, if possible

[PATCH 0/1] Avoid calling find in the Makefile, if possible

[Johannes Schindelin via GitGitGadget]

I noticed this quite a bit of time ago, but did not get a chance to look
into it in detail: all of a sudden, make started really slowly over here.

The culprit turned out to be a find call, which was in the Makefile for
ages, so I was puzzled why it only caused problems recently.

After some digging, I found out that the hdr-check target is the culprit:
before it was introduced, $(LIB_H) did not need to be expanded, and as a
consequence the find call was not executed. Once hdr-check made it into 
master, though, $(LIB_H) must be expanded to define that rule.

Since I have tons of worktrees as subdirectories of my principal Git clone,
and since I also have a 3rdparty/ directory with tons of repositories I use
for various testing/contributing purposes, this find is quite a little slow
over here.

So here is my suggested fix. It is based on similar code we already had in
the Makefile, obviously also intended to avoid an expensive find invocation.

Johannes Schindelin (1):
  Makefile: use `git ls-files` to list header files, if possible

 Makefile | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)


base-commit: c65a2884eae159bad540135479bc8afe20ff62d1
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-130%2Fdscho%2Favoid-find-in-makefile-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-130/dscho/avoid-find-in-makefile-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/130
-- 
gitgitgadget

[PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In d85b0dff72 (Makefile: use `find` to determine static header
dependencies, 2014-08-25), we switched from a static list of header
files to a dynamically-generated one, asking `find` to enumerate them.

Back in those days, we did not use `$(LIB_H)` by default, and many a
`make` implementation seems smart enough not to run that `find` command
in that case, so it was deemed okay to run `find` for special targets
requiring this macro.

However, as of ebb7baf02f (Makefile: add a hdr-check target,
2018-09-19), $(LIB_H) is part of a global rule and therefore must be
expanded. Meaning: this `find` command has to be run upon every
`make` invocation. In the presence of many a worktree, this can tax the
developers' patience quite a bit.

Even in the absence of worktrees or other untracked files and
directories, the cost of I/O to generate that list of header files is
simply a lot larger than a simple `git ls-files` call.

Therefore, just like in 335339758c (Makefile: ask "ls-files" to list
source files if available, 2011-10-18), we now prefer to use `git
ls-files` to enumerate the header files to enumerating them via `find`,
falling back to the latter if the former failed (which would be the case
e.g. in a worktree that was extracted from a source .tar file rather
than from a clone of Git's sources).

This has one notable consequence: we no longer include `command-list.h`
in `LIB_H`, as it is a generated file, not a tracked one.

Likewise, we no longer include not-yet-tracked header files in `LIB_H`.

Given the speed improvements, these consequences seem a comparably small
price to pay.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Makefile | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index c5240942f2..2644324915 100644
--- a/Makefile
+++ b/Makefile
@@ -841,7 +841,8 @@ VCSSVN_LIB = vcs-svn/lib.a
 
 GENERATED_H += command-list.h
 
-LIB_H = $(shell $(FIND) . \
+LIB_H = $(shell git ls-files '*.h' ':!t/' ':!Documentation/' 2>/dev/null || \
+	$(FIND) . \
 	-name .git -prune -o \
 	-name t -prune -o \
 	-name Documentation -prune -o \
@@ -2363,7 +2364,7 @@ else
 # should _not_ be included here, since they are necessary even when
 # building an object for the first time.
 
-$(OBJECTS): $(LIB_H)
+$(OBJECTS): $(LIB_H) $(GENERATED_H)
 endif
 
 exec-cmd.sp exec-cmd.s exec-cmd.o: GIT-PREFIX
-- 
gitgitgadget

Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible

[Jeff King]

On Fri, Mar 01, 2019 at 11:57:46AM -0800, Johannes Schindelin via GitGitGadget wrote:

> In d85b0dff72 (Makefile: use `find` to determine static header
> dependencies, 2014-08-25), we switched from a static list of header
> files to a dynamically-generated one, asking `find` to enumerate them.
> 
> Back in those days, we did not use `$(LIB_H)` by default, and many a
> `make` implementation seems smart enough not to run that `find` command
> in that case, so it was deemed okay to run `find` for special targets
> requiring this macro.
> 
> However, as of ebb7baf02f (Makefile: add a hdr-check target,
> 2018-09-19), $(LIB_H) is part of a global rule and therefore must be
> expanded. Meaning: this `find` command has to be run upon every
> `make` invocation. In the presence of many a worktree, this can tax the
> developers' patience quite a bit.

I'm confused about this part. We don't run hdr-check by default. Why
would make need the value of $(LIB_H)? Yet empirically it does run find.

Worse, it seems to actually run it _three times_. Once for the $(HCO)
target, once for the .PHONY, and once for the hdr-check target. I think
the .PHONY one is unavoidable (it doesn't know which names we might
otherwise be building should be marked), but the other two seem like
bugs in make (or at least pessimisations).

It makes me wonder if we'd be better off pushing hdr-check into a
separate script. It doesn't actually use make's dependency tree in any
meaningful way. And then regular invocations wouldn't even have to pay
the `ls-files` price.

If we are going to keep it in the Makefile, we should probably use a
":=" rule to avoid running it three times.

> Even in the absence of worktrees or other untracked files and
> directories, the cost of I/O to generate that list of header files is
> simply a lot larger than a simple `git ls-files` call.
> 
> Therefore, just like in 335339758c (Makefile: ask "ls-files" to list
> source files if available, 2011-10-18), we now prefer to use `git
> ls-files` to enumerate the header files to enumerating them via `find`,
> falling back to the latter if the former failed (which would be the case
> e.g. in a worktree that was extracted from a source .tar file rather
> than from a clone of Git's sources).

That seems reasonable (regardless of whether it is in a script or in the
Makefile). Another option is to use -maxdepth, but that involves
guessing how deep people might actually put header files.

> This has one notable consequence: we no longer include `command-list.h`
> in `LIB_H`, as it is a generated file, not a tracked one.

We should be able to add back $(GENERATED_H) as appropriate. I see you
did it for the non-computed-dependencies case. Couldn't we do the same
for $(LOCALIZED_C) and $(CHK_HDRS)?

> Likewise, we no longer include not-yet-tracked header files in `LIB_H`.

I think that's probably OK.

-Peff

Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible

[Jeff King]

On Fri, Mar 01, 2019 at 04:36:19PM -0500, Jeff King wrote:

> > However, as of ebb7baf02f (Makefile: add a hdr-check target,
> > 2018-09-19), $(LIB_H) is part of a global rule and therefore must be
> > expanded. Meaning: this `find` command has to be run upon every
> > `make` invocation. In the presence of many a worktree, this can tax the
> > developers' patience quite a bit.
> 
> I'm confused about this part. We don't run hdr-check by default. Why
> would make need the value of $(LIB_H)? Yet empirically it does run find.
> 
> Worse, it seems to actually run it _three times_. Once for the $(HCO)
> target, once for the .PHONY, and once for the hdr-check target. I think
> the .PHONY one is unavoidable (it doesn't know which names we might
> otherwise be building should be marked), but the other two seem like
> bugs in make (or at least pessimisations).
> 
> It makes me wonder if we'd be better off pushing hdr-check into a
> separate script. It doesn't actually use make's dependency tree in any
> meaningful way. And then regular invocations wouldn't even have to pay
> the `ls-files` price.
> 
> If we are going to keep it in the Makefile, we should probably use a
> ":=" rule to avoid running it three times.

Looping in Ramsay as the author of hdr-check. I think we could even do
this without using a separate script, like so:

diff --git a/Makefile b/Makefile
index c5240942f2..a65c4599e9 100644
--- a/Makefile
+++ b/Makefile
@@ -1852,7 +1852,6 @@ ifndef V
 	QUIET_MSGFMT   = @echo '   ' MSGFMT $@;
 	QUIET_GCOV     = @echo '   ' GCOV $@;
 	QUIET_SP       = @echo '   ' SP $<;
-	QUIET_HDR      = @echo '   ' HDR $<;
 	QUIET_RC       = @echo '   ' RC $@;
 	QUIET_SUBDIR0  = +@subdir=
 	QUIET_SUBDIR1  = ;$(NO_SUBDIR) echo '   ' SUBDIR $$subdir; \
@@ -2740,11 +2739,13 @@ EXCEPT_HDRS := $(GEN_HDRS) compat% xdiff%
 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 $<
-
-.PHONY: hdr-check $(HCO)
-hdr-check: $(HCO)
+.PHONY: hdr-check
+hdr-check:
+	@for i in $(LIB_H); do \
+		echo "    HDR $$i"; \
+		$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $$i || \
+			exit 1; \
+	done
 
 .PHONY: style
 style:

The one thing we do lose, though, is make's parallelization. It would
probably be possible to actually shove this into a sub-make which
defined the hdr-check rules, but I don't know how complicated that would
become.

> > This has one notable consequence: we no longer include `command-list.h`
> > in `LIB_H`, as it is a generated file, not a tracked one.
> 
> We should be able to add back $(GENERATED_H) as appropriate. I see you
> did it for the non-computed-dependencies case. Couldn't we do the same
> for $(LOCALIZED_C) and $(CHK_HDRS)?

Answering my own question somewhat: $(CHK_HDRS) explicitly ignores the
generated headers, so we wouldn't want to bother re-adding it there
anyway. Possibly $(LOCALIZED_C) would care, though. The generated file
does have translatable strings in it.

-Peff

Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible

[Jeff King]

On Fri, Mar 01, 2019 at 04:54:15PM -0500, Jeff King wrote:

> The one thing we do lose, though, is make's parallelization. It would
> probably be possible to actually shove this into a sub-make which
> defined the hdr-check rules, but I don't know how complicated that would
> become.

This seems to work, though it's kind of horrid.

It costs at least one extra process to run "make hdr-check", and
probably more for things like $(GIT_VERSION) that the Makefile include
likely triggers. But when you're not running hdr-check (which is the
norm), it's zero-cost.

-Peff

diff --git a/Makefile b/Makefile
index c5240942f2..ab6ecf450e 100644
--- a/Makefile
+++ b/Makefile
@@ -2735,16 +2735,8 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
 .PHONY: sparse $(SP_OBJ)
 sparse: $(SP_OBJ)
 
-GEN_HDRS := command-list.h unicode-width.h
-EXCEPT_HDRS := $(GEN_HDRS) compat% xdiff%
-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 $<
-
-.PHONY: hdr-check $(HCO)
-hdr-check: $(HCO)
+hdr-check:
+	$(MAKE) -f hdr-check.mak hdr-check
 
 .PHONY: style
 style:
diff --git a/hdr-check.mak b/hdr-check.mak
new file mode 100644
index 0000000000..b8924afa90
--- /dev/null
+++ b/hdr-check.mak
@@ -0,0 +1,12 @@
+include Makefile
+
+GEN_HDRS := command-list.h unicode-width.h
+EXCEPT_HDRS := $(GEN_HDRS) compat% xdiff%
+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 $<
+
+.PHONY: hdr-check $(HCO)
+hdr-check: $(HCO)

Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible

[Johannes Schindelin]

Hi Peff,

On Fri, 1 Mar 2019, Jeff King wrote:

> On Fri, Mar 01, 2019 at 04:54:15PM -0500, Jeff King wrote:
> 
> > The one thing we do lose, though, is make's parallelization. It would
> > probably be possible to actually shove this into a sub-make which
> > defined the hdr-check rules, but I don't know how complicated that would
> > become.
> 
> This seems to work, though it's kind of horrid.
> 
> It costs at least one extra process to run "make hdr-check", and
> probably more for things like $(GIT_VERSION) that the Makefile include
> likely triggers. But when you're not running hdr-check (which is the
> norm), it's zero-cost.

If we want to go that route (and I am not saying we should), we could
easily just add another target (say, `check-headers`) that requires a list
of headers to check to be passed in via a Makefile variable that is
defined via the command-line.

Ciao,
Dscho

> 
> -Peff
> 
> diff --git a/Makefile b/Makefile
> index c5240942f2..ab6ecf450e 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2735,16 +2735,8 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
>  .PHONY: sparse $(SP_OBJ)
>  sparse: $(SP_OBJ)
>  
> -GEN_HDRS := command-list.h unicode-width.h
> -EXCEPT_HDRS := $(GEN_HDRS) compat% xdiff%
> -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 $<
> -
> -.PHONY: hdr-check $(HCO)
> -hdr-check: $(HCO)
> +hdr-check:
> +	$(MAKE) -f hdr-check.mak hdr-check
>  
>  .PHONY: style
>  style:
> diff --git a/hdr-check.mak b/hdr-check.mak
> new file mode 100644
> index 0000000000..b8924afa90
> --- /dev/null
> +++ b/hdr-check.mak
> @@ -0,0 +1,12 @@
> +include Makefile
> +
> +GEN_HDRS := command-list.h unicode-width.h
> +EXCEPT_HDRS := $(GEN_HDRS) compat% xdiff%
> +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 $<
> +
> +.PHONY: hdr-check $(HCO)
> +hdr-check: $(HCO)
> 

Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible

[Johannes Schindelin]

Hi Peff,

On Fri, 1 Mar 2019, Jeff King wrote:

> On Fri, Mar 01, 2019 at 04:36:19PM -0500, Jeff King wrote:
> 
> > > This has one notable consequence: we no longer include
> > > `command-list.h` in `LIB_H`, as it is a generated file, not a
> > > tracked one.
> > 
> > We should be able to add back $(GENERATED_H) as appropriate. I see you
> > did it for the non-computed-dependencies case. Couldn't we do the same
> > for $(LOCALIZED_C) and $(CHK_HDRS)?
> 
> Answering my own question somewhat: $(CHK_HDRS) explicitly ignores the
> generated headers, so we wouldn't want to bother re-adding it there
> anyway. Possibly $(LOCALIZED_C) would care, though. The generated file
> does have translatable strings in it.

Yes, and it is defined thusly:

	LOCALIZED_C = $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)

So it does include the generated headers explictly anyway, with or without
my patch.

Ciao,
Dscho

Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible

[Johannes Schindelin]

Hi Peff,

On Fri, 1 Mar 2019, Jeff King wrote:

> On Fri, Mar 01, 2019 at 11:57:46AM -0800, Johannes Schindelin via GitGitGadget wrote:
> 
> > In d85b0dff72 (Makefile: use `find` to determine static header
> > dependencies, 2014-08-25), we switched from a static list of header
> > files to a dynamically-generated one, asking `find` to enumerate them.
> > 
> > Back in those days, we did not use `$(LIB_H)` by default, and many a
> > `make` implementation seems smart enough not to run that `find` command
> > in that case, so it was deemed okay to run `find` for special targets
> > requiring this macro.
> > 
> > However, as of ebb7baf02f (Makefile: add a hdr-check target,
> > 2018-09-19), $(LIB_H) is part of a global rule and therefore must be
> > expanded. Meaning: this `find` command has to be run upon every
> > `make` invocation. In the presence of many a worktree, this can tax the
> > developers' patience quite a bit.
> 
> I'm confused about this part. We don't run hdr-check by default. Why
> would make need the value of $(LIB_H)? Yet empirically it does run find.
> 
> Worse, it seems to actually run it _three times_. Once for the $(HCO)
> target, once for the .PHONY, and once for the hdr-check target. I think
> the .PHONY one is unavoidable (it doesn't know which names we might
> otherwise be building should be marked), but the other two seem like
> bugs in make (or at least pessimisations).
> 
> It makes me wonder if we'd be better off pushing hdr-check into a
> separate script. It doesn't actually use make's dependency tree in any
> meaningful way. And then regular invocations wouldn't even have to pay
> the `ls-files` price.
> 
> If we are going to keep it in the Makefile, we should probably use a
> ":=" rule to avoid running it three times.

Good point.

> > Even in the absence of worktrees or other untracked files and
> > directories, the cost of I/O to generate that list of header files is
> > simply a lot larger than a simple `git ls-files` call.
> > 
> > Therefore, just like in 335339758c (Makefile: ask "ls-files" to list
> > source files if available, 2011-10-18), we now prefer to use `git
> > ls-files` to enumerate the header files to enumerating them via `find`,
> > falling back to the latter if the former failed (which would be the case
> > e.g. in a worktree that was extracted from a source .tar file rather
> > than from a clone of Git's sources).
> 
> That seems reasonable (regardless of whether it is in a script or in the
> Makefile). Another option is to use -maxdepth, but that involves
> guessing how deep people might actually put header files.

It would also fail to work when somebody clones an unrelated repository
that contains header files in the top-level directory into the Git
worktree. I know somebody like that: me.

> > This has one notable consequence: we no longer include `command-list.h`
> > in `LIB_H`, as it is a generated file, not a tracked one.
> 
> We should be able to add back $(GENERATED_H) as appropriate. I see you
> did it for the non-computed-dependencies case. Couldn't we do the same
> for $(LOCALIZED_C) and $(CHK_HDRS)?

As you figured out, CHK_HDRS *specifically* excludes the generated
headers, and as I pointed out: LOCALIZED_C includes $(GENERATED_H)
explicitly.

So I think we're good on that front.

> > Likewise, we no longer include not-yet-tracked header files in `LIB_H`.
> 
> I think that's probably OK.

It does potentially make developing new patches more challenging. I, for
one, do not immediately stage every new file I add, especially not header
files. But then, I rarely recompile after only editing such a new header
file (I would more likely modify also the source file that includes that
header).

So while I think this issue could potentially cause problems only *very*
rarely, I think that it would be a really terribly confusing thing if it
happened.

But I probably worry too much about it?

Ciao,
Dscho

Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible

[Jeff King]

On Sat, Mar 02, 2019 at 08:54:55PM +0100, Johannes Schindelin wrote:

> On Fri, 1 Mar 2019, Jeff King wrote:
> 
> > On Fri, Mar 01, 2019 at 04:54:15PM -0500, Jeff King wrote:
> > 
> > > The one thing we do lose, though, is make's parallelization. It would
> > > probably be possible to actually shove this into a sub-make which
> > > defined the hdr-check rules, but I don't know how complicated that would
> > > become.
> > 
> > This seems to work, though it's kind of horrid.
> > 
> > It costs at least one extra process to run "make hdr-check", and
> > probably more for things like $(GIT_VERSION) that the Makefile include
> > likely triggers. But when you're not running hdr-check (which is the
> > norm), it's zero-cost.
> 
> If we want to go that route (and I am not saying we should), we could
> easily just add another target (say, `check-headers`) that requires a list
> of headers to check to be passed in via a Makefile variable that is
> defined via the command-line.

Yeah, that was actually going to be my initial attempt before I tried
the "include" shortcut. :) I'd worry slightly about things like
command-line limits, but maybe our list isn't big enough to justify
that.

-Peff

Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible

[Jeff King]

On Sat, Mar 02, 2019 at 08:57:57PM +0100, Johannes Schindelin wrote:

> Hi Peff,
> 
> On Fri, 1 Mar 2019, Jeff King wrote:
> 
> > On Fri, Mar 01, 2019 at 04:36:19PM -0500, Jeff King wrote:
> > 
> > > > This has one notable consequence: we no longer include
> > > > `command-list.h` in `LIB_H`, as it is a generated file, not a
> > > > tracked one.
> > > 
> > > We should be able to add back $(GENERATED_H) as appropriate. I see you
> > > did it for the non-computed-dependencies case. Couldn't we do the same
> > > for $(LOCALIZED_C) and $(CHK_HDRS)?
> > 
> > Answering my own question somewhat: $(CHK_HDRS) explicitly ignores the
> > generated headers, so we wouldn't want to bother re-adding it there
> > anyway. Possibly $(LOCALIZED_C) would care, though. The generated file
> > does have translatable strings in it.
> 
> Yes, and it is defined thusly:
> 
> 	LOCALIZED_C = $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)
> 
> So it does include the generated headers explictly anyway, with or without
> my patch.

Oops, I should have read the source more carefully before responding.

Is it worth mentioning in the commit message? I.e., something like:

   This has one notable consequence: we no longer include
   `command-list.h` in `LIB_H`, as it is a generated file, not a tracked
   one, but that is easily worked around. Of the three sites that use
   `LIB_H`, two (`LOCALIZED_C` and `CHK_HDRS`) already handle generated
   headers separately. In the third, the computed-dependency fallback,
   we can just add in a reference to $(GENERATED_H).

-Peff

Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible

[Jeff King]

On Sat, Mar 02, 2019 at 09:05:24PM +0100, Johannes Schindelin wrote:

> > That seems reasonable (regardless of whether it is in a script or in the
> > Makefile). Another option is to use -maxdepth, but that involves
> > guessing how deep people might actually put header files.
> 
> It would also fail to work when somebody clones an unrelated repository
> that contains header files in the top-level directory into the Git
> worktree. I know somebody like that: me.

Good point.

By the way, "make hdr-check" already fails for me on master, as I do not have
libgcrypt installed, and it unconditionally checks sha256/gcrypt.h.

I wonder if this actually does point to hdr-check needing to be smarter
about checking the headers that are actually used in compilation on your
platform. Or if that file should just be added to the set of excluded
headers.

> > We should be able to add back $(GENERATED_H) as appropriate. I see you
> > did it for the non-computed-dependencies case. Couldn't we do the same
> > for $(LOCALIZED_C) and $(CHK_HDRS)?
> 
> As you figured out, CHK_HDRS *specifically* excludes the generated
> headers, and as I pointed out: LOCALIZED_C includes $(GENERATED_H)
> explicitly.
> 
> So I think we're good on that front.

Yeah, agreed.

> > > Likewise, we no longer include not-yet-tracked header files in `LIB_H`.
> > 
> > I think that's probably OK.
> 
> It does potentially make developing new patches more challenging. I, for
> one, do not immediately stage every new file I add, especially not header
> files. But then, I rarely recompile after only editing such a new header
> file (I would more likely modify also the source file that includes that
> header).
> 
> So while I think this issue could potentially cause problems only *very*
> rarely, I think that it would be a really terribly confusing thing if it
> happened.
> 
> But I probably worry too much about it?

I think it's not ideal, but it's probably an acceptable tradeoff. The
LIB_H list is used for three things:

  - hdr-check, which I'd think would generally be run periodically on a
    full tree to catch any new header breakages. But I dunno, maybe
    people want to run it as soon as they've written new code.

  - the .po generation, which generally is a separate workflow from
    writing new header files

  - the header-dependency fallback code. This is definitely the place
    where somebody just adding a new header file and running "make"
    might get bitten. But it only kicks in for ancient, crappy compilers
    that don't do dependency computation; so I think most developers
    would not be using it. (this is your cue to explain to me how
    some workflow involving MSVC does not compute dependencies, and
    I'm unknowingly dismissing a large portion developers ;) ).

-Peff

Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible

[Ramsay Jones]

On 03/03/2019 17:19, Jeff King wrote:
> On Sat, Mar 02, 2019 at 09:05:24PM +0100, Johannes Schindelin wrote:
> 
>>> That seems reasonable (regardless of whether it is in a script or in the
>>> Makefile). Another option is to use -maxdepth, but that involves
>>> guessing how deep people might actually put header files.
>>
>> It would also fail to work when somebody clones an unrelated repository
>> that contains header files in the top-level directory into the Git
>> worktree. I know somebody like that: me.
> 
> Good point.

[Sorry for the late reply - I have been AWOL this weekend and
I am only just catching up with email! :-D ]

So, tl;dr: soon, I will be submitting a patch to remove the
'hdr-check' target completely, for now anyway.

> By the way, "make hdr-check" already fails for me on master, as I do not have
> libgcrypt installed, and it unconditionally checks sha256/gcrypt.h.

Yep, for me too. There is a similar problem if you build with
NO_CURL and don't have the 'curl/curl.h' header file, etc ...

The first version of the 'hdr-check' target re-introduced a static
list of header files, but I didn't think people would appreciate
having to maintain the list manually, so I gave up on that!

The next version was essentially the same patch that started this
thread from Johannes (ie. the 'git ls-files' patch), given that
the 'tags' targets had found it necessary. However, when I did some
'informal' timing tests (ie 5x 'time make ...' and average), this
did not make any noticeable difference for me (_even_ on cygwin!). ;-)

Of course, I had already made the mistake of trying to re-use
something that was 'handy' (ie. LIB_H) and already there.
However, LIB_H wasn't quite what I wanted - I needed a sub-set
of it.

So, I already have a 'hdr-check-fixup' branch (I think I have
already mentioned it), in which the first commit looks like:

  diff --git a/Makefile b/Makefile
  index c5240942f2..3401d1b9fb 100644
  --- a/Makefile
  +++ b/Makefile
  @@ -2735,9 +2735,10 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
   .PHONY: sparse $(SP_OBJ)
   sparse: $(SP_OBJ)
   
  +HC_HDRS := $(wildcard *.h negotiator/*.h block-sha1/*.h ppc/*.h ewah/*.h \
  +       sha1dc/*.h refs/*.h vcs-svn/*.h)
   GEN_HDRS := command-list.h unicode-width.h
  -EXCEPT_HDRS := $(GEN_HDRS) compat% xdiff%
  -CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H)))
  +CHK_HDRS = $(filter-out $(GEN_HDRS),$(HC_HDRS))
   HCO = $(patsubst %.h,%.hco,$(CHK_HDRS))
   
   $(HCO): %.hco: %.h FORCE
  

... which drops the use of LIB_H entirely.

Now, I have timed this and, for me, it no faster ... (I suspect
that it would be faster for Johannes, but it would still cause
a problem if you have 'top-level header files from some other
repo ...').

So, if we really need to solve the problem, allowing for some
unrelated headers in your worktree, then we can only use a
static list, or a 'git ls-files' approach.

Anyway, for now, since I seem to be the only person using this
target, I think we should just remove it while I think again.
(I can put it in my config.mak, so there will be no loss for me).

ATB,
Ramsay Jones

Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible

[Johannes Schindelin]

Hi Peff,

On Sun, 3 Mar 2019, Jeff King wrote:

> By the way, "make hdr-check" already fails for me on master, as I do not
> have libgcrypt installed, and it unconditionally checks sha256/gcrypt.h.

Oh? I thought it'd fail only on Windows, so I have not submitted
https://github.com/dscho/git/commit/fix-hdr-check yet:

-- snipsnap --
From c13d9985284d4b452db0d95b6949e02c533db634 Mon Sep 17 00:00:00 2001
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Sat, 23 Feb 2019 20:38:40 +0100
Subject: [PATCH] hdr-check: make it work on Windows

On Windows, we define a specific set of pre-processor macros, among
other reasons: to avoid including syslog.h (which is not available on
Windows).

The hdr-check target did not use those definitions, resulting in a
failure to include said syslog.h.

To fix that, let's let the hdr-check target make use of ALL_CFLAGS,
which does include the pre-processor macros that would e.g. skip the
conditional block including syslog.h.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index c5240942f2..b9f00f0ea0 100644
--- a/Makefile
+++ b/Makefile
@@ -2741,7 +2741,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) $(ALL_CFLAGS) -include git-compat-util.h -I. -o /dev/null -c -xc $<
 
 .PHONY: hdr-check $(HCO)
 hdr-check: $(HCO)
-- 
2.20.1.windows.1.1576.g37ac248661.dirty

Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible

[Johannes Schindelin]

Hi Ramsay,

On Sun, 3 Mar 2019, Ramsay Jones wrote:

> On 03/03/2019 17:19, Jeff King wrote:
> > On Sat, Mar 02, 2019 at 09:05:24PM +0100, Johannes Schindelin wrote:
> > 
> >>> That seems reasonable (regardless of whether it is in a script or in the
> >>> Makefile). Another option is to use -maxdepth, but that involves
> >>> guessing how deep people might actually put header files.
> >>
> >> It would also fail to work when somebody clones an unrelated repository
> >> that contains header files in the top-level directory into the Git
> >> worktree. I know somebody like that: me.
> > 
> > Good point.
> 
> [Sorry for the late reply - I have been AWOL this weekend and
> I am only just catching up with email! :-D ]

You should never feel the need to apologize for spending a weekend away
from the keyboard (AKA "having a life").

> So, tl;dr: soon, I will be submitting a patch to remove the
> 'hdr-check' target completely, for now anyway.

You mentioned later that you might be the only person using that target,
and if that were so, I would agree.

However, I started looking into adding `hdr-check` to the Azure Pipeline
we have (which would make a *ton* of sense, if you ask me), along with the
`sparse` thing we talked about.

So I am a bit opposed to removing that target.

> > By the way, "make hdr-check" already fails for me on master, as I do
> > not have libgcrypt installed, and it unconditionally checks
> > sha256/gcrypt.h.
> 
> Yep, for me too. There is a similar problem if you build with
> NO_CURL and don't have the 'curl/curl.h' header file, etc ...

I think all it needs to do is to pass `$(ALL_CFLAGS)` to gcc (although we
might need to add something like `#if defined(SHA256_GCRYPT) ... #endif`
guards to `sha256/gcrypt.h` to make it work in Peff's case).

But given that this target really is meant to catch all kinds of errors,
I'd be in favor of declaring that target requiring things like libgcrypt's
header files to be installed. We can easily make that happen in our CI
builds.

> The first version of the 'hdr-check' target re-introduced a static list
> of header files, but I didn't think people would appreciate having to
> maintain the list manually, so I gave up on that!
> 
> The next version was essentially the same patch that started this thread
> from Johannes (ie. the 'git ls-files' patch), given that the 'tags'
> targets had found it necessary. However, when I did some 'informal'
> timing tests (ie 5x 'time make ...' and average), this did not make any
> noticeable difference for me (_even_ on cygwin!). ;-)

My complaint is not about the speed in a regular clone, but about *my*
special clone, where I have some 50+ worktrees (that I really like to keep
in the same directory, thankyouverymuch, I ignore them via
`.git/info/exclude`, and I really like to have them all tucked away neatly
under the `/usr/src/git/` directory) and also some 30+ cloned repositories
in `3rdparty/` for random testing (including a bare clone of the Linux
kernel, of course also ignored via `.git/info/exclude`).

In that scenario, my version of `find` takes ages, only to finally even
throw a segmentation fault (!).

> So, I already have a 'hdr-check-fixup' branch (I think I have
> already mentioned it), in which the first commit looks like:
> 
>   diff --git a/Makefile b/Makefile
>   index c5240942f2..3401d1b9fb 100644
>   --- a/Makefile
>   +++ b/Makefile
>   @@ -2735,9 +2735,10 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
>    .PHONY: sparse $(SP_OBJ)
>    sparse: $(SP_OBJ)
>    
>   +HC_HDRS := $(wildcard *.h negotiator/*.h block-sha1/*.h ppc/*.h ewah/*.h \
>   +       sha1dc/*.h refs/*.h vcs-svn/*.h)

But of course that would also share at least to a lesser extend the
shortcomings of a static list.

I still like the `ls-files` method the best.

>    GEN_HDRS := command-list.h unicode-width.h
>   -EXCEPT_HDRS := $(GEN_HDRS) compat% xdiff%
>   -CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H)))
>   +CHK_HDRS = $(filter-out $(GEN_HDRS),$(HC_HDRS))
>    HCO = $(patsubst %.h,%.hco,$(CHK_HDRS))
>    
>    $(HCO): %.hco: %.h FORCE
>   
> 
> ... which drops the use of LIB_H entirely.
> 
> Now, I have timed this and, for me, it no faster ... (I suspect
> that it would be faster for Johannes, but it would still cause
> a problem if you have 'top-level header files from some other
> repo ...').
> 
> So, if we really need to solve the problem, allowing for some
> unrelated headers in your worktree, then we can only use a
> static list, or a 'git ls-files' approach.

Or we can use `ls-files`, and fall back to your wildcard idea if
`ls-files` fails for some reason (typically because `.git/` is missing,
e.g. in case of an unpacked source .tar).

> Anyway, for now, since I seem to be the only person using this
> target, I think we should just remove it while I think again.
> (I can put it in my config.mak, so there will be no loss for me).

As I said above, I would rather keep it, with the `ls-files` and `:=`
fixup.

If others *really* feel strongly about lazy-evaluating `LIB_H`, then I
would *additionally* use Peff's sub-make hack. But only if others feel
strongly about it, because I, for one, certainly don't.

Ciao,
Dscho

[PATCH v2 0/1] Avoid calling find in the Makefile, if possible

[Johannes Schindelin via GitGitGadget]

I noticed this quite a bit of time ago, but did not get a chance to look
into it in detail: all of a sudden, make started really slowly over here.

The culprit turned out to be a find call, which was in the Makefile for
ages, so I was puzzled why it only caused problems recently.

After some digging, I found out that the hdr-check target is the culprit:
before it was introduced, $(LIB_H) did not need to be expanded, and as a
consequence the find call was not executed. Once hdr-check made it into 
master, though, $(LIB_H) must be expanded to define that rule.

Since I have tons of worktrees as subdirectories of my principal Git clone,
and since I also have a 3rdparty/ directory with tons of repositories I use
for various testing/contributing purposes, this find is quite a little slow
over here.

So here is my suggested fix. It is based on similar code we already had in
the Makefile, obviously also intended to avoid an expensive find invocation.

Changes since v1:

 * Since LIB_H's lazy evaluation kicks in all the time anyway, changed the = 
   to := to avoid evaluating it three times.
 * Clarified in the commit message that the existing sites using $(LIB_H) 
   are not affected by this change (or at least not affected as long as no
   untracked header files are included in .c files).

Johannes Schindelin (1):
  Makefile: use `git ls-files` to list header files, if possible

 Makefile | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)


base-commit: c65a2884eae159bad540135479bc8afe20ff62d1
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-130%2Fdscho%2Favoid-find-in-makefile-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-130/dscho/avoid-find-in-makefile-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/130

Range-diff vs v1:

 1:  0b5529406b ! 1:  cb253bd0cf Makefile: use `git ls-files` to list header files, if possible
     @@ -29,7 +29,11 @@
          than from a clone of Git's sources).
      
          This has one notable consequence: we no longer include `command-list.h`
     -    in `LIB_H`, as it is a generated file, not a tracked one.
     +    in `LIB_H`, as it is a generated file, not a tracked one, but that is
     +    easily worked around. Of the three sites that use `LIB_H`, two
     +    (`LOCALIZED_C` and `CHK_HDRS`) already handle generated headers
     +    separately. In the third, the computed-dependency fallback, we can just
     +    add in a reference to $(GENERATED_H).
      
          Likewise, we no longer include not-yet-tracked header files in `LIB_H`.
      
     @@ -46,7 +50,7 @@
       GENERATED_H += command-list.h
       
      -LIB_H = $(shell $(FIND) . \
     -+LIB_H = $(shell git ls-files '*.h' ':!t/' ':!Documentation/' 2>/dev/null || \
     ++LIB_H := $(shell git ls-files '*.h' ':!t/' ':!Documentation/' 2>/dev/null || \
      +	$(FIND) . \
       	-name .git -prune -o \
       	-name t -prune -o \

-- 
gitgitgadget

[PATCH v2 1/1] Makefile: use `git ls-files` to list header files, if possible

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In d85b0dff72 (Makefile: use `find` to determine static header
dependencies, 2014-08-25), we switched from a static list of header
files to a dynamically-generated one, asking `find` to enumerate them.

Back in those days, we did not use `$(LIB_H)` by default, and many a
`make` implementation seems smart enough not to run that `find` command
in that case, so it was deemed okay to run `find` for special targets
requiring this macro.

However, as of ebb7baf02f (Makefile: add a hdr-check target,
2018-09-19), $(LIB_H) is part of a global rule and therefore must be
expanded. Meaning: this `find` command has to be run upon every
`make` invocation. In the presence of many a worktree, this can tax the
developers' patience quite a bit.

Even in the absence of worktrees or other untracked files and
directories, the cost of I/O to generate that list of header files is
simply a lot larger than a simple `git ls-files` call.

Therefore, just like in 335339758c (Makefile: ask "ls-files" to list
source files if available, 2011-10-18), we now prefer to use `git
ls-files` to enumerate the header files to enumerating them via `find`,
falling back to the latter if the former failed (which would be the case
e.g. in a worktree that was extracted from a source .tar file rather
than from a clone of Git's sources).

This has one notable consequence: we no longer include `command-list.h`
in `LIB_H`, as it is a generated file, not a tracked one, but that is
easily worked around. Of the three sites that use `LIB_H`, two
(`LOCALIZED_C` and `CHK_HDRS`) already handle generated headers
separately. In the third, the computed-dependency fallback, we can just
add in a reference to $(GENERATED_H).

Likewise, we no longer include not-yet-tracked header files in `LIB_H`.

Given the speed improvements, these consequences seem a comparably small
price to pay.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Makefile | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index c5240942f2..0c4712cb48 100644
--- a/Makefile
+++ b/Makefile
@@ -841,7 +841,8 @@ VCSSVN_LIB = vcs-svn/lib.a
 
 GENERATED_H += command-list.h
 
-LIB_H = $(shell $(FIND) . \
+LIB_H := $(shell git ls-files '*.h' ':!t/' ':!Documentation/' 2>/dev/null || \
+	$(FIND) . \
 	-name .git -prune -o \
 	-name t -prune -o \
 	-name Documentation -prune -o \
@@ -2363,7 +2364,7 @@ else
 # should _not_ be included here, since they are necessary even when
 # building an object for the first time.
 
-$(OBJECTS): $(LIB_H)
+$(OBJECTS): $(LIB_H) $(GENERATED_H)
 endif
 
 exec-cmd.sp exec-cmd.s exec-cmd.o: GIT-PREFIX
-- 
gitgitgadget

Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible

[Ramsay Jones]

On 04/03/2019 12:38, Johannes Schindelin wrote:

>> So, tl;dr: soon, I will be submitting a patch to remove the
>> 'hdr-check' target completely, for now anyway.
> 
> You mentioned later that you might be the only person using that target,
> and if that were so, I would agree.

It is obviously difficult to judge such a thing, but I suspect
that the number is very low, if not exactly one. ;-)

> However, I started looking into adding `hdr-check` to the Azure Pipeline
> we have (which would make a *ton* of sense, if you ask me), along with the

Hmm, I am in two minds about this - I feel duty bound to warn you
that, if you go ahead with this idea, you will likely create a
rod for your own back! :-D

If the false positive rate can be kept reasonably low, then this
would be a good idea. However, some projects are just not a good
fit for this kind of thing (and Git may be one of them).

> `sparse` thing we talked about.
> 
> So I am a bit opposed to removing that target.

OK, I took the 'nuclear' route because this affects people (to a
greater or lesser degree) even when they don't know/care about
this target. If it is just (or mostly) me, then it didn't seem
worth causing issues for you (or anyone else working on GTW?).

>>> By the way, "make hdr-check" already fails for me on master, as I do
>>> not have libgcrypt installed, and it unconditionally checks
>>> sha256/gcrypt.h.
>>
>> Yep, for me too. There is a similar problem if you build with
>> NO_CURL and don't have the 'curl/curl.h' header file, etc ...
> 
> I think all it needs to do is to pass `$(ALL_CFLAGS)` to gcc (although we
> might need to add something like `#if defined(SHA256_GCRYPT) ... #endif`
> guards to `sha256/gcrypt.h` to make it work in Peff's case).

Just adding $(ALL_CFLAGS) to the command-line is not a cure-all,
unfortunately! :( Also, the 'sha256/gcrypt.h' should be excluded
from the list of headers to check, conditionally, based on the
build variable SHA256_GCRYPT. 

> But given that this target really is meant to catch all kinds of errors,
> I'd be in favor of declaring that target requiring things like libgcrypt's
> header files to be installed. We can easily make that happen in our CI
> builds.

That would help, but you may not be able to cover all the platform
specific build options available. dunno. (I haven't given it enough
thought).

>> The next version was essentially the same patch that started this thread
>> from Johannes (ie. the 'git ls-files' patch), given that the 'tags'
>> targets had found it necessary. However, when I did some 'informal'
>> timing tests (ie 5x 'time make ...' and average), this did not make any
>> noticeable difference for me (_even_ on cygwin!). ;-)
> 
> My complaint is not about the speed in a regular clone, but about *my*
> special clone, where I have some 50+ worktrees (that I really like to keep
> in the same directory, thankyouverymuch, I ignore them via
> `.git/info/exclude`, and I really like to have them all tucked away neatly
> under the `/usr/src/git/` directory) and also some 30+ cloned repositories
> in `3rdparty/` for random testing (including a bare clone of the Linux
> kernel, of course also ignored via `.git/info/exclude`).

Oh, wow! I can't imagine ever doing anything like that. However,
it is a perfectly valid setup and the build system should not
make it hard for you to work the way you want.

> In that scenario, my version of `find` takes ages, only to finally even
> throw a segmentation fault (!).
At least the segfault curtailed your misery! :-D

>> So, I already have a 'hdr-check-fixup' branch (I think I have
>> already mentioned it), in which the first commit looks like:
>>
>>   diff --git a/Makefile b/Makefile
>>   index c5240942f2..3401d1b9fb 100644
>>   --- a/Makefile
>>   +++ b/Makefile
>>   @@ -2735,9 +2735,10 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
>>    .PHONY: sparse $(SP_OBJ)
>>    sparse: $(SP_OBJ)
>>    
>>   +HC_HDRS := $(wildcard *.h negotiator/*.h block-sha1/*.h ppc/*.h ewah/*.h \
>>   +       sha1dc/*.h refs/*.h vcs-svn/*.h)
> 
> But of course that would also share at least to a lesser extend the
> shortcomings of a static list.

Yes, but it is no worse than ...

> I still like the `ls-files` method the best.
> 
>>    GEN_HDRS := command-list.h unicode-width.h
>>   -EXCEPT_HDRS := $(GEN_HDRS) compat% xdiff%
>>   -CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H)))

... the original, manually maintained, exception list!

(This patch was written before the 'sha256/' directory existed.
So the fact that it 'fixes' the GCRYPT error is simply an accident
of timing! Today, it would be conditionally added to the exception
list. Yes, I watched that error go from the 'pu' branch down to
the 'master' branch).

>>   +CHK_HDRS = $(filter-out $(GEN_HDRS),$(HC_HDRS))
>>    HCO = $(patsubst %.h,%.hco,$(CHK_HDRS))
>>    
>>    $(HCO): %.hco: %.h FORCE
>>   
>>
>> ... which drops the use of LIB_H entirely.
>>
>> So, if we really need to solve the problem, allowing for some
>> unrelated headers in your worktree, then we can only use a
>> static list, or a 'git ls-files' approach.
> 
> Or we can use `ls-files`, and fall back to your wildcard idea if
> `ls-files` fails for some reason (typically because `.git/` is missing,
> e.g. in case of an unpacked source .tar).

Yes, I think an 'git ls-files' approach is the way to go. (I am not
sure that the 'hdr-check' target would be of any use 'offline' at
all, but I suppose we could use a generated file in that case).

>> Anyway, for now, since I seem to be the only person using this
>> target, I think we should just remove it while I think again.
>> (I can put it in my config.mak, so there will be no loss for me).
> 
> As I said above, I would rather keep it, with the `ls-files` and `:=`
> fixup.

I would be happy with that, if you are. :-D

ATB,
Ramsay Jones

Re: [PATCH v2 1/1] Makefile: use `git ls-files` to list header files, if possible

[Ramsay Jones]

On 04/03/2019 13:47, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> In d85b0dff72 (Makefile: use `find` to determine static header
> dependencies, 2014-08-25), we switched from a static list of header
> files to a dynamically-generated one, asking `find` to enumerate them.
> 
> Back in those days, we did not use `$(LIB_H)` by default, and many a
> `make` implementation seems smart enough not to run that `find` command
> in that case, so it was deemed okay to run `find` for special targets
> requiring this macro.
> 
> However, as of ebb7baf02f (Makefile: add a hdr-check target,
> 2018-09-19), $(LIB_H) is part of a global rule and therefore must be
> expanded. Meaning: this `find` command has to be run upon every
> `make` invocation. In the presence of many a worktree, this can tax the
> developers' patience quite a bit.
> 
> Even in the absence of worktrees or other untracked files and
> directories, the cost of I/O to generate that list of header files is
> simply a lot larger than a simple `git ls-files` call.
> 
> Therefore, just like in 335339758c (Makefile: ask "ls-files" to list
> source files if available, 2011-10-18), we now prefer to use `git
> ls-files` to enumerate the header files to enumerating them via `find`,
> falling back to the latter if the former failed (which would be the case
> e.g. in a worktree that was extracted from a source .tar file rather
> than from a clone of Git's sources).
> 
> This has one notable consequence: we no longer include `command-list.h`
> in `LIB_H`, as it is a generated file, not a tracked one, but that is

Heh, just to be _unnecessarily_ picky, but this is not always true.
The 'command-list.h' header is _sometimes_ not included in the LIB_H
variable - it simply depends on whether it has been generated by the
time the $(FIND) is called.

Obviously, not worth a re-roll. Otherwise, this LGTM.

Thanks!

ATB,
Ramsay Jones

> easily worked around. Of the three sites that use `LIB_H`, two
> (`LOCALIZED_C` and `CHK_HDRS`) already handle generated headers
> separately. In the third, the computed-dependency fallback, we can just
> add in a reference to $(GENERATED_H).
> 
> Likewise, we no longer include not-yet-tracked header files in `LIB_H`.
> 
> Given the speed improvements, these consequences seem a comparably small
> price to pay.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  Makefile | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index c5240942f2..0c4712cb48 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -841,7 +841,8 @@ VCSSVN_LIB = vcs-svn/lib.a
>  
>  GENERATED_H += command-list.h
>  
> -LIB_H = $(shell $(FIND) . \
> +LIB_H := $(shell git ls-files '*.h' ':!t/' ':!Documentation/' 2>/dev/null || \
> +	$(FIND) . \
>  	-name .git -prune -o \
>  	-name t -prune -o \
>  	-name Documentation -prune -o \
> @@ -2363,7 +2364,7 @@ else
>  # should _not_ be included here, since they are necessary even when
>  # building an object for the first time.
>  
> -$(OBJECTS): $(LIB_H)
> +$(OBJECTS): $(LIB_H) $(GENERATED_H)
>  endif
>  
>  exec-cmd.sp exec-cmd.s exec-cmd.o: GIT-PREFIX
> 

Re: [PATCH v2 1/1] Makefile: use `git ls-files` to list header files, if possible

[Ramsay Jones]

On 04/03/2019 20:38, Ramsay Jones wrote:
> 
> 
> On 04/03/2019 13:47, Johannes Schindelin via GitGitGadget wrote:
>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>>
>> In d85b0dff72 (Makefile: use `find` to determine static header
>> dependencies, 2014-08-25), we switched from a static list of header
>> files to a dynamically-generated one, asking `find` to enumerate them.
>>
>> Back in those days, we did not use `$(LIB_H)` by default, and many a
>> `make` implementation seems smart enough not to run that `find` command
>> in that case, so it was deemed okay to run `find` for special targets
>> requiring this macro.
>>
>> However, as of ebb7baf02f (Makefile: add a hdr-check target,
>> 2018-09-19), $(LIB_H) is part of a global rule and therefore must be
>> expanded. Meaning: this `find` command has to be run upon every
>> `make` invocation. In the presence of many a worktree, this can tax the
>> developers' patience quite a bit.
>>
>> Even in the absence of worktrees or other untracked files and
>> directories, the cost of I/O to generate that list of header files is
>> simply a lot larger than a simple `git ls-files` call.
>>
>> Therefore, just like in 335339758c (Makefile: ask "ls-files" to list
>> source files if available, 2011-10-18), we now prefer to use `git
>> ls-files` to enumerate the header files to enumerating them via `find`,
>> falling back to the latter if the former failed (which would be the case
>> e.g. in a worktree that was extracted from a source .tar file rather
>> than from a clone of Git's sources).
>>
>> This has one notable consequence: we no longer include `command-list.h`
>> in `LIB_H`, as it is a generated file, not a tracked one, but that is
> 
> Heh, just to be _unnecessarily_ picky, but this is not always true.
> The 'command-list.h' header is _sometimes_ not included in the LIB_H
> variable - it simply depends on whether it has been generated by the
> time the $(FIND) is called.
> 
> Obviously, not worth a re-roll. Otherwise, this LGTM.

Ahem! Obviously, I didn't read the commit message closely enough!

However, _before_ this change, then 'command-list.h' was sometimes
not included in $(LIB_H) ...

Sorry for the noise!

ATB,
Ramsay Jones

Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible

[Jeff King]

On Mon, Mar 04, 2019 at 01:38:13PM +0100, Johannes Schindelin wrote:

> > So, tl;dr: soon, I will be submitting a patch to remove the
> > 'hdr-check' target completely, for now anyway.
> 
> You mentioned later that you might be the only person using that target,
> and if that were so, I would agree.
> 
> However, I started looking into adding `hdr-check` to the Azure Pipeline
> we have (which would make a *ton* of sense, if you ask me), along with the
> `sparse` thing we talked about.
> 
> So I am a bit opposed to removing that target.

Yeah, I agree. If it's a linting check we expect developers to follow,
then we need to make it easy for developers to run it.

> > Yep, for me too. There is a similar problem if you build with
> > NO_CURL and don't have the 'curl/curl.h' header file, etc ...
> 
> I think all it needs to do is to pass `$(ALL_CFLAGS)` to gcc (although we
> might need to add something like `#if defined(SHA256_GCRYPT) ... #endif`
> guards to `sha256/gcrypt.h` to make it work in Peff's case).
> 
> But given that this target really is meant to catch all kinds of errors,
> I'd be in favor of declaring that target requiring things like libgcrypt's
> header files to be installed. We can easily make that happen in our CI
> builds.

I don't think that really scales, though. For CI, perhaps it's not too
bad, but IMHO there's real value making it possible for everyday
developers to run linting and tests locally, because it keeps the
feedback cycle tight. In other words, imagine my changes go through a
sequence like this:

  1. I run "make" locally

  2. I run "make test" locally

  3. I push, and CI runs "make && make test" on more platforms, or with
     more knobs tweaked.

it's preferable to find out about problems in the early steps, because
the cost to run each of the subsequent steps is much higher.

So if we're adding more linting, it would ideally be part of the build.
The obvious reasons not to are that it's expensive, or that it's hard
for the local user to set up. But I think at least a hdr-check that
checks _your_ platform would be valuable to run as part of step 1 or 2.

I also think it points to a weakness in the current hdr-check scheme. It
omits compat/* entirely, so we can't detect any problems there. But if
we are going to add those back in, then making it run in CI is not as
simple as "just make sure we have all libraries installed". It's
inherently going to be a different set per platform (and we'd want to
run "make hdr-check" on each of the platforms where we already run "make
test").

> > Anyway, for now, since I seem to be the only person using this
> > target, I think we should just remove it while I think again.
> > (I can put it in my config.mak, so there will be no loss for me).
> 
> As I said above, I would rather keep it, with the `ls-files` and `:=`
> fixup.
> 
> If others *really* feel strongly about lazy-evaluating `LIB_H`, then I
> would *additionally* use Peff's sub-make hack. But only if others feel
> strongly about it, because I, for one, certainly don't.

My measurements show it's not a big expense at least on my system (even
with the 3x runs). So I can live with just moving it to ls-files to
bound the filesystem access, and calling that good enough.

-Peff

Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible

[Jeff King]

On Mon, Mar 04, 2019 at 12:08:41PM +0100, Johannes Schindelin wrote:

> -- snipsnap --
> From c13d9985284d4b452db0d95b6949e02c533db634 Mon Sep 17 00:00:00 2001
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> Date: Sat, 23 Feb 2019 20:38:40 +0100
> Subject: [PATCH] hdr-check: make it work on Windows
> 
> On Windows, we define a specific set of pre-processor macros, among
> other reasons: to avoid including syslog.h (which is not available on
> Windows).
> 
> The hdr-check target did not use those definitions, resulting in a
> failure to include said syslog.h.
> 
> To fix that, let's let the hdr-check target make use of ALL_CFLAGS,
> which does include the pre-processor macros that would e.g. skip the
> conditional block including syslog.h.

This makes sense to me, though as you noted elsewhere, it doesn't fix
the gcrypt problem, since that file unconditionally wants to look at the
system gcrypt.h (and we control at the Makefile level whether we
actually look at sha256/gcrypt.h).

-Peff

Re: [PATCH v2 1/1] Makefile: use `git ls-files` to list header files, if possible

[Jeff King]

On Mon, Mar 04, 2019 at 05:47:06AM -0800, Johannes Schindelin via GitGitGadget wrote:

> Therefore, just like in 335339758c (Makefile: ask "ls-files" to list
> source files if available, 2011-10-18), we now prefer to use `git
> ls-files` to enumerate the header files to enumerating them via `find`,
> falling back to the latter if the former failed (which would be the case
> e.g. in a worktree that was extracted from a source .tar file rather
> than from a clone of Git's sources).

Thanks, this version looks good to me, and seems like an obvious first
step regardless of whether we want to later push it into a sub-make.

-Peff

Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> This makes sense to me, though as you noted elsewhere, it doesn't fix
> the gcrypt problem, since that file unconditionally wants to look at the
> system gcrypt.h (and we control at the Makefile level whether we
> actually look at sha256/gcrypt.h).

Hmm, is that because the header check target does not know which *.h
files we ship are actually used in a particular build?

After a normal build, with dynamic dependency checking on, we would
have sufficient information to figure it out.  Would that help?

Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible

[Johannes Schindelin]

Hi Junio,

On Tue, 5 Mar 2019, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > This makes sense to me, though as you noted elsewhere, it doesn't fix
> > the gcrypt problem, since that file unconditionally wants to look at the
> > system gcrypt.h (and we control at the Makefile level whether we
> > actually look at sha256/gcrypt.h).
> 
> Hmm, is that because the header check target does not know which *.h
> files we ship are actually used in a particular build?
> 
> After a normal build, with dynamic dependency checking on, we would
> have sufficient information to figure it out.  Would that help?

Yes, *if* the dynamic dependency checking is in effect (read: if we are
compiling with GCC).

However, I think that one of the benefits of the current approach is that
hdr-check finds issues also in headers that are *not* part of the current
build. Read: once hdr-check is run as part of the CI build, it is harder
for some random contributor to break hdr-check for somebody else with
different compile time options.

Ciao,
Dscho

Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible

[Junio C Hamano]

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

>> After a normal build, with dynamic dependency checking on, we would
>> have sufficient information to figure it out.  Would that help?
>
> Yes, *if* the dynamic dependency checking is in effect (read: if we are
> compiling with GCC).
>
> However, I think that one of the benefits of the current approach is that
> hdr-check finds issues also in headers that are *not* part of the current
> build. Read: once hdr-check is run as part of the CI build, it is harder
> for some random contributor to break hdr-check for somebody else with
> different compile time options.

Sure.  That is exactly why we have multiple configurations at the
CI, hopefully more than each individual contributor has at hand so
that CI can give more coverage than any single individual can test.

What I was assuming was that at least one of these multiple
configurations would cover the case where sha256/gcrypt.h truly is
used, because that particular build is configured to use the system
header and library.  And it would be a far better approach to make
that build check also for the header, if we can cover all the
problematic (in the same sense as the sha256/gcrypt.h) header files,
than unconditionally "checking" without knowing the prerequisite and
causing a false positive.

Having said that, the optional use of ls-files introduced by the
patch in question is a valid incremental improvement over the
current Makefile, so it is a pretty much independent/orthogonal
issue.

Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible

[Jeff King]

On Tue, Mar 05, 2019 at 02:50:11PM +0900, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > This makes sense to me, though as you noted elsewhere, it doesn't fix
> > the gcrypt problem, since that file unconditionally wants to look at the
> > system gcrypt.h (and we control at the Makefile level whether we
> > actually look at sha256/gcrypt.h).
> 
> Hmm, is that because the header check target does not know which *.h
> files we ship are actually used in a particular build?

Yes, exactly.

> After a normal build, with dynamic dependency checking on, we would
> have sufficient information to figure it out.  Would that help?

Yeah, that's what I was hinting at earlier in the thread. Here it is
sketched out to an actual working patch. The sub-make bits could
actually be a shell script instead of a Makefile; the only point in
using make is to use the parent "-j" for parallelism.

Note also that by including compat/ headers, I noticed that bswap.h
fails its hdr-check. :)

So do command-list.h (it needs gettext.h to handle the _() markings, but
that only comes in via cache.h), and all of xdiff (which is probably
fixable, but I didn't bother here).

---
 Makefile       | 23 ++++++++++++-----------
 compat/bswap.h |  5 +++++
 2 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/Makefile b/Makefile
index c5240942f2..283e934d7b 100644
--- a/Makefile
+++ b/Makefile
@@ -1852,7 +1852,6 @@ ifndef V
 	QUIET_MSGFMT   = @echo '   ' MSGFMT $@;
 	QUIET_GCOV     = @echo '   ' GCOV $@;
 	QUIET_SP       = @echo '   ' SP $<;
-	QUIET_HDR      = @echo '   ' HDR $<;
 	QUIET_RC       = @echo '   ' RC $@;
 	QUIET_SUBDIR0  = +@subdir=
 	QUIET_SUBDIR1  = ;$(NO_SUBDIR) echo '   ' SUBDIR $$subdir; \
@@ -2735,16 +2734,18 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
 .PHONY: sparse $(SP_OBJ)
 sparse: $(SP_OBJ)
 
-GEN_HDRS := command-list.h unicode-width.h
-EXCEPT_HDRS := $(GEN_HDRS) compat% xdiff%
-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 $<
-
-.PHONY: hdr-check $(HCO)
-hdr-check: $(HCO)
+.PHONY: hdr-check
+hdr-check: all
+ifdef USE_COMPUTED_HEADER_DEPENDENCIES
+	@$(MAKE) -f hdr-check.mak CC="$(CC)" V=$(V) \
+		$$(sed -n 's/^\([^ ]*\)\.h:/\1.hco/p' .depend/* | \
+		   sort -u | \
+		   egrep -v '^(xdiff/|unicode-width.h|command-list.h)' \
+		)
+else
+	@echo >&2 "error: hdr-check supported only on platforms with computed dependencies"
+	@false
+endif
 
 .PHONY: style
 style:
diff --git a/compat/bswap.h b/compat/bswap.h
index 5078ce5ecc..e4e25735ce 100644
--- a/compat/bswap.h
+++ b/compat/bswap.h
@@ -1,3 +1,6 @@
+#ifndef COMPAT_BSWAP_H
+#define COMPAT_BSWAP_H
+
 /*
  * Let's make sure we always have a sane definition for ntohl()/htonl().
  * Some libraries define those as a function call, just to perform byte
@@ -210,3 +213,5 @@ static inline void put_be64(void *ptr, uint64_t value)
 }
 
 #endif
+
+#endif /* COMPAT_BSWAP_H */
-- 
2.21.0.684.gc9dc8b89c9

Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible

[Ramsay Jones]

On 05/03/2019 23:07, Jeff King wrote:
> On Tue, Mar 05, 2019 at 02:50:11PM +0900, Junio C Hamano wrote:
> 
>> Jeff King <peff@peff.net> writes:
>>
>>> This makes sense to me, though as you noted elsewhere, it doesn't fix
>>> the gcrypt problem, since that file unconditionally wants to look at the
>>> system gcrypt.h (and we control at the Makefile level whether we
>>> actually look at sha256/gcrypt.h).
>>
>> Hmm, is that because the header check target does not know which *.h
>> files we ship are actually used in a particular build?
> 
> Yes, exactly.
> 
>> After a normal build, with dynamic dependency checking on, we would
>> have sufficient information to figure it out.  Would that help?
> 
> Yeah, that's what I was hinting at earlier in the thread. Here it is
> sketched out to an actual working patch. The sub-make bits could
> actually be a shell script instead of a Makefile; the only point in
> using make is to use the parent "-j" for parallelism.

sigh. :(

I wish my patch removing this target had been picked up now!

Earlier this evening I spent an hour or so writing an email which
tried to discourage spending any time on this, because of the
potential for this to be a huge time-suck. An unrewarding one at
that! :-D

The email was actually prompted by someone mentioning 'dynamic
compiler dependencies', because that's how it always starts ...

I deleted that email (it's not in my drafts folder anyway)
because, in the end, it is not up to me to say how people should
spend their time.

So I won't! :-D

ATB,
Ramsay Jones

Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible

[Jeff King]

On Wed, Mar 06, 2019 at 12:23:20AM +0000, Ramsay Jones wrote:

> > Yeah, that's what I was hinting at earlier in the thread. Here it is
> > sketched out to an actual working patch. The sub-make bits could
> > actually be a shell script instead of a Makefile; the only point in
> > using make is to use the parent "-j" for parallelism.
> 
> sigh. :(
> 
> I wish my patch removing this target had been picked up now!
> 
> Earlier this evening I spent an hour or so writing an email which
> tried to discourage spending any time on this, because of the
> potential for this to be a huge time-suck. An unrewarding one at
> that! :-D

Heh. I am OK with removing it, too.

My thinking earlier in the thread was that it should go in our bag of
linting tools that people should generally run. But actually, it is kind
of expensive to run, and it does not actually help anything immediately
in practice. I.e., what we really care about is that the C source files
compile, and running "make" does that (and especially running it on
various platforms). This is just checking for a _potential_ problem if
somebody were to include a particular header file at the start of
another C file.

So it's really about 2 steps removed from stopping an actual problem.

> I deleted that email (it's not in my drafts folder anyway)
> because, in the end, it is not up to me to say how people should
> spend their time.

FWIW, it was a fun exercise to build on the compiler dependencies. ;)
And I think I'm done playing with it for now. "make hdr-check" does not
run for me, but I think I've convinced myself that it's not all that
important that I run it.

I did forget to include one file in my earlier patch (the newly added
hdr-check.mak). I imagine one could guess at its contents, but here is
the complete patch, for the sake of reference.

-Peff

---
 Makefile       | 23 ++++++++++++-----------
 compat/bswap.h |  5 +++++
 hdr-check.mak  |  8 ++++++++
 3 files changed, 25 insertions(+), 11 deletions(-)
 create mode 100644 hdr-check.mak

diff --git a/Makefile b/Makefile
index c5240942f2..283e934d7b 100644
--- a/Makefile
+++ b/Makefile
@@ -1852,7 +1852,6 @@ ifndef V
 	QUIET_MSGFMT   = @echo '   ' MSGFMT $@;
 	QUIET_GCOV     = @echo '   ' GCOV $@;
 	QUIET_SP       = @echo '   ' SP $<;
-	QUIET_HDR      = @echo '   ' HDR $<;
 	QUIET_RC       = @echo '   ' RC $@;
 	QUIET_SUBDIR0  = +@subdir=
 	QUIET_SUBDIR1  = ;$(NO_SUBDIR) echo '   ' SUBDIR $$subdir; \
@@ -2735,16 +2734,18 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
 .PHONY: sparse $(SP_OBJ)
 sparse: $(SP_OBJ)
 
-GEN_HDRS := command-list.h unicode-width.h
-EXCEPT_HDRS := $(GEN_HDRS) compat% xdiff%
-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 $<
-
-.PHONY: hdr-check $(HCO)
-hdr-check: $(HCO)
+.PHONY: hdr-check
+hdr-check: all
+ifdef USE_COMPUTED_HEADER_DEPENDENCIES
+	@$(MAKE) -f hdr-check.mak CC="$(CC)" V=$(V) \
+		$$(sed -n 's/^\([^ ]*\)\.h:/\1.hco/p' .depend/* | \
+		   sort -u | \
+		   egrep -v '^(xdiff/|unicode-width.h|command-list.h)' \
+		)
+else
+	@echo >&2 "error: hdr-check supported only on platforms with computed dependencies"
+	@false
+endif
 
 .PHONY: style
 style:
diff --git a/compat/bswap.h b/compat/bswap.h
index 5078ce5ecc..e4e25735ce 100644
--- a/compat/bswap.h
+++ b/compat/bswap.h
@@ -1,3 +1,6 @@
+#ifndef COMPAT_BSWAP_H
+#define COMPAT_BSWAP_H
+
 /*
  * Let's make sure we always have a sane definition for ntohl()/htonl().
  * Some libraries define those as a function call, just to perform byte
@@ -210,3 +213,5 @@ static inline void put_be64(void *ptr, uint64_t value)
 }
 
 #endif
+
+#endif /* COMPAT_BSWAP_H */
diff --git a/hdr-check.mak b/hdr-check.mak
new file mode 100644
index 0000000000..00a3110fda
--- /dev/null
+++ b/hdr-check.mak
@@ -0,0 +1,8 @@
+.PHONY: FORCE
+
+ifndef V
+QUIET_HDR = @echo '   ' HDR $<;
+endif
+
+%.hco: %.h FORCE
+	$(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $<
-- 
2.21.0.699.ge1748d4d73

Re: [PATCH 1/1] Makefile: use `git ls-files` to list header files, if possible

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Mar 06, 2019 at 12:23:20AM +0000, Ramsay Jones wrote:
>
>> > Yeah, that's what I was hinting at earlier in the thread. Here it is
>> > sketched out to an actual working patch. The sub-make bits could
>> > actually be a shell script instead of a Makefile; the only point in
>> > using make is to use the parent "-j" for parallelism.
>> 
>> sigh. :(
>> 
>> I wish my patch removing this target had been picked up now!
>> 
>> Earlier this evening I spent an hour or so writing an email which
>> tried to discourage spending any time on this, because of the
>> potential for this to be a huge time-suck. An unrewarding one at
>> that! :-D
>
> Heh. I am OK with removing it, too.

FWIW, I am fine with that plan as well.

> My thinking earlier in the thread was that it should go in our bag of
> linting tools that people should generally run. But actually, it is kind
> of expensive to run, and it does not actually help anything immediately
> in practice. I.e., what we really care about is that the C source files
> compile, and running "make" does that (and especially running it on
> various platforms). This is just checking for a _potential_ problem if
> somebody were to include a particular header file at the start of
> another C file.
>
> So it's really about 2 steps removed from stopping an actual problem.

Yeah, exactly.

> diff --git a/compat/bswap.h b/compat/bswap.h
> index 5078ce5ecc..e4e25735ce 100644
> --- a/compat/bswap.h
> +++ b/compat/bswap.h
> @@ -1,3 +1,6 @@
> +#ifndef COMPAT_BSWAP_H
> +#define COMPAT_BSWAP_H
> +
>  /*
>   * Let's make sure we always have a sane definition for ntohl()/htonl().
>   * Some libraries define those as a function call, just to perform byte
> @@ -210,3 +213,5 @@ static inline void put_be64(void *ptr, uint64_t value)
>  }
>  
>  #endif
> +
> +#endif /* COMPAT_BSWAP_H */

This probably is worth having as an independent clean-up.

[PATCH] compat/bswap: add include header guards

[Jeff King]

On Wed, Mar 06, 2019 at 02:28:01PM +0900, Junio C Hamano wrote:

> > +#ifndef COMPAT_BSWAP_H
> > +#define COMPAT_BSWAP_H
> [...]
> 
> This probably is worth having as an independent clean-up.

Yeah, let's do that now before we forget.

-- >8 --
Subject: [PATCH] compat/bswap: add include header guards

Our compat/bswap.h lacks the usual preprocessor guards against multiple
inclusion. This usually isn't an issue since it only gets included from
git-compat-util.h, which has its own guards. But it would produce
redeclaration errors if any file included it separately.

Our hdr-check target would complain about this, except that it currently
skips items in compat/ entirely.

Signed-off-by: Jeff King <peff@peff.net>
---
 compat/bswap.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/compat/bswap.h b/compat/bswap.h
index 5078ce5ecc..e4e25735ce 100644
--- a/compat/bswap.h
+++ b/compat/bswap.h
@@ -1,3 +1,6 @@
+#ifndef COMPAT_BSWAP_H
+#define COMPAT_BSWAP_H
+
 /*
  * Let's make sure we always have a sane definition for ntohl()/htonl().
  * Some libraries define those as a function call, just to perform byte
@@ -210,3 +213,5 @@ static inline void put_be64(void *ptr, uint64_t value)
 }
 
 #endif
+
+#endif /* COMPAT_BSWAP_H */
-- 
2.21.0.699.ge1748d4d73

Re: [PATCH] compat/bswap: add include header guards

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Mar 06, 2019 at 02:28:01PM +0900, Junio C Hamano wrote:
>
>> > +#ifndef COMPAT_BSWAP_H
>> > +#define COMPAT_BSWAP_H
>> [...]
>> 
>> This probably is worth having as an independent clean-up.
>
> Yeah, let's do that now before we forget.

Thanks

>
> -- >8 --
> Subject: [PATCH] compat/bswap: add include header guards
> ...