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

[Ramsay Jones <ramsay@ramsayjones.plus.com>]

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