[PATCH] Update hard-coded header dependencies

[PATCH] Update hard-coded header dependencies

[Jonathan Nieder]

The fall-back rules used when compilers don't support the -MMD switch
to generate makefile rules based on #includes have been out of date
since v1.7.12.1~22^2~8 (move git_version_string into version.c,
2012-06-02).

Checked with 'make CHECK_HEADER_DEPENDENCIES=yes'.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Maybe it's worth switching to plain

	LIB_H += $(wildcard *.h)

?  People using ancient compilers that never change headers wouldn't
be hurt, people using modern compilers that do change headers also
wouldn't be hurt, and we could stop pretending to maintain an
up-to-date list.

 Makefile | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Makefile b/Makefile
index 2320de5..18f0fad 100644
--- a/Makefile
+++ b/Makefile
@@ -646,15 +646,19 @@ LIB_H += cache.h
 LIB_H += color.h
 LIB_H += column.h
 LIB_H += commit.h
+LIB_H += commit-slab.h
+LIB_H += compat/apple-common-crypto.h
 LIB_H += compat/bswap.h
 LIB_H += compat/mingw.h
 LIB_H += compat/obstack.h
 LIB_H += compat/poll/poll.h
 LIB_H += compat/precompose_utf8.h
 LIB_H += compat/terminal.h
+LIB_H += compat/win32/alloca.h
 LIB_H += compat/win32/dirent.h
 LIB_H += compat/win32/pthread.h
 LIB_H += compat/win32/syslog.h
+LIB_H += connect.h
 LIB_H += connected.h
 LIB_H += convert.h
 LIB_H += credential.h
@@ -678,6 +682,7 @@ LIB_H += grep.h
 LIB_H += hashmap.h
 LIB_H += help.h
 LIB_H += http.h
+LIB_H += khash.h
 LIB_H += kwset.h
 LIB_H += levenshtein.h
 LIB_H += line-log.h
@@ -721,6 +726,7 @@ LIB_H += sha1-lookup.h
 LIB_H += shortlog.h
 LIB_H += sideband.h
 LIB_H += sigchain.h
+LIB_H += split-index.h
 LIB_H += strbuf.h
 LIB_H += streaming.h
 LIB_H += string-list.h
@@ -728,6 +734,7 @@ LIB_H += submodule.h
 LIB_H += tag.h
 LIB_H += tar.h
 LIB_H += thread-utils.h
+LIB_H += trace.h
 LIB_H += transport.h
 LIB_H += tree-walk.h
 LIB_H += tree.h
@@ -744,6 +751,7 @@ LIB_H += vcs-svn/repo_tree.h
 LIB_H += vcs-svn/sliding_window.h
 LIB_H += vcs-svn/svndiff.h
 LIB_H += vcs-svn/svndump.h
+LIB_H += version.h
 LIB_H += walker.h
 LIB_H += wildmatch.h
 LIB_H += wt-status.h
-- 

Re: [PATCH] Update hard-coded header dependencies

[Jeff King]

On Fri, Aug 08, 2014 at 02:58:26PM -0700, Jonathan Nieder wrote:

> Maybe it's worth switching to plain
> 
> 	LIB_H += $(wildcard *.h)
> 
> ?  People using ancient compilers that never change headers wouldn't
> be hurt, people using modern compilers that do change headers also
> wouldn't be hurt, and we could stop pretending to maintain an
> up-to-date list.

Yeah, I think that makes sense. I'd imagine most of the developers are
on a modern platform and don't use the static list at all, so we don't
notice when it breaks (and even when you do use it, it's quite hard to
notice anyway).

We'd have to do a multi-directory wildcard, though, to catch the header
files stuck in compat/* and elsewhere. We could list the containing
directories manually, but that's yet another thing to go wrong. For
people using the git repo, it would probably be fine to do:

  LIB_H += $(shell git ls-files -- '*.h')

That wouldn't count new files a developer adds until they "git add" some
version of the file, but that is not so bad (right now they have to add
it to the Makefile, and anyway, I think most devs are using the computed
dependencies).

But that doesn't work for distributed tarballs, which would have to
convert that to a static list somehow. Maybe

  LIB_H += $(shell find . -name '*.h' -print)

would work?

-Peff

Re: [PATCH] Update hard-coded header dependencies

[Jeff King]

On Sun, Aug 10, 2014 at 03:48:24PM -0400, Jeff King wrote:

> On Fri, Aug 08, 2014 at 02:58:26PM -0700, Jonathan Nieder wrote:
> 
> > Maybe it's worth switching to plain
> > 
> > 	LIB_H += $(wildcard *.h)
> > 
> > ?  People using ancient compilers that never change headers wouldn't
> > be hurt, people using modern compilers that do change headers also
> > wouldn't be hurt, and we could stop pretending to maintain an
> > up-to-date list.
> [...]
> Maybe
> 
>   LIB_H += $(shell find . -name '*.h' -print)
> 
> would work?

I took a stab at this and it seems to work. Here's a series.

  [1/2]: Makefile: use `find` to determine static header dependencies
  [2/2]: Makefile: drop CHECK_HEADER_DEPENDENCIES code

-Peff

[PATCH 1/2] Makefile: use "find" to determine static header dependencies

[Jeff King]

Most modern platforms will use automatically computed header
dependencies to figure out when a C file needs to be rebuilt
due to a header changing. With old compilers, however, we
fall back to a static list of header files. If any of them
changes, we recompile everything. This is overly
conservative, but the best we can do without compiler
support.

It is unfortunately easy for our static header list to grow
stale, as none of the regular developers make use of it.
Instead of trying to keep it up to date, let's invoke "find"
to generate the list dynamically.

We'd like to avoid running find at all when it is not
necessary, since it may add a non-trivial amount of time to
the build.  Make is _almost_ smart enough to avoid
evaluating the function when it is not necessary. For the
static header dependencies, we include $(LIB_H) as a
dependency only if COMPUTE_HEADER_DEPENDENCIES is turned
off, so we don't trigger its use there unless necessary. So
far so good.

However, we do always define $(LIB_H) as a dependency of
po/git.pot. Even though we do not actually try to build that
target, make will still evaluate the dependencies when
reading the Makefile, and expand the variable. This is not
ideal because almost nobody wants to build po/git.pot (only
the translation maintainer does it, and even then only once
or twice per release). We can hack around this by invoking a
sub-make which evaluates the variable only when po/git.pot
is actually being built.

Signed-off-by: Jeff King <peff@peff.net>
---
I also optimized the "find" a bit by pruning out some
directories that are almost certainly uninteresting. That
means we wouldn't catch an include of "t/foo.h", but I think
that is probably an OK assumption.

I'm open to attempts to improve my ugly git.pot hack. I
thought at first it was caused by the use of ":=" in
assigning LOCALIZED_C, but after much testing, I think it is
actually the expansion of the dependencies.

 Makefile | 143 ++++++---------------------------------------------------------
 1 file changed, 13 insertions(+), 130 deletions(-)

diff --git a/Makefile b/Makefile
index 2320de5..08dd973 100644
--- a/Makefile
+++ b/Makefile
@@ -432,7 +432,6 @@ XDIFF_OBJS =
 VCSSVN_OBJS =
 GENERATED_H =
 EXTRA_CPPFLAGS =
-LIB_H =
 LIB_OBJS =
 PROGRAM_OBJS =
 PROGRAMS =
@@ -631,131 +630,11 @@ VCSSVN_LIB = vcs-svn/lib.a
 
 GENERATED_H += common-cmds.h
 
-LIB_H += advice.h
-LIB_H += archive.h
-LIB_H += argv-array.h
-LIB_H += attr.h
-LIB_H += bisect.h
-LIB_H += blob.h
-LIB_H += branch.h
-LIB_H += builtin.h
-LIB_H += bulk-checkin.h
-LIB_H += bundle.h
-LIB_H += cache-tree.h
-LIB_H += cache.h
-LIB_H += color.h
-LIB_H += column.h
-LIB_H += commit.h
-LIB_H += compat/bswap.h
-LIB_H += compat/mingw.h
-LIB_H += compat/obstack.h
-LIB_H += compat/poll/poll.h
-LIB_H += compat/precompose_utf8.h
-LIB_H += compat/terminal.h
-LIB_H += compat/win32/dirent.h
-LIB_H += compat/win32/pthread.h
-LIB_H += compat/win32/syslog.h
-LIB_H += connected.h
-LIB_H += convert.h
-LIB_H += credential.h
-LIB_H += csum-file.h
-LIB_H += decorate.h
-LIB_H += delta.h
-LIB_H += diff.h
-LIB_H += diffcore.h
-LIB_H += dir.h
-LIB_H += exec_cmd.h
-LIB_H += ewah/ewok.h
-LIB_H += ewah/ewok_rlw.h
-LIB_H += fetch-pack.h
-LIB_H += fmt-merge-msg.h
-LIB_H += fsck.h
-LIB_H += gettext.h
-LIB_H += git-compat-util.h
-LIB_H += gpg-interface.h
-LIB_H += graph.h
-LIB_H += grep.h
-LIB_H += hashmap.h
-LIB_H += help.h
-LIB_H += http.h
-LIB_H += kwset.h
-LIB_H += levenshtein.h
-LIB_H += line-log.h
-LIB_H += line-range.h
-LIB_H += list-objects.h
-LIB_H += ll-merge.h
-LIB_H += log-tree.h
-LIB_H += mailmap.h
-LIB_H += merge-blobs.h
-LIB_H += merge-recursive.h
-LIB_H += mergesort.h
-LIB_H += notes-cache.h
-LIB_H += notes-merge.h
-LIB_H += notes-utils.h
-LIB_H += notes.h
-LIB_H += object.h
-LIB_H += pack-objects.h
-LIB_H += pack-revindex.h
-LIB_H += pack.h
-LIB_H += pack-bitmap.h
-LIB_H += parse-options.h
-LIB_H += patch-ids.h
-LIB_H += pathspec.h
-LIB_H += pkt-line.h
-LIB_H += prio-queue.h
-LIB_H += progress.h
-LIB_H += prompt.h
-LIB_H += quote.h
-LIB_H += reachable.h
-LIB_H += reflog-walk.h
-LIB_H += refs.h
-LIB_H += remote.h
-LIB_H += rerere.h
-LIB_H += resolve-undo.h
-LIB_H += revision.h
-LIB_H += run-command.h
-LIB_H += send-pack.h
-LIB_H += sequencer.h
-LIB_H += sha1-array.h
-LIB_H += sha1-lookup.h
-LIB_H += shortlog.h
-LIB_H += sideband.h
-LIB_H += sigchain.h
-LIB_H += strbuf.h
-LIB_H += streaming.h
-LIB_H += string-list.h
-LIB_H += submodule.h
-LIB_H += tag.h
-LIB_H += tar.h
-LIB_H += thread-utils.h
-LIB_H += transport.h
-LIB_H += tree-walk.h
-LIB_H += tree.h
-LIB_H += unpack-trees.h
-LIB_H += unicode_width.h
-LIB_H += url.h
-LIB_H += urlmatch.h
-LIB_H += userdiff.h
-LIB_H += utf8.h
-LIB_H += varint.h
-LIB_H += vcs-svn/fast_export.h
-LIB_H += vcs-svn/line_buffer.h
-LIB_H += vcs-svn/repo_tree.h
-LIB_H += vcs-svn/sliding_window.h
-LIB_H += vcs-svn/svndiff.h
-LIB_H += vcs-svn/svndump.h
-LIB_H += walker.h
-LIB_H += wildmatch.h
-LIB_H += wt-status.h
-LIB_H += xdiff-interface.h
-LIB_H += xdiff/xdiff.h
-LIB_H += xdiff/xdiffi.h
-LIB_H += xdiff/xemit.h
-LIB_H += xdiff/xinclude.h
-LIB_H += xdiff/xmacros.h
-LIB_H += xdiff/xprepare.h
-LIB_H += xdiff/xtypes.h
-LIB_H += xdiff/xutils.h
+LIB_H = $(shell find . \
+	-name .git -prune -o \
+	-name t -prune -o \
+	-name Documentation -prune -o \
+	-name '*.h' -print)
 
 LIB_OBJS += abspath.o
 LIB_OBJS += advice.o
@@ -1381,7 +1260,6 @@ ifdef NO_INET_PTON
 endif
 ifndef NO_UNIX_SOCKETS
 	LIB_OBJS += unix-socket.o
-	LIB_H += unix-socket.h
 	PROGRAM_OBJS += credential-cache.o
 	PROGRAM_OBJS += credential-cache--daemon.o
 endif
@@ -1405,12 +1283,10 @@ endif
 ifdef BLK_SHA1
 	SHA1_HEADER = "block-sha1/sha1.h"
 	LIB_OBJS += block-sha1/sha1.o
-	LIB_H += block-sha1/sha1.h
 else
 ifdef PPC_SHA1
 	SHA1_HEADER = "ppc/sha1.h"
 	LIB_OBJS += ppc/sha1.o ppc/sha1ppc.o
-	LIB_H += ppc/sha1.h
 else
 ifdef APPLE_COMMON_CRYPTO
 	COMPAT_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL
@@ -2128,7 +2004,7 @@ XGETTEXT_FLAGS_C = $(XGETTEXT_FLAGS) --language=C \
 XGETTEXT_FLAGS_SH = $(XGETTEXT_FLAGS) --language=Shell \
 	--keyword=gettextln --keyword=eval_gettextln
 XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --keyword=__ --language=Perl
-LOCALIZED_C := $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)
+LOCALIZED_C := $(C_OBJ:o=c) $(GENERATED_H)
 LOCALIZED_SH := $(SCRIPT_SH)
 LOCALIZED_PERL := $(SCRIPT_PERL)
 
@@ -2138,6 +2014,8 @@ LOCALIZED_SH += t/t0200/test.sh
 LOCALIZED_PERL += t/t0200/test.perl
 endif
 
+ifdef REAL_GIT_POT
+LOCALIZED_C += $(LIB_H)
 po/git.pot: $(LOCALIZED_C)
 	$(QUIET_XGETTEXT)$(XGETTEXT) -o$@+ $(XGETTEXT_FLAGS_C) $(LOCALIZED_C)
 	$(QUIET_XGETTEXT)$(XGETTEXT) -o$@+ --join-existing $(XGETTEXT_FLAGS_SH) \
@@ -2145,6 +2023,11 @@ po/git.pot: $(LOCALIZED_C)
 	$(QUIET_XGETTEXT)$(XGETTEXT) -o$@+ --join-existing $(XGETTEXT_FLAGS_PERL) \
 		$(LOCALIZED_PERL)
 	mv $@+ $@
+else
+.PHONY: po/git.pot
+po/git.pot:
+	@$(MAKE) po/git.pot REAL_GIT_POT=Yes
+endif
 
 pot: po/git.pot
 
-- 
2.1.0.346.ga0367b9

Re: [PATCH 1/2] Makefile: use "find" to determine static header dependencies

[Jonathan Nieder]

Hi,

Jeff King wrote:

> However, we do always define $(LIB_H) as a dependency of
> po/git.pot. Even though we do not actually try to build that
> target, make will still evaluate the dependencies when
> reading the Makefile, and expand the variable. This is not
> ideal

Would the following work?  The current dependencies for po/git.pot are
not correct anyway --- they include LOCALIZED_C but not LOCALIZED_SH
or LOCALIZED_PERL, so someone hacking on shell scripts and then trying
'make po/git.pot' could end up with the pot file not being
regenerated.

-- >8 --
Subject: i18n: treat "make pot" as an explicitly-invoked target

po/git.pot is normally used as-is and not regenerated by people
building git, so it is okay if an explicit "make po/git.pot" always
automatically regenerates it.  Depend on the magic FORCE target
instead of explicitly keeping track of dependencies.

This simplifies the makefile, in particular preparing for a moment
when $(LIB_H), which is part of $(LOCALIZED_C), can be computed on the
fly.

We still need a dependency on GENERATED_H, to force those files to be
built when regenerating git.pot.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 2320de5..cf0ccdf 100644
--- a/Makefile
+++ b/Makefile
@@ -2138,7 +2138,7 @@ LOCALIZED_SH += t/t0200/test.sh
 LOCALIZED_PERL += t/t0200/test.perl
 endif
 
-po/git.pot: $(LOCALIZED_C)
+po/git.pot: $(GENERATED_H) FORCE
 	$(QUIET_XGETTEXT)$(XGETTEXT) -o$@+ $(XGETTEXT_FLAGS_C) $(LOCALIZED_C)
 	$(QUIET_XGETTEXT)$(XGETTEXT) -o$@+ --join-existing $(XGETTEXT_FLAGS_SH) \
 		$(LOCALIZED_SH)
-- 

Re: [PATCH 1/2] Makefile: use "find" to determine static header dependencies

[Jeff King]

On Thu, Aug 21, 2014 at 07:48:18AM -0700, Jonathan Nieder wrote:

> Subject: i18n: treat "make pot" as an explicitly-invoked target
> 
> po/git.pot is normally used as-is and not regenerated by people
> building git, so it is okay if an explicit "make po/git.pot" always
> automatically regenerates it.  Depend on the magic FORCE target
> instead of explicitly keeping track of dependencies.
> 
> This simplifies the makefile, in particular preparing for a moment
> when $(LIB_H), which is part of $(LOCALIZED_C), can be computed on the
> fly.
> 
> We still need a dependency on GENERATED_H, to force those files to be
> built when regenerating git.pot.
> 
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

Yeah, this is way less gross than what I proposed, and I do not think it
hurts anything. We do still need to drop the use of ":=" in assigning
LOCALIZED_C, but I do not think there is any need for it in the first
place.

-Peff

[PATCH v2 0/3] dropping manually-maintained LIB_H

[Jeff King]

On Fri, Aug 22, 2014 at 12:12:36AM -0400, Jeff King wrote:

> > po/git.pot is normally used as-is and not regenerated by people
> > building git, so it is okay if an explicit "make po/git.pot" always
> > automatically regenerates it.  Depend on the magic FORCE target
> > instead of explicitly keeping track of dependencies.
> 
> Yeah, this is way less gross than what I proposed, and I do not think it
> hurts anything. We do still need to drop the use of ":=" in assigning
> LOCALIZED_C, but I do not think there is any need for it in the first
> place.

Here's a re-roll of my series on top of your patch. In addition to
rebasing, I also switched it to use $(FIND) in the shell snippet rather
than a bare "find".

I notice that for the ctags generation we actually try "git ls-tree"
first and then fall back to "find". I guess we could do that here, but I
do not think the speed improvement matters much. And I think the "find"
output is a little more conservative. If you are adding a new header
file but have not mentioned it to git yet, I think we would prefer to
err on the side of including it as a potential dependency.

  [1/3]: i18n: treat "make pot" as an explicitly-invoked target
  [2/3]: Makefile: use `find` to determine static header dependencies
  [3/3]: Makefile: drop CHECK_HEADER_DEPENDENCIES code

-Peff

[PATCH 1/3] i18n: treat "make pot" as an explicitly-invoked target

[Jeff King]

From: Jonathan Nieder <jrnieder@gmail.com>

po/git.pot is normally used as-is and not regenerated by people
building git, so it is okay if an explicit "make po/git.pot" always
automatically regenerates it.  Depend on the magic FORCE target
instead of explicitly keeping track of dependencies.

This simplifies the makefile, in particular preparing for a moment
when $(LIB_H), which is part of $(LOCALIZED_C), can be computed on the
fly. It also fixes a slight breakage in which changes to perl and shell
scripts did not trigger a rebuild of po/git.pot.

We still need a dependency on GENERATED_H, to force those files to be
built when regenerating git.pot.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
Mostly as you sent it, but I mentioned the missing script dependencies
in the commit message, too.

 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 2320de5..cf0ccdf 100644
--- a/Makefile
+++ b/Makefile
@@ -2138,7 +2138,7 @@ LOCALIZED_SH += t/t0200/test.sh
 LOCALIZED_PERL += t/t0200/test.perl
 endif
 
-po/git.pot: $(LOCALIZED_C)
+po/git.pot: $(GENERATED_H) FORCE
 	$(QUIET_XGETTEXT)$(XGETTEXT) -o$@+ $(XGETTEXT_FLAGS_C) $(LOCALIZED_C)
 	$(QUIET_XGETTEXT)$(XGETTEXT) -o$@+ --join-existing $(XGETTEXT_FLAGS_SH) \
 		$(LOCALIZED_SH)
-- 
2.1.0.346.ga0367b9

[PATCH 2/3] Makefile: use `find` to determine static header dependencies

[Jeff King]

Most modern platforms will use automatically computed header
dependencies to figure out when a C file needs rebuilt due
to a header changing. With old compilers, however, we
fallback to a static list of header files. If any of them
changes, we recompile everything. This is overly
conservative, but the best we can do on older platforms.

It is unfortunately easy for our static header list to grow
stale, as none of the regular developers make use of it.
Instead of trying to keep it up to date, let's invoke "find"
to generate the list dynamically.

Since we do not use the value $(LIB_H) unless either
COMPUTE_HEADER_DEPENDENCIES is turned on or the user is
building "po/git.pot" (where it comes in via $(LOCALIZED_C),
make is smart enough to not even run this "find" in most
cases. However, we do need to stop using the "immediate"
variable assignment ":=" for $(LOCALIZED_C). That's OK,
because it was not otherwise useful here.

Signed-off-by: Jeff King <peff@peff.net>
---
I cannot see any reason for the ":=", but maybe I am missing something.

 Makefile | 140 ++++-----------------------------------------------------------
 1 file changed, 8 insertions(+), 132 deletions(-)

diff --git a/Makefile b/Makefile
index cf0ccdf..f2b85c9 100644
--- a/Makefile
+++ b/Makefile
@@ -432,7 +432,6 @@ XDIFF_OBJS =
 VCSSVN_OBJS =
 GENERATED_H =
 EXTRA_CPPFLAGS =
-LIB_H =
 LIB_OBJS =
 PROGRAM_OBJS =
 PROGRAMS =
@@ -631,131 +630,11 @@ VCSSVN_LIB = vcs-svn/lib.a
 
 GENERATED_H += common-cmds.h
 
-LIB_H += advice.h
-LIB_H += archive.h
-LIB_H += argv-array.h
-LIB_H += attr.h
-LIB_H += bisect.h
-LIB_H += blob.h
-LIB_H += branch.h
-LIB_H += builtin.h
-LIB_H += bulk-checkin.h
-LIB_H += bundle.h
-LIB_H += cache-tree.h
-LIB_H += cache.h
-LIB_H += color.h
-LIB_H += column.h
-LIB_H += commit.h
-LIB_H += compat/bswap.h
-LIB_H += compat/mingw.h
-LIB_H += compat/obstack.h
-LIB_H += compat/poll/poll.h
-LIB_H += compat/precompose_utf8.h
-LIB_H += compat/terminal.h
-LIB_H += compat/win32/dirent.h
-LIB_H += compat/win32/pthread.h
-LIB_H += compat/win32/syslog.h
-LIB_H += connected.h
-LIB_H += convert.h
-LIB_H += credential.h
-LIB_H += csum-file.h
-LIB_H += decorate.h
-LIB_H += delta.h
-LIB_H += diff.h
-LIB_H += diffcore.h
-LIB_H += dir.h
-LIB_H += exec_cmd.h
-LIB_H += ewah/ewok.h
-LIB_H += ewah/ewok_rlw.h
-LIB_H += fetch-pack.h
-LIB_H += fmt-merge-msg.h
-LIB_H += fsck.h
-LIB_H += gettext.h
-LIB_H += git-compat-util.h
-LIB_H += gpg-interface.h
-LIB_H += graph.h
-LIB_H += grep.h
-LIB_H += hashmap.h
-LIB_H += help.h
-LIB_H += http.h
-LIB_H += kwset.h
-LIB_H += levenshtein.h
-LIB_H += line-log.h
-LIB_H += line-range.h
-LIB_H += list-objects.h
-LIB_H += ll-merge.h
-LIB_H += log-tree.h
-LIB_H += mailmap.h
-LIB_H += merge-blobs.h
-LIB_H += merge-recursive.h
-LIB_H += mergesort.h
-LIB_H += notes-cache.h
-LIB_H += notes-merge.h
-LIB_H += notes-utils.h
-LIB_H += notes.h
-LIB_H += object.h
-LIB_H += pack-objects.h
-LIB_H += pack-revindex.h
-LIB_H += pack.h
-LIB_H += pack-bitmap.h
-LIB_H += parse-options.h
-LIB_H += patch-ids.h
-LIB_H += pathspec.h
-LIB_H += pkt-line.h
-LIB_H += prio-queue.h
-LIB_H += progress.h
-LIB_H += prompt.h
-LIB_H += quote.h
-LIB_H += reachable.h
-LIB_H += reflog-walk.h
-LIB_H += refs.h
-LIB_H += remote.h
-LIB_H += rerere.h
-LIB_H += resolve-undo.h
-LIB_H += revision.h
-LIB_H += run-command.h
-LIB_H += send-pack.h
-LIB_H += sequencer.h
-LIB_H += sha1-array.h
-LIB_H += sha1-lookup.h
-LIB_H += shortlog.h
-LIB_H += sideband.h
-LIB_H += sigchain.h
-LIB_H += strbuf.h
-LIB_H += streaming.h
-LIB_H += string-list.h
-LIB_H += submodule.h
-LIB_H += tag.h
-LIB_H += tar.h
-LIB_H += thread-utils.h
-LIB_H += transport.h
-LIB_H += tree-walk.h
-LIB_H += tree.h
-LIB_H += unpack-trees.h
-LIB_H += unicode_width.h
-LIB_H += url.h
-LIB_H += urlmatch.h
-LIB_H += userdiff.h
-LIB_H += utf8.h
-LIB_H += varint.h
-LIB_H += vcs-svn/fast_export.h
-LIB_H += vcs-svn/line_buffer.h
-LIB_H += vcs-svn/repo_tree.h
-LIB_H += vcs-svn/sliding_window.h
-LIB_H += vcs-svn/svndiff.h
-LIB_H += vcs-svn/svndump.h
-LIB_H += walker.h
-LIB_H += wildmatch.h
-LIB_H += wt-status.h
-LIB_H += xdiff-interface.h
-LIB_H += xdiff/xdiff.h
-LIB_H += xdiff/xdiffi.h
-LIB_H += xdiff/xemit.h
-LIB_H += xdiff/xinclude.h
-LIB_H += xdiff/xmacros.h
-LIB_H += xdiff/xprepare.h
-LIB_H += xdiff/xtypes.h
-LIB_H += xdiff/xutils.h
+LIB_H = $(shell $(FIND) . \
+	-name .git -prune -o \
+	-name t -prune -o \
+	-name Documentation -prune -o \
+	-name '*.h' -print)
 
 LIB_OBJS += abspath.o
 LIB_OBJS += advice.o
@@ -1381,7 +1260,6 @@ ifdef NO_INET_PTON
 endif
 ifndef NO_UNIX_SOCKETS
 	LIB_OBJS += unix-socket.o
-	LIB_H += unix-socket.h
 	PROGRAM_OBJS += credential-cache.o
 	PROGRAM_OBJS += credential-cache--daemon.o
 endif
@@ -1405,12 +1283,10 @@ endif
 ifdef BLK_SHA1
 	SHA1_HEADER = "block-sha1/sha1.h"
 	LIB_OBJS += block-sha1/sha1.o
-	LIB_H += block-sha1/sha1.h
 else
 ifdef PPC_SHA1
 	SHA1_HEADER = "ppc/sha1.h"
 	LIB_OBJS += ppc/sha1.o ppc/sha1ppc.o
-	LIB_H += ppc/sha1.h
 else
 ifdef APPLE_COMMON_CRYPTO
 	COMPAT_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL
@@ -2128,9 +2004,9 @@ XGETTEXT_FLAGS_C = $(XGETTEXT_FLAGS) --language=C \
 XGETTEXT_FLAGS_SH = $(XGETTEXT_FLAGS) --language=Shell \
 	--keyword=gettextln --keyword=eval_gettextln
 XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --keyword=__ --language=Perl
-LOCALIZED_C := $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)
-LOCALIZED_SH := $(SCRIPT_SH)
-LOCALIZED_PERL := $(SCRIPT_PERL)
+LOCALIZED_C = $(C_OBJ:o=c) $(GENERATED_H)
+LOCALIZED_SH = $(SCRIPT_SH)
+LOCALIZED_PERL = $(SCRIPT_PERL)
 
 ifdef XGETTEXT_INCLUDE_TESTS
 LOCALIZED_C += t/t0200/test.c
-- 
2.1.0.346.ga0367b9

Re: [PATCH 2/3] Makefile: use `find` to determine static header dependencies

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Since we do not use the value $(LIB_H) unless either
> COMPUTE_HEADER_DEPENDENCIES is turned on or the user is
> building "po/git.pot" (where it comes in via $(LOCALIZED_C),
> make is smart enough to not even run this "find" in most
> cases. However, we do need to stop using the "immediate"
> variable assignment ":=" for $(LOCALIZED_C). That's OK,
> because it was not otherwise useful here.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I cannot see any reason for the ":=", but maybe I am missing something.

If the right-hand-side were something like $(shell find ...) that
was heavy-weight then it might have made sense, but I do not think
it is that.  It has stayed to be := ever since it was introduced by
cd5513a7 (i18n: Makefile: "pot" target to extract messages marked
for translation, 2011-02-22).

And now you use LIB_H only once ;-).

Also interestingly, I notice that it is very clear that it is not
"LIB_H" but "ANY_H" ;-)

>
>  Makefile | 140 ++++-----------------------------------------------------------
>  1 file changed, 8 insertions(+), 132 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index cf0ccdf..f2b85c9 100644
> --- a/Makefile
> +++ b/Makefile
> ...
> @@ -2128,9 +2004,9 @@ XGETTEXT_FLAGS_C = $(XGETTEXT_FLAGS) --language=C \
>  XGETTEXT_FLAGS_SH = $(XGETTEXT_FLAGS) --language=Shell \
>  	--keyword=gettextln --keyword=eval_gettextln
>  XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --keyword=__ --language=Perl
> -LOCALIZED_C := $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)
> -LOCALIZED_SH := $(SCRIPT_SH)
> -LOCALIZED_PERL := $(SCRIPT_PERL)
> +LOCALIZED_C = $(C_OBJ:o=c) $(GENERATED_H)
> +LOCALIZED_SH = $(SCRIPT_SH)
> +LOCALIZED_PERL = $(SCRIPT_PERL)
>  
>  ifdef XGETTEXT_INCLUDE_TESTS
>  LOCALIZED_C += t/t0200/test.c

Re: [PATCH 2/3] Makefile: use `find` to determine static header dependencies

[Jeff King]

On Mon, Aug 25, 2014 at 12:30:51PM -0700, Junio C Hamano wrote:

> Also interestingly, I notice that it is very clear that it is not
> "LIB_H" but "ANY_H" ;-)

Yeah, it has been that way for quite a while. I don't know if it is that
big a deal, but it would not be unreasonable to do a patch to rename on
top (I am not sure what the new one would be; ANY_H is probably OK).

-Peff

Re: [PATCH 2/3] Makefile: use `find` to determine static header dependencies

[Jonathan Nieder]

Jeff King wrote:

> -LOCALIZED_C := $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)
> +LOCALIZED_C = $(C_OBJ:o=c) $(GENERATED_H)

Why is LIB_H dropped here?  This would mean that po/git.pot stops
including strings from macros and static inline functions in headers
(e.g., in parse-options.h).

The rest looks good.

Thanks,
Jonathan

Re: [PATCH 2/3] Makefile: use `find` to determine static header dependencies

[Jeff King]

On Mon, Aug 25, 2014 at 12:46:41PM -0700, Jonathan Nieder wrote:

> Jeff King wrote:
> 
> > -LOCALIZED_C := $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)
> > +LOCALIZED_C = $(C_OBJ:o=c) $(GENERATED_H)
> 
> Why is LIB_H dropped here?  This would mean that po/git.pot stops
> including strings from macros and static inline functions in headers
> (e.g., in parse-options.h).

Ick, this is an accidental leftover from the earlier iteration of the
patch, which moved that part to a separate line (inside my gross
GIT_REAL_POT conditional). The extra line went away, but I forgot to add
$(LIB_H) back in here. Thanks for noticing.

Here's a revised version of the patch which fixes it (and I
double-checked to make sure it continues to not execute the find unless
you are doing a "make pot").

-- >8 --
Subject: Makefile: use `find` to determine static header dependencies

Most modern platforms will use automatically computed header
dependencies to figure out when a C file needs rebuilt due
to a header changing. With old compilers, however, we
fallback to a static list of header files. If any of them
changes, we recompile everything. This is overly
conservative, but the best we can do on older platforms.

It is unfortunately easy for our static header list to grow
stale, as none of the regular developers make use of it.
Instead of trying to keep it up to date, let's invoke "find"
to generate the list dynamically.

Since we do not use the value $(LIB_H) unless either
COMPUTE_HEADER_DEPENDENCIES is turned on or the user is
building "po/git.pot" (where it comes in via $(LOCALIZED_C),
make is smart enough to not even run this "find" in most
cases. However, we do need to stop using the "immediate"
variable assignment ":=" for $(LOCALIZED_C). That's OK,
because it was not otherwise useful here.

Signed-off-by: Jeff King <peff@peff.net>
---
 Makefile | 140 ++++-----------------------------------------------------------
 1 file changed, 8 insertions(+), 132 deletions(-)

diff --git a/Makefile b/Makefile
index cf0ccdf..a4fc440 100644
--- a/Makefile
+++ b/Makefile
@@ -432,7 +432,6 @@ XDIFF_OBJS =
 VCSSVN_OBJS =
 GENERATED_H =
 EXTRA_CPPFLAGS =
-LIB_H =
 LIB_OBJS =
 PROGRAM_OBJS =
 PROGRAMS =
@@ -631,131 +630,11 @@ VCSSVN_LIB = vcs-svn/lib.a
 
 GENERATED_H += common-cmds.h
 
-LIB_H += advice.h
-LIB_H += archive.h
-LIB_H += argv-array.h
-LIB_H += attr.h
-LIB_H += bisect.h
-LIB_H += blob.h
-LIB_H += branch.h
-LIB_H += builtin.h
-LIB_H += bulk-checkin.h
-LIB_H += bundle.h
-LIB_H += cache-tree.h
-LIB_H += cache.h
-LIB_H += color.h
-LIB_H += column.h
-LIB_H += commit.h
-LIB_H += compat/bswap.h
-LIB_H += compat/mingw.h
-LIB_H += compat/obstack.h
-LIB_H += compat/poll/poll.h
-LIB_H += compat/precompose_utf8.h
-LIB_H += compat/terminal.h
-LIB_H += compat/win32/dirent.h
-LIB_H += compat/win32/pthread.h
-LIB_H += compat/win32/syslog.h
-LIB_H += connected.h
-LIB_H += convert.h
-LIB_H += credential.h
-LIB_H += csum-file.h
-LIB_H += decorate.h
-LIB_H += delta.h
-LIB_H += diff.h
-LIB_H += diffcore.h
-LIB_H += dir.h
-LIB_H += exec_cmd.h
-LIB_H += ewah/ewok.h
-LIB_H += ewah/ewok_rlw.h
-LIB_H += fetch-pack.h
-LIB_H += fmt-merge-msg.h
-LIB_H += fsck.h
-LIB_H += gettext.h
-LIB_H += git-compat-util.h
-LIB_H += gpg-interface.h
-LIB_H += graph.h
-LIB_H += grep.h
-LIB_H += hashmap.h
-LIB_H += help.h
-LIB_H += http.h
-LIB_H += kwset.h
-LIB_H += levenshtein.h
-LIB_H += line-log.h
-LIB_H += line-range.h
-LIB_H += list-objects.h
-LIB_H += ll-merge.h
-LIB_H += log-tree.h
-LIB_H += mailmap.h
-LIB_H += merge-blobs.h
-LIB_H += merge-recursive.h
-LIB_H += mergesort.h
-LIB_H += notes-cache.h
-LIB_H += notes-merge.h
-LIB_H += notes-utils.h
-LIB_H += notes.h
-LIB_H += object.h
-LIB_H += pack-objects.h
-LIB_H += pack-revindex.h
-LIB_H += pack.h
-LIB_H += pack-bitmap.h
-LIB_H += parse-options.h
-LIB_H += patch-ids.h
-LIB_H += pathspec.h
-LIB_H += pkt-line.h
-LIB_H += prio-queue.h
-LIB_H += progress.h
-LIB_H += prompt.h
-LIB_H += quote.h
-LIB_H += reachable.h
-LIB_H += reflog-walk.h
-LIB_H += refs.h
-LIB_H += remote.h
-LIB_H += rerere.h
-LIB_H += resolve-undo.h
-LIB_H += revision.h
-LIB_H += run-command.h
-LIB_H += send-pack.h
-LIB_H += sequencer.h
-LIB_H += sha1-array.h
-LIB_H += sha1-lookup.h
-LIB_H += shortlog.h
-LIB_H += sideband.h
-LIB_H += sigchain.h
-LIB_H += strbuf.h
-LIB_H += streaming.h
-LIB_H += string-list.h
-LIB_H += submodule.h
-LIB_H += tag.h
-LIB_H += tar.h
-LIB_H += thread-utils.h
-LIB_H += transport.h
-LIB_H += tree-walk.h
-LIB_H += tree.h
-LIB_H += unpack-trees.h
-LIB_H += unicode_width.h
-LIB_H += url.h
-LIB_H += urlmatch.h
-LIB_H += userdiff.h
-LIB_H += utf8.h
-LIB_H += varint.h
-LIB_H += vcs-svn/fast_export.h
-LIB_H += vcs-svn/line_buffer.h
-LIB_H += vcs-svn/repo_tree.h
-LIB_H += vcs-svn/sliding_window.h
-LIB_H += vcs-svn/svndiff.h
-LIB_H += vcs-svn/svndump.h
-LIB_H += walker.h
-LIB_H += wildmatch.h
-LIB_H += wt-status.h
-LIB_H += xdiff-interface.h
-LIB_H += xdiff/xdiff.h
-LIB_H += xdiff/xdiffi.h
-LIB_H += xdiff/xemit.h
-LIB_H += xdiff/xinclude.h
-LIB_H += xdiff/xmacros.h
-LIB_H += xdiff/xprepare.h
-LIB_H += xdiff/xtypes.h
-LIB_H += xdiff/xutils.h
+LIB_H = $(shell $(FIND) . \
+	-name .git -prune -o \
+	-name t -prune -o \
+	-name Documentation -prune -o \
+	-name '*.h' -print)
 
 LIB_OBJS += abspath.o
 LIB_OBJS += advice.o
@@ -1381,7 +1260,6 @@ ifdef NO_INET_PTON
 endif
 ifndef NO_UNIX_SOCKETS
 	LIB_OBJS += unix-socket.o
-	LIB_H += unix-socket.h
 	PROGRAM_OBJS += credential-cache.o
 	PROGRAM_OBJS += credential-cache--daemon.o
 endif
@@ -1405,12 +1283,10 @@ endif
 ifdef BLK_SHA1
 	SHA1_HEADER = "block-sha1/sha1.h"
 	LIB_OBJS += block-sha1/sha1.o
-	LIB_H += block-sha1/sha1.h
 else
 ifdef PPC_SHA1
 	SHA1_HEADER = "ppc/sha1.h"
 	LIB_OBJS += ppc/sha1.o ppc/sha1ppc.o
-	LIB_H += ppc/sha1.h
 else
 ifdef APPLE_COMMON_CRYPTO
 	COMPAT_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL
@@ -2128,9 +2004,9 @@ XGETTEXT_FLAGS_C = $(XGETTEXT_FLAGS) --language=C \
 XGETTEXT_FLAGS_SH = $(XGETTEXT_FLAGS) --language=Shell \
 	--keyword=gettextln --keyword=eval_gettextln
 XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --keyword=__ --language=Perl
-LOCALIZED_C := $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)
-LOCALIZED_SH := $(SCRIPT_SH)
-LOCALIZED_PERL := $(SCRIPT_PERL)
+LOCALIZED_C = $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)
+LOCALIZED_SH = $(SCRIPT_SH)
+LOCALIZED_PERL = $(SCRIPT_PERL)
 
 ifdef XGETTEXT_INCLUDE_TESTS
 LOCALIZED_C += t/t0200/test.c
-- 
2.1.0.346.ga0367b9

Re: [PATCH 2/3] Makefile: use `find` to determine static header dependencies

[Jeff King]

On Mon, Aug 25, 2014 at 04:00:42PM -0400, Jeff King wrote:

> On Mon, Aug 25, 2014 at 12:46:41PM -0700, Jonathan Nieder wrote:
> 
> > Jeff King wrote:
> > 
> > > -LOCALIZED_C := $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)
> > > +LOCALIZED_C = $(C_OBJ:o=c) $(GENERATED_H)
> > 
> > Why is LIB_H dropped here?  This would mean that po/git.pot stops
> > including strings from macros and static inline functions in headers
> > (e.g., in parse-options.h).
> 
> Ick, this is an accidental leftover from the earlier iteration of the
> patch, which moved that part to a separate line (inside my gross
> GIT_REAL_POT conditional). The extra line went away, but I forgot to add
> $(LIB_H) back in here. Thanks for noticing.

As an aside, this makes an interesting case study for our git.git
workflow.

In some workflows, I would have made the original unacceptable patch,
you would have reviewed it, and then I would have made the followup
patch to adjust it, and you would have reviewed that. But it's quite
hard to see my mistake in just the followup patch; the fact that
$(LIB_H) was originally part of $(LOCALIZED_C) does not appear in that
hunk at all.

But in our workflow, we squash out the unacceptable diff, and you review
from scratch the movement from the original working state (assuming the
status quo was working :) ) to the newly proposed state. And there the
mistake is quite a bit more obvious.

Just an interesting observation.

-Peff

Re: [PATCH 2/3] Makefile: use `find` to determine static header dependencies

[Jonathan Nieder]

Jeff King wrote:

> It is unfortunately easy for our static header list to grow
> stale, as none of the regular developers make use of it.
> Instead of trying to keep it up to date, let's invoke "find"
> to generate the list dynamically.

Yep, I like this.

It does mean that people who run "make pot" have to be a little more
vigilant about not keeping around extra .h files by mistake.  But I
trust them.

[...]
> +LIB_H = $(shell $(FIND) . \
> +	-name .git -prune -o \
> +	-name t -prune -o \
> +	-name Documentation -prune -o \
> +	-name '*.h' -print)

Tiny nit: I might use

	$(shell $(FIND) * \
		-name . -o
		-name '.*' -prune -o \
		-name t -prune -o \
		-name Documentation -prune -o \
		-name '*.h' -print)

or

	$(shell $(FIND) * \
		-name '.?*' -prune -o \
		-name t -prune -o \
		-name Documentation -prune -o \
		-name '*.h' -print)

to avoid spending time looking in other dot-directories like .svn,
.hg, or .snapshot.

With or without such a change,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH 2/3] Makefile: use `find` to determine static header dependencies

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Jeff King wrote:
>
>> It is unfortunately easy for our static header list to grow
>> stale, as none of the regular developers make use of it.
>> Instead of trying to keep it up to date, let's invoke "find"
>> to generate the list dynamically.
>
> Yep, I like this.
>
> It does mean that people who run "make pot" have to be a little more
> vigilant about not keeping around extra .h files by mistake.  But I
> trust them.
>
> [...]
>> +LIB_H = $(shell $(FIND) . \
>> +	-name .git -prune -o \
>> +	-name t -prune -o \
>> +	-name Documentation -prune -o \
>> +	-name '*.h' -print)
>
> Tiny nit: I might use
>
> 	$(shell $(FIND) * \
> 		-name . -o
> 		-name '.*' -prune -o \
> 		-name t -prune -o \
> 		-name Documentation -prune -o \
> 		-name '*.h' -print)
>
> or
>
> 	$(shell $(FIND) * \
> 		-name '.?*' -prune -o \
> 		-name t -prune -o \
> 		-name Documentation -prune -o \
> 		-name '*.h' -print)
>
> to avoid spending time looking in other dot-directories like .svn,
> .hg, or .snapshot.

Wouldn't it be sufficient to start digging not from "*" but from
"??*"?  That is, something like

	find ??* \( -name Documentation -o -name .\?\* \) -prune -o -name \*.h

;-)

> With or without such a change,
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

Re: [PATCH 2/3] Makefile: use `find` to determine static header dependencies

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> Tiny nit: I might use
>>
>> 	$(shell $(FIND) * \
>> 		-name . -o
>> 		-name '.*' -prune -o \
>> 		-name t -prune -o \
>> 		-name Documentation -prune -o \
>> 		-name '*.h' -print)
>>
>> or
>>
>> 	$(shell $(FIND) * \
>> 		-name '.?*' -prune -o \
>> 		-name t -prune -o \
>> 		-name Documentation -prune -o \
>> 		-name '*.h' -print)
>>
>> to avoid spending time looking in other dot-directories like .svn,
>> .hg, or .snapshot.
>
> Wouldn't it be sufficient to start digging not from "*" but from
> "??*"?

Gah, the * was supposed to be . in my examples (though it doesn't
hurt).

> 	find ??* \( -name Documentation -o -name .\?\* \) -prune -o -name \*.h

Heh.  Yeah, that would work. ;-)

Re: [PATCH 2/3] Makefile: use `find` to determine static header dependencies

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

>> Wouldn't it be sufficient to start digging not from "*" but from
>> "??*"?
>
> Gah, the * was supposed to be . in my examples (though it doesn't
> hurt).
>
>> 	find ??* \( -name Documentation -o -name .\?\* \) -prune -o -name \*.h
>
> Heh.  Yeah, that would work. ;-)

Continuing useless discussion...

Actually as you are not excluding CVS, RCS, etc., and using ??* as
the starting point will exclude .git, .hg, etc. at the top, I think
we can shorten it even further and say

	find ??* -name Documentation -prune -o -name \*.h

or something.

...and time to go back to something more serious and practical.

Don't we want to exclude contrib/ by the way?

Re: [PATCH 2/3] Makefile: use `find` to determine static header dependencies

[Jeff King]

On Mon, Aug 25, 2014 at 03:08:50PM -0700, Junio C Hamano wrote:

> Jonathan Nieder <jrnieder@gmail.com> writes:
> 
> >> Wouldn't it be sufficient to start digging not from "*" but from
> >> "??*"?
> >
> > Gah, the * was supposed to be . in my examples (though it doesn't
> > hurt).
> >
> >> 	find ??* \( -name Documentation -o -name .\?\* \) -prune -o -name \*.h
> >
> > Heh.  Yeah, that would work. ;-)
> 
> Continuing useless discussion...
> 
> Actually as you are not excluding CVS, RCS, etc., and using ??* as
> the starting point will exclude .git, .hg, etc. at the top, I think
> we can shorten it even further and say
> 
> 	find ??* -name Documentation -prune -o -name \*.h
> 
> or something.

I had originally considered starting with "find *", but I was worried
about shell globbing overflowing command-line limits here. "echo *" on a
built tree is about 12K. That's laughably small for Linux, but would
other systems (which, after all, are the main targets) be more picky?

POSIX lists 4K as the minimum, and that has to fit the environment, too.

I'd also be fine to try it and see if anybody on an antique system
complains.

> Don't we want to exclude contrib/ by the way?

Probably. For calculating dependencies, it is OK to be overly
conservative (the worst case is that we trigger a recompile if somebody
touched contrib/.../foo.h, which is rather unlikely).

For the .pot file, being conservative is a little annoying.  In theory
we might want to translate stuff in contrib/, but it probably is just
extra work for translators for not much benefit (though I have not
really used gettext; I assume it only pulls in strings marked with _()
and friends, so being conservative is maybe not that big a deal...).

In that sense, maybe we should just hit the whole tree to be on the
conservative side. The two reasons I did not in my initial attempt were:

  1. Performance. But with the final form, we only the run the `find` at
     all very rarely, so shaving off a few readdirs() is not that big a
     deal.

  2. There are a few problematic areas. t/perf may contain build trees
     which are copies of git, which I expect would confuse gettext.

So I dunno.

-Peff

Re: [PATCH 2/3] Makefile: use `find` to determine static header dependencies

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> Actually as you are not excluding CVS, RCS, etc., and using ??* as
>> the starting point will exclude .git, .hg, etc. at the top, I think
>> we can shorten it even further and say
>> 
>> 	find ??* -name Documentation -prune -o -name \*.h
>> 
>> or something.
>
> I had originally considered starting with "find *", but I was worried
> about shell globbing overflowing command-line limits here. "echo *" on a
> built tree is about 12K.

OK.  What I queued is still your original which is the most
conservative among various "fun" alternatives we have seen so far on
this thread, so we should be good ;-)

Thanks.

Re: [PATCH 2/3] Makefile: use `find` to determine static header dependencies

[Jeff King]

On Tue, Aug 26, 2014 at 09:54:19AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> >> Actually as you are not excluding CVS, RCS, etc., and using ??* as
> >> the starting point will exclude .git, .hg, etc. at the top, I think
> >> we can shorten it even further and say
> >> 
> >> 	find ??* -name Documentation -prune -o -name \*.h
> >> 
> >> or something.
> >
> > I had originally considered starting with "find *", but I was worried
> > about shell globbing overflowing command-line limits here. "echo *" on a
> > built tree is about 12K.
> 
> OK.  What I queued is still your original which is the most
> conservative among various "fun" alternatives we have seen so far on
> this thread, so we should be good ;-)

The only thing I think mine does not do that Jonathan suggested is
dropping .hg, etc. I do not know why anyone would track git in hg, but
it might make sense to s/.git/.?/ in what I sent.

(I noticed also that you did not queue the third patch to drop
CHECK_HEADER_DEPENDENCIES; I'm OK if we keep it, but I wanted to make
sure it was not an oversight).

-Peff

Re: [PATCH 2/3] Makefile: use `find` to determine static header dependencies

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Aug 26, 2014 at 09:54:19AM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> >> Actually as you are not excluding CVS, RCS, etc., and using ??* as
>> >> the starting point will exclude .git, .hg, etc. at the top, I think
>> >> we can shorten it even further and say
>> >> 
>> >> 	find ??* -name Documentation -prune -o -name \*.h
>> >> 
>> >> or something.
>> >
>> > I had originally considered starting with "find *", but I was worried
>> > about shell globbing overflowing command-line limits here. "echo *" on a
>> > built tree is about 12K.
>> 
>> OK.  What I queued is still your original which is the most
>> conservative among various "fun" alternatives we have seen so far on
>> this thread, so we should be good ;-)
>
> The only thing I think mine does not do that Jonathan suggested is
> dropping .hg, etc. I do not know why anyone would track git in hg, but
> it might make sense to s/.git/.?/ in what I sent.
>
> (I noticed also that you did not queue the third patch to drop
> CHECK_HEADER_DEPENDENCIES; I'm OK if we keep it, but I wanted to make
> sure it was not an oversight).

It started as "I just ran out of time to really think about it" and
transitioned to "Ahh, I forgot that I postponed deciding" ;-)

[PATCH 3/3] Makefile: drop CHECK_HEADER_DEPENDENCIES code

[Jeff King]

This code was useful when we kept a static list of header
files, and it was easy to forget to update it. Since the last
commit, we generate the list dynamically.

Technically this could still be used to find a dependency
that our dynamic check misses (e.g., a header file without a
".h" extension).  But that is reasonably unlikely to be
added, and even less likely to be noticed by this tool
(because it has to be run manually)., It is not worth
carrying around the cruft in the Makefile.

Signed-off-by: Jeff King <peff@peff.net>
---
Same as before.

 Makefile | 59 -----------------------------------------------------------
 1 file changed, 59 deletions(-)

diff --git a/Makefile b/Makefile
index f2b85c9..23e621f 100644
--- a/Makefile
+++ b/Makefile
@@ -317,9 +317,6 @@ all::
 # dependency rules.  The default is "auto", which means to use computed header
 # dependencies if your compiler is detected to support it.
 #
-# Define CHECK_HEADER_DEPENDENCIES to check for problems in the hard-coded
-# dependency rules.
-#
 # Define NATIVE_CRLF if your platform uses CRLF for line endings.
 #
 # Define XDL_FAST_HASH to use an alternative line-hashing method in
@@ -904,11 +901,6 @@ sysconfdir = etc
 endif
 endif
 
-ifdef CHECK_HEADER_DEPENDENCIES
-COMPUTE_HEADER_DEPENDENCIES = no
-USE_COMPUTED_HEADER_DEPENDENCIES =
-endif
-
 ifndef COMPUTE_HEADER_DEPENDENCIES
 COMPUTE_HEADER_DEPENDENCIES = auto
 endif
@@ -1809,29 +1801,13 @@ $(dep_dirs):
 missing_dep_dirs := $(filter-out $(wildcard $(dep_dirs)),$(dep_dirs))
 dep_file = $(dir $@).depend/$(notdir $@).d
 dep_args = -MF $(dep_file) -MQ $@ -MMD -MP
-ifdef CHECK_HEADER_DEPENDENCIES
-$(error cannot compute header dependencies outside a normal build. \
-Please unset CHECK_HEADER_DEPENDENCIES and try again)
-endif
 endif
 
 ifneq ($(COMPUTE_HEADER_DEPENDENCIES),yes)
-ifndef CHECK_HEADER_DEPENDENCIES
 dep_dirs =
 missing_dep_dirs =
 dep_args =
 endif
-endif
-
-ifdef CHECK_HEADER_DEPENDENCIES
-ifndef PRINT_HEADER_DEPENDENCIES
-missing_deps = $(filter-out $(notdir $^), \
-	$(notdir $(shell $(MAKE) -s $@ \
-		CHECK_HEADER_DEPENDENCIES=YesPlease \
-		USE_COMPUTED_HEADER_DEPENDENCIES=YesPlease \
-		PRINT_HEADER_DEPENDENCIES=YesPlease)))
-endif
-endif
 
 ASM_SRC := $(wildcard $(OBJECTS:o=S))
 ASM_OBJ := $(ASM_SRC:S=o)
@@ -1839,45 +1815,10 @@ C_OBJ := $(filter-out $(ASM_OBJ),$(OBJECTS))
 
 .SUFFIXES:
 
-ifdef PRINT_HEADER_DEPENDENCIES
-$(C_OBJ): %.o: %.c FORCE
-	echo $^
-$(ASM_OBJ): %.o: %.S FORCE
-	echo $^
-
-ifndef CHECK_HEADER_DEPENDENCIES
-$(error cannot print header dependencies during a normal build. \
-Please set CHECK_HEADER_DEPENDENCIES and try again)
-endif
-endif
-
-ifndef PRINT_HEADER_DEPENDENCIES
-ifdef CHECK_HEADER_DEPENDENCIES
-$(C_OBJ): %.o: %.c $(dep_files) FORCE
-	@set -e; echo CHECK $@; \
-	missing_deps="$(missing_deps)"; \
-	if test "$$missing_deps"; \
-	then \
-		echo missing dependencies: $$missing_deps; \
-		false; \
-	fi
-$(ASM_OBJ): %.o: %.S $(dep_files) FORCE
-	@set -e; echo CHECK $@; \
-	missing_deps="$(missing_deps)"; \
-	if test "$$missing_deps"; \
-	then \
-		echo missing dependencies: $$missing_deps; \
-		false; \
-	fi
-endif
-endif
-
-ifndef CHECK_HEADER_DEPENDENCIES
 $(C_OBJ): %.o: %.c GIT-CFLAGS $(missing_dep_dirs)
 	$(QUIET_CC)$(CC) -o $*.o -c $(dep_args) $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<
 $(ASM_OBJ): %.o: %.S GIT-CFLAGS $(missing_dep_dirs)
 	$(QUIET_CC)$(CC) -o $*.o -c $(dep_args) $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<
-endif
 
 %.s: %.c GIT-CFLAGS FORCE
 	$(QUIET_CC)$(CC) -o $@ -S $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<
-- 
2.1.0.346.ga0367b9

Re: [PATCH 1/2] Makefile: use "find" to determine static header dependencies

[Jiang Xin]

2014-08-21 22:48 GMT+08:00 Jonathan Nieder <jrnieder@gmail.com>:
> Hi,
>
> Jeff King wrote:
>
>> However, we do always define $(LIB_H) as a dependency of
>> po/git.pot. Even though we do not actually try to build that
>> target, make will still evaluate the dependencies when
>> reading the Makefile, and expand the variable. This is not
>> ideal
>
> Would the following work?  The current dependencies for po/git.pot are
> not correct anyway --- they include LOCALIZED_C but not LOCALIZED_SH
> or LOCALIZED_PERL, so someone hacking on shell scripts and then trying
> 'make po/git.pot' could end up with the pot file not being
> regenerated.
>
> -- >8 --
> Subject: i18n: treat "make pot" as an explicitly-invoked target
>
> po/git.pot is normally used as-is and not regenerated by people
> building git, so it is okay if an explicit "make po/git.pot" always
> automatically regenerates it.  Depend on the magic FORCE target
> instead of explicitly keeping track of dependencies.
>

That's great, it's better than what I used to execute:

    rm po/git.pot; make pot


-- 
Jiang Xin

[PATCH 2/2] Makefile: drop CHECK_HEADER_DEPENDENCIES code

[Jeff King]

This code was useful when we kept a static list of header
files, and it was easy to forget to update it. Since the last
commit, we generate the list dynamically.

Technically this could still be used to find a dependency
that our dynamic check misses (e.g., a header file without a
".h" extension).  But that is reasonably unlikely to be
added, and even less likely to be noticed by this tool
(because it has to be run manually)., It is not worth
carrying around the cruft in the Makefile.

Signed-off-by: Jeff King <peff@peff.net>
---
I'm open to leaving this, as it's not hurting anything aside from
clutter, and it could possibly be used to cross-check the dynamic rule.
I just couldn't resist that all-deletion diffstat.

 Makefile | 59 -----------------------------------------------------------
 1 file changed, 59 deletions(-)

diff --git a/Makefile b/Makefile
index 08dd973..65ff772 100644
--- a/Makefile
+++ b/Makefile
@@ -317,9 +317,6 @@ all::
 # dependency rules.  The default is "auto", which means to use computed header
 # dependencies if your compiler is detected to support it.
 #
-# Define CHECK_HEADER_DEPENDENCIES to check for problems in the hard-coded
-# dependency rules.
-#
 # Define NATIVE_CRLF if your platform uses CRLF for line endings.
 #
 # Define XDL_FAST_HASH to use an alternative line-hashing method in
@@ -904,11 +901,6 @@ sysconfdir = etc
 endif
 endif
 
-ifdef CHECK_HEADER_DEPENDENCIES
-COMPUTE_HEADER_DEPENDENCIES = no
-USE_COMPUTED_HEADER_DEPENDENCIES =
-endif
-
 ifndef COMPUTE_HEADER_DEPENDENCIES
 COMPUTE_HEADER_DEPENDENCIES = auto
 endif
@@ -1809,29 +1801,13 @@ $(dep_dirs):
 missing_dep_dirs := $(filter-out $(wildcard $(dep_dirs)),$(dep_dirs))
 dep_file = $(dir $@).depend/$(notdir $@).d
 dep_args = -MF $(dep_file) -MQ $@ -MMD -MP
-ifdef CHECK_HEADER_DEPENDENCIES
-$(error cannot compute header dependencies outside a normal build. \
-Please unset CHECK_HEADER_DEPENDENCIES and try again)
-endif
 endif
 
 ifneq ($(COMPUTE_HEADER_DEPENDENCIES),yes)
-ifndef CHECK_HEADER_DEPENDENCIES
 dep_dirs =
 missing_dep_dirs =
 dep_args =
 endif
-endif
-
-ifdef CHECK_HEADER_DEPENDENCIES
-ifndef PRINT_HEADER_DEPENDENCIES
-missing_deps = $(filter-out $(notdir $^), \
-	$(notdir $(shell $(MAKE) -s $@ \
-		CHECK_HEADER_DEPENDENCIES=YesPlease \
-		USE_COMPUTED_HEADER_DEPENDENCIES=YesPlease \
-		PRINT_HEADER_DEPENDENCIES=YesPlease)))
-endif
-endif
 
 ASM_SRC := $(wildcard $(OBJECTS:o=S))
 ASM_OBJ := $(ASM_SRC:S=o)
@@ -1839,45 +1815,10 @@ C_OBJ := $(filter-out $(ASM_OBJ),$(OBJECTS))
 
 .SUFFIXES:
 
-ifdef PRINT_HEADER_DEPENDENCIES
-$(C_OBJ): %.o: %.c FORCE
-	echo $^
-$(ASM_OBJ): %.o: %.S FORCE
-	echo $^
-
-ifndef CHECK_HEADER_DEPENDENCIES
-$(error cannot print header dependencies during a normal build. \
-Please set CHECK_HEADER_DEPENDENCIES and try again)
-endif
-endif
-
-ifndef PRINT_HEADER_DEPENDENCIES
-ifdef CHECK_HEADER_DEPENDENCIES
-$(C_OBJ): %.o: %.c $(dep_files) FORCE
-	@set -e; echo CHECK $@; \
-	missing_deps="$(missing_deps)"; \
-	if test "$$missing_deps"; \
-	then \
-		echo missing dependencies: $$missing_deps; \
-		false; \
-	fi
-$(ASM_OBJ): %.o: %.S $(dep_files) FORCE
-	@set -e; echo CHECK $@; \
-	missing_deps="$(missing_deps)"; \
-	if test "$$missing_deps"; \
-	then \
-		echo missing dependencies: $$missing_deps; \
-		false; \
-	fi
-endif
-endif
-
-ifndef CHECK_HEADER_DEPENDENCIES
 $(C_OBJ): %.o: %.c GIT-CFLAGS $(missing_dep_dirs)
 	$(QUIET_CC)$(CC) -o $*.o -c $(dep_args) $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<
 $(ASM_OBJ): %.o: %.S GIT-CFLAGS $(missing_dep_dirs)
 	$(QUIET_CC)$(CC) -o $*.o -c $(dep_args) $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<
-endif
 
 %.s: %.c GIT-CFLAGS FORCE
 	$(QUIET_CC)$(CC) -o $@ -S $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<
-- 
2.1.0.346.ga0367b9

Re: [PATCH] Update hard-coded header dependencies

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> The fall-back rules used when compilers don't support the -MMD switch
> to generate makefile rules based on #includes have been out of date
> since v1.7.12.1~22^2~8 (move git_version_string into version.c,
> 2012-06-02).
>
> Checked with 'make CHECK_HEADER_DEPENDENCIES=yes'.
>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> Maybe it's worth switching to plain
>
> 	LIB_H += $(wildcard *.h)
>
> ?  People using ancient compilers that never change headers wouldn't
> be hurt, people using modern compilers that do change headers also
> wouldn't be hurt, and we could stop pretending to maintain an
> up-to-date list.

I agree that it is very tempting to declare that we do not manually
"maintain" the dependency list and force people without -MMD to
recompile whenever any unrelated header changes.  Especially that
this patch only works on the 'master' branch and upwards, and does
not even work on 'maint', let alone 1.9 or 1.8.5 maintenance tracks.

Let's consider the merit of that approach after 2.1 is out.  Thanks.

Re: [PATCH] Update hard-coded header dependencies

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> ?  People using ancient compilers that never change headers wouldn't
>> be hurt, people using modern compilers that do change headers also
>> wouldn't be hurt, and we could stop pretending to maintain an
>> up-to-date list.
>
> I agree that it is very tempting to declare that we do not manually
> "maintain" the dependency list and force people without -MMD to
> recompile whenever any unrelated header changes.  Especially that
> this patch only works on the 'master' branch and upwards, and does
> not even work on 'maint', let alone 1.9 or 1.8.5 maintenance tracks.
>
> Let's consider the merit of that approach after 2.1 is out.  Thanks.

Actually "upwards" is not even true; the 'next' branch already wants
e.g. trace.h to build credential-store.o, which is not needed for
the 'master' branch.