[PATCH 0/4] Makefile fixes

[PATCH 0/4] Makefile fixes

[Jonathan Nieder]

Here are the aforementioned small fixes for the Makefile, intended for
maint.  I hope they are of some use.

Jonathan Nieder (4):
  Makefile: http-push.c uses the git headers
  Makefile: make ppc/sha1ppc.o depend on GIT-CFLAGS
  Makefile: fix .s pattern rule dependencies
  Makefile: stop cleaning arm directory

 Makefile |   12 +++++-------
 1 files changed, 5 insertions(+), 7 deletions(-)

[PATCH 1/4] Makefile: fix http-push.o dependencies

[Jonathan Nieder]

Since it is not in LIB_OBJS, http-push.o needs an explicit
$(GIT_H) dependency.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
http-push.c begins:

#include "cache.h"
#include "commit.h"

 Makefile |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index 856ba09..dc7c929 100644
--- a/Makefile
+++ b/Makefile
@@ -1557,9 +1557,7 @@ git-imap-send$X: imap-send.o $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
 		$(LIBS) $(OPENSSL_LINK) $(OPENSSL_LIBSSL)
 
-http.o http-walker.o http-push.o: http.h
-
-http.o http-walker.o: $(LIB_H)
+http.o http-walker.o http-push.o: http.h $(LIB_H)
 
 git-http-fetch$X: revision.o http.o http-walker.o http-fetch.o $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
-- 
1.6.5.3

[PATCH 2/4] Makefile: make ppc/sha1ppc.o depend on GIT-CFLAGS

[Jonathan Nieder]

Any rule that makes use of ALL_CFLAGS should depend on GIT-CFLAGS
to avoid trouble.  This one would not actually be affected by any
build flags except the optimization level, so leaving the
dependency out was mostly harmless.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Makefile |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index dc7c929..bb3879e 100644
--- a/Makefile
+++ b/Makefile
@@ -1526,7 +1526,7 @@ git.o git.spec \
 	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) $<
 %.s: %.c GIT-CFLAGS
 	$(QUIET_CC)$(CC) -S $(ALL_CFLAGS) $<
-%.o: %.S
+%.o: %.S GIT-CFLAGS
 	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) $<
 
 exec_cmd.o: exec_cmd.c GIT-CFLAGS
-- 
1.6.5.3

[PATCH 3/4] Makefile: fix .s pattern rule dependencies

[Jonathan Nieder]

'make git.s' fails to regenerate an assembler listing if git.c
has not changed but a header it includes has.  The %.s: %.c
pattern rule is meant to be invoked by hand, so it would be
better to make it always run.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
This adds yet another phony .FORCE-foo target.  Wouldn’t it be simpler
to use a single target called .FORCE, or is there something I am
missing that that would break?

 Makefile |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index bb3879e..cd210e3 100644
--- a/Makefile
+++ b/Makefile
@@ -1524,7 +1524,7 @@ git.o git.spec \
 
 %.o: %.c GIT-CFLAGS
 	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) $<
-%.s: %.c GIT-CFLAGS
+%.s: %.c GIT-CFLAGS .FORCE-LISTING
 	$(QUIET_CC)$(CC) -S $(ALL_CFLAGS) $<
 %.o: %.S GIT-CFLAGS
 	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) $<
@@ -1859,7 +1859,7 @@ endif
 .PHONY: all install clean strip
 .PHONY: shell_compatibility_test please_set_SHELL_PATH_to_a_more_modern_shell
 .PHONY: .FORCE-GIT-VERSION-FILE TAGS tags cscope .FORCE-GIT-CFLAGS
-.PHONY: .FORCE-GIT-BUILD-OPTIONS
+.PHONY: .FORCE-GIT-BUILD-OPTIONS .FORCE-LISTING
 
 ### Check documentation
 #
-- 
1.6.5.3

[PATCH 4/4] Makefile: do not clean arm directory

[Jonathan Nieder]

The ARM SHA-1 implementation was removed by commit 30ae47b
(remove ARM and Mozilla SHA1 implementations, 2009-08-17).  Prune
its directory from the list of object files to delete in 'make
clean'.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
One could argue that this should be left in, to allow 'make
clean' to clean up after the old version in an upgrade.  But that
way lies long rules for clean that never get tested for their
intended purpose.

 Makefile |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index cd210e3..47e1412 100644
--- a/Makefile
+++ b/Makefile
@@ -1833,7 +1833,7 @@ distclean: clean
 	$(RM) configure
 
 clean:
-	$(RM) *.o block-sha1/*.o arm/*.o ppc/*.o compat/*.o compat/*/*.o xdiff/*.o \
+	$(RM) *.o block-sha1/*.o ppc/*.o compat/*.o compat/*/*.o xdiff/*.o \
 		$(LIB_FILE) $(XDIFF_LIB)
 	$(RM) $(ALL_PROGRAMS) $(BUILT_INS) git$X
 	$(RM) $(TEST_PROGRAMS)
-- 
1.6.5.3

Re: [PATCH 1/4] Makefile: fix http-push.o dependencies

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Since it is not in LIB_OBJS, http-push.o needs an explicit
> $(GIT_H) dependency.

I think you meant $(LIB_H).

    $ make
    $ touch cache.h
    $ make http-push.o

does rebuild it with the current Makefile without this patch, because
it has this seemingly unrelated line (worse yet, this gives even more than
what are listed in LIB_H).

    $(patsubst git-%$X,%.o,$(PROGRAMS)) git.o: $(LIB_H) $(wildcard */*.h)

Puzzlingly messy.

Re: [PATCH 0/4] Makefile fixes

[Nanako Shiraishi]

Junio, could you tell us what happened to this thread?

Makefile improvements.  No discussion.

Re: [PATCH 0/4] Makefile fixes

[Junio C Hamano]

Nanako Shiraishi <nanako3@lavabit.com> writes:

> Junio, could you tell us what happened to this thread?
>
> Makefile improvements.  No discussion.

I took 4/4, and after looking at them again, I think 2/4 looks sensible,
too.

I was puzzled by 3/4 and I still am; the dependency rules are the same for
%.o and %.s yet the patch changes only %.s.  Either it leaves the same
breakage for %.o (which is much more important in practice), or the
problem Jonathan has with %.s may have other causes, but it was unclear to
me.

Re: [PATCH 0/4] Makefile fixes

[Jonathan Nieder]

Junio C Hamano wrote:
> Nanako Shiraishi <nanako3@lavabit.com> writes:
> 
>> Junio, could you tell us what happened to this thread?
>>
>> Makefile improvements.  No discussion.

These had some issues and instead of following up, I simply forgot
about them.

> I took 4/4, and after looking at them again, I think 2/4 looks sensible,
> too.

I also think the patch for 2/4 looks sensible, but the commit message
does not make much sense.  Optimization flags do not affect
compilation of assembler code as far as I can tell.  It would have
made more sense to say something like "Since the only .S file in git
does not have any #ifdefs, leaving the dependency out was mostly
harmless."  (Will resend.)

> I was puzzled by 3/4 and I still am; the dependency rules are the same for
> %.o and %.s yet the patch changes only %.s.  Either it leaves the same
> breakage for %.o (which is much more important in practice), or the
> problem Jonathan has with %.s may have other causes, but it was unclear to
> me.

The Makefile lists dependencies for each .o target elsewhere.  While
cleaning up those other dependency rules, I noticed there was nothing
analogous for the .s targets.  You can reproduce this by running
"make var.o var.s && touch cache.h && make var.o var.s".

Of course, I should have mentioned this in the commit message.  Will
resend as well.  Sorry to leave these standing for so long.

Sincerely,
Jonathan

[PATCH v2] Makefile: make ppc/sha1ppc.o depend on GIT-CFLAGS

[Jonathan Nieder]

The %.o: %.S pattern rule should depend on GIT-CFLAGS to avoid
trouble when ALL_CFLAGS changes.

The pattern only applies to one file (ppc/sha1ppc.S) and that
file does not use any #ifdefs, so leaving the dependency out is
probably harmless.  Nevertheless, it is safer to include the
dependency in case future code's behavior does depend on the
build flags.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Hopefully the justification is a little clearer this time.

This is not a high-priority change.  The problem it addresses is only
an aesthetic one as far as I can tell.  Still, I would be happy to see
it fixed; thanks for the reminder.

 Makefile |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index c11719c..015bfab 100644
--- a/Makefile
+++ b/Makefile
@@ -1635,7 +1635,7 @@ git.o git.spec \
 	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) $<
 %.s: %.c GIT-CFLAGS
 	$(QUIET_CC)$(CC) -S $(ALL_CFLAGS) $<
-%.o: %.S
+%.o: %.S GIT-CFLAGS
 	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) $<
 
 exec_cmd.o: exec_cmd.c GIT-CFLAGS
-- 
1.6.6.rc2

[PATCH v2 0/5] Makefile: fix generation of assembler listings

[Jonathan Nieder]

Jonathan Nieder wrote:

> This adds yet another phony .FORCE-foo target.  Wouldn’t it be simpler
> to use a single target called .FORCE, or is there something I am
> missing that that would break?

I didn’t hear any screams when I suggested this about a month ago, so
let’s try it out.

Patch 1 fixes a problem I noticed when tweaking the Makefile to
automatically generate dependencies for the %.o targets.  The problem
is that the dependencies for the corresponding %.s (code listing)
targets are not included in the Makefile at all, automatically or not.
Thus the command "make var.s var.o && touch cache.h && make var.s var.o"
produces the output

CC var.s
CC var.o
CC var.o

not regenerating var.s to reflect potential changes in cache.h.

"make git.s" previously did not work at all; patches 2-3 fix that.

Jonathan Nieder (5):
  Makefile: regenerate assembler listings when asked
  Makefile: use target-specific variable to pass flags to cc
  Makefile: learn to generate listings for targets requiring special
    flags
  Makefile: consolidate .FORCE-* targets
  git-gui/Makefile: consolidate .FORCE-* targets

 Documentation/Makefile |    4 +-
 Makefile               |   56 ++++++++++++++++++++---------------------------
 git-gui/Makefile       |    7 ++---
 3 files changed, 29 insertions(+), 38 deletions(-)

[PATCH 1/5] Makefile: regenerate assembler listings when asked

[Jonathan Nieder]

'make var.s' fails to regenerate an assembler listing if var.c
has not changed but a header it includes has:

	$ make var.s
	    CC var.s
	$ touch cache.h
	$ make var.s
	$

The corresponding problem for 'make var.o' does not occur because
the Makefile lists dependencies for each .o target explicitly;
analogous dependency rules for the .s targets are not present.
Rather than add some, it seems better to force 'make' to always
regenerate assembler listings, since the assembler listing
targets are only invoked when specifically requested on the make
command line.

Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Makefile |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index c11719c..ba4d071 100644
--- a/Makefile
+++ b/Makefile
@@ -1633,7 +1633,7 @@ git.o git.spec \
 
 %.o: %.c GIT-CFLAGS
 	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) $<
-%.s: %.c GIT-CFLAGS
+%.s: %.c GIT-CFLAGS .FORCE-LISTING
 	$(QUIET_CC)$(CC) -S $(ALL_CFLAGS) $<
 %.o: %.S
 	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) $<
@@ -1978,7 +1978,7 @@ endif
 .PHONY: all install clean strip
 .PHONY: shell_compatibility_test please_set_SHELL_PATH_to_a_more_modern_shell
 .PHONY: .FORCE-GIT-VERSION-FILE TAGS tags cscope .FORCE-GIT-CFLAGS
-.PHONY: .FORCE-GIT-BUILD-OPTIONS
+.PHONY: .FORCE-GIT-BUILD-OPTIONS .FORCE-LISTING
 
 ### Check documentation
 #
-- 
1.6.6.rc2

[PATCH 2/5] Makefile: use target-specific variable to pass flags to cc

[Jonathan Nieder]

This allows reusing the standard %.o: %.c pattern rule even for
targets that require special flags to be set.  Thus after this
change, any changes in the command for compilation only have to
be performed in one place.

Target-specific variables have been supported in GNU make since
version 3.77, which has been available since 1998.

Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Makefile |   41 ++++++++++++++++++-----------------------
 1 files changed, 18 insertions(+), 23 deletions(-)

diff --git a/Makefile b/Makefile
index ba4d071..81190a6 100644
--- a/Makefile
+++ b/Makefile
@@ -1467,20 +1467,19 @@ shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell
 strip: $(PROGRAMS) git$X
 	$(STRIP) $(STRIP_OPTS) $(PROGRAMS) git$X
 
-git.o: git.c common-cmds.h GIT-CFLAGS
-	$(QUIET_CC)$(CC) -DGIT_VERSION='"$(GIT_VERSION)"' \
-		'-DGIT_HTML_PATH="$(htmldir_SQ)"' \
-		$(ALL_CFLAGS) -o $@ -c $(filter %.c,$^)
+git.o: common-cmds.h
+git.o: ALL_CFLAGS += -DGIT_VERSION='"$(GIT_VERSION)"' \
+	'-DGIT_HTML_PATH="$(htmldir_SQ)"'
 
 git$X: git.o $(BUILTIN_OBJS) $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ git.o \
 		$(BUILTIN_OBJS) $(ALL_LDFLAGS) $(LIBS)
 
-builtin-help.o: builtin-help.c common-cmds.h GIT-CFLAGS
-	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) \
-		'-DGIT_HTML_PATH="$(htmldir_SQ)"' \
-		'-DGIT_MAN_PATH="$(mandir_SQ)"' \
-		'-DGIT_INFO_PATH="$(infodir_SQ)"' $<
+builtin-help.o: common-cmds.h
+builtin-help.o: ALL_CFLAGS += \
+	'-DGIT_HTML_PATH="$(htmldir_SQ)"' \
+	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
+	'-DGIT_INFO_PATH="$(infodir_SQ)"'
 
 $(BUILT_INS): git$X
 	$(QUIET_BUILT_IN)$(RM) $@ && \
@@ -1638,25 +1637,21 @@ git.o git.spec \
 %.o: %.S
 	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) $<
 
-exec_cmd.o: exec_cmd.c GIT-CFLAGS
-	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) \
-		'-DGIT_EXEC_PATH="$(gitexecdir_SQ)"' \
-		'-DBINDIR="$(bindir_relative_SQ)"' \
-		'-DPREFIX="$(prefix_SQ)"' \
-		$<
+exec_cmd.o: ALL_CFLAGS += \
+	'-DGIT_EXEC_PATH="$(gitexecdir_SQ)"' \
+	'-DBINDIR="$(bindir_relative_SQ)"' \
+	'-DPREFIX="$(prefix_SQ)"'
 
-builtin-init-db.o: builtin-init-db.c GIT-CFLAGS
-	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) -DDEFAULT_GIT_TEMPLATE_DIR='"$(template_dir_SQ)"' $<
+builtin-init-db.o: ALL_CFLAGS += \
+	-DDEFAULT_GIT_TEMPLATE_DIR='"$(template_dir_SQ)"'
 
-config.o: config.c GIT-CFLAGS
-	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) -DETC_GITCONFIG='"$(ETC_GITCONFIG_SQ)"' $<
+config.o: ALL_CFLAGS += -DETC_GITCONFIG='"$(ETC_GITCONFIG_SQ)"'
 
-http.o: http.c GIT-CFLAGS
-	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) -DGIT_USER_AGENT='"git/$(GIT_VERSION)"' $<
+http.o: ALL_CFLAGS += -DGIT_USER_AGENT='"git/$(GIT_VERSION)"'
 
 ifdef NO_EXPAT
-http-walker.o: http-walker.c http.h GIT-CFLAGS
-	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) -DNO_EXPAT $<
+http-walker.o: http.h
+http-walker.o: ALL_CFLAGS += -DNO_EXPAT
 endif
 
 git-%$X: %.o $(GITLIBS)
-- 
1.6.6.rc2

[PATCH 3/5] Makefile: learn to generate listings for targets requiring special flags

[Jonathan Nieder]

'make git.s' to debug code generation of main() fails because
git.c makes use of preprocessor symbols such as GIT_VERSION that
are not set.  make does not generate code listings for
builtin_help.c, exec_cmd.c, builtin-init-db.c, config.c, http.c,
or http-walker.c either, for the same reason.

So pass the flags used to generate each .o file when generating
the corresponding assembler listing.

Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Makefile |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/Makefile b/Makefile
index 81190a6..3d774c6 100644
--- a/Makefile
+++ b/Makefile
@@ -1468,7 +1468,7 @@ strip: $(PROGRAMS) git$X
 	$(STRIP) $(STRIP_OPTS) $(PROGRAMS) git$X
 
 git.o: common-cmds.h
-git.o: ALL_CFLAGS += -DGIT_VERSION='"$(GIT_VERSION)"' \
+git.s git.o: ALL_CFLAGS += -DGIT_VERSION='"$(GIT_VERSION)"' \
 	'-DGIT_HTML_PATH="$(htmldir_SQ)"'
 
 git$X: git.o $(BUILTIN_OBJS) $(GITLIBS)
@@ -1476,7 +1476,7 @@ git$X: git.o $(BUILTIN_OBJS) $(GITLIBS)
 		$(BUILTIN_OBJS) $(ALL_LDFLAGS) $(LIBS)
 
 builtin-help.o: common-cmds.h
-builtin-help.o: ALL_CFLAGS += \
+builtin-help.s builtin-help.o: ALL_CFLAGS += \
 	'-DGIT_HTML_PATH="$(htmldir_SQ)"' \
 	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
 	'-DGIT_INFO_PATH="$(infodir_SQ)"'
@@ -1637,21 +1637,21 @@ git.o git.spec \
 %.o: %.S
 	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) $<
 
-exec_cmd.o: ALL_CFLAGS += \
+exec_cmd.s exec_cmd.o: ALL_CFLAGS += \
 	'-DGIT_EXEC_PATH="$(gitexecdir_SQ)"' \
 	'-DBINDIR="$(bindir_relative_SQ)"' \
 	'-DPREFIX="$(prefix_SQ)"'
 
-builtin-init-db.o: ALL_CFLAGS += \
+builtin-init-db.s builtin-init-db.o: ALL_CFLAGS += \
 	-DDEFAULT_GIT_TEMPLATE_DIR='"$(template_dir_SQ)"'
 
-config.o: ALL_CFLAGS += -DETC_GITCONFIG='"$(ETC_GITCONFIG_SQ)"'
+config.s config.o: ALL_CFLAGS += -DETC_GITCONFIG='"$(ETC_GITCONFIG_SQ)"'
 
-http.o: ALL_CFLAGS += -DGIT_USER_AGENT='"git/$(GIT_VERSION)"'
+http.s http.o: ALL_CFLAGS += -DGIT_USER_AGENT='"git/$(GIT_VERSION)"'
 
 ifdef NO_EXPAT
 http-walker.o: http.h
-http-walker.o: ALL_CFLAGS += -DNO_EXPAT
+http-walker.s http-walker.o: ALL_CFLAGS += -DNO_EXPAT
 endif
 
 git-%$X: %.o $(GITLIBS)
-- 
1.6.6.rc2

[PATCH 4/5] Makefile: consolidate .FORCE-* targets

[Jonathan Nieder]

Providing multiple targets to force a rebuild is unnecessary
complication.

Avoid using a name that could conflict with future special
targets in GNU make (a leading period followed by uppercase
letters).

The corresponding change to the git-gui Makefile is left for
another patch.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Documentation/Makefile |    4 ++--
 Makefile               |   15 ++++++---------
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/Documentation/Makefile b/Documentation/Makefile
index 4797b2d..8a8a395 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -204,7 +204,7 @@ install-pdf: pdf
 install-html: html
 	'$(SHELL_PATH_SQ)' ./install-webdoc.sh $(DESTDIR)$(htmldir)
 
-../GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
+../GIT-VERSION-FILE: FORCE
 	$(QUIET_SUBDIR0)../ $(QUIET_SUBDIR1) GIT-VERSION-FILE
 
 -include ../GIT-VERSION-FILE
@@ -337,4 +337,4 @@ quick-install-man:
 quick-install-html:
 	'$(SHELL_PATH_SQ)' ./install-doc-quick.sh $(HTML_REF) $(DESTDIR)$(htmldir)
 
-.PHONY: .FORCE-GIT-VERSION-FILE
+.PHONY: FORCE
diff --git a/Makefile b/Makefile
index 3d774c6..fd79cd6 100644
--- a/Makefile
+++ b/Makefile
@@ -222,7 +222,7 @@ all::
 #   DEFAULT_EDITOR='$GIT_FALLBACK_EDITOR',
 #   DEFAULT_EDITOR='"C:\Program Files\Vim\gvim.exe" --nofork'
 
-GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
+GIT-VERSION-FILE: FORCE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
 -include GIT-VERSION-FILE
 
@@ -1632,7 +1632,7 @@ git.o git.spec \
 
 %.o: %.c GIT-CFLAGS
 	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) $<
-%.s: %.c GIT-CFLAGS .FORCE-LISTING
+%.s: %.c GIT-CFLAGS FORCE
 	$(QUIET_CC)$(CC) -S $(ALL_CFLAGS) $<
 %.o: %.S
 	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) $<
@@ -1723,7 +1723,7 @@ cscope:
 TRACK_CFLAGS = $(subst ','\'',$(ALL_CFLAGS)):\
              $(bindir_SQ):$(gitexecdir_SQ):$(template_dir_SQ):$(prefix_SQ)
 
-GIT-CFLAGS: .FORCE-GIT-CFLAGS
+GIT-CFLAGS: FORCE
 	@FLAGS='$(TRACK_CFLAGS)'; \
 	    if test x"$$FLAGS" != x"`cat GIT-CFLAGS 2>/dev/null`" ; then \
 		echo 1>&2 "    * new build flags or prefix"; \
@@ -1733,7 +1733,7 @@ GIT-CFLAGS: .FORCE-GIT-CFLAGS
 # We need to apply sq twice, once to protect from the shell
 # that runs GIT-BUILD-OPTIONS, and then again to protect it
 # and the first level quoting from the shell that runs "echo".
-GIT-BUILD-OPTIONS: .FORCE-GIT-BUILD-OPTIONS
+GIT-BUILD-OPTIONS: FORCE
 	@echo SHELL_PATH=\''$(subst ','\'',$(SHELL_PATH_SQ))'\' >$@
 	@echo PERL_PATH=\''$(subst ','\'',$(PERL_PATH_SQ))'\' >>$@
 	@echo TAR=\''$(subst ','\'',$(subst ','\'',$(TAR)))'\' >>$@
@@ -1745,14 +1745,12 @@ GIT-BUILD-OPTIONS: .FORCE-GIT-BUILD-OPTIONS
 ifndef NO_TCLTK
 TRACK_VARS = $(subst ','\'',-DTCLTK_PATH='$(TCLTK_PATH_SQ)')
 
-GIT-GUI-VARS: .FORCE-GIT-GUI-VARS
+GIT-GUI-VARS: FORCE
 	@VARS='$(TRACK_VARS)'; \
 	    if test x"$$VARS" != x"`cat $@ 2>/dev/null`" ; then \
 		echo 1>&2 "    * new Tcl/Tk interpreter location"; \
 		echo "$$VARS" >$@; \
             fi
-
-.PHONY: .FORCE-GIT-GUI-VARS
 endif
 
 ### Testing rules
@@ -1972,8 +1970,7 @@ endif
 
 .PHONY: all install clean strip
 .PHONY: shell_compatibility_test please_set_SHELL_PATH_to_a_more_modern_shell
-.PHONY: .FORCE-GIT-VERSION-FILE TAGS tags cscope .FORCE-GIT-CFLAGS
-.PHONY: .FORCE-GIT-BUILD-OPTIONS .FORCE-LISTING
+.PHONY: FORCE TAGS tags cscope
 
 ### Check documentation
 #
-- 
1.6.6.rc2

[PATCH git-gui 5/5] git-gui/Makefile: consolidate .FORCE-* targets

[Jonathan Nieder]

Providing multiple targets to force a rebuild is unnecessary
complication.

Avoid using a name that could conflict with future special
targets in GNU make (a leading period followed by uppercase
letters).

Cc: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
This is my first git-gui patch, so it is likely I have missed some
conventions.  If that is the case, please let me know.

Thanks,
Jonathn

 Makefile |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index b3580e9..197b55e 100644
--- a/Makefile
+++ b/Makefile
@@ -7,7 +7,7 @@ all::
 # TCL_PATH must be vaild for this to work.
 #
 
-GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
+GIT-VERSION-FILE: FORCE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
 -include GIT-VERSION-FILE
 
@@ -270,7 +270,7 @@ TRACK_VARS = \
 	GITGUI_MACOSXAPP=$(GITGUI_MACOSXAPP) \
 #end TRACK_VARS
 
-GIT-GUI-VARS: .FORCE-GIT-GUI-VARS
+GIT-GUI-VARS: FORCE
 	@VARS='$(TRACK_VARS)'; \
 	if test x"$$VARS" != x"`cat $@ 2>/dev/null`" ; then \
 		echo 1>&2 "    * new locations or Tcl/Tk interpreter"; \
@@ -340,5 +340,4 @@ ifdef GITGUI_WINDOWS_WRAPPER
 endif
 
 .PHONY: all install uninstall dist-version clean
-.PHONY: .FORCE-GIT-VERSION-FILE
-.PHONY: .FORCE-GIT-GUI-VARS
+.PHONY: FORCE
-- 
1.6.6.rc2

Re: [PATCH v2 0/5] Makefile: fix generation of assembler listings

[Linus Torvalds]

On Wed, 6 Jan 2010, Jonathan Nieder wrote:
> 
> Patch 1 fixes a problem I noticed when tweaking the Makefile to
> automatically generate dependencies for the %.o targets.  The problem
> is that the dependencies for the corresponding %.s (code listing)
> targets are not included in the Makefile at all, automatically or not.

Patches 1-3 (which were the ones I was cc'd on) look sane to me. Having 
real dependencies might be prettier, but I agree that since a *.s file is 
only generated on demand (and useful mainly to see code generation), just 
forcing the build makes sense.

		Linus

Re: [PATCH v2] Makefile: make ppc/sha1ppc.o depend on GIT-CFLAGS

[Nicolas Pitre]

On Wed, 6 Jan 2010, Jonathan Nieder wrote:

> The %.o: %.S pattern rule should depend on GIT-CFLAGS to avoid
> trouble when ALL_CFLAGS changes.
> 
> The pattern only applies to one file (ppc/sha1ppc.S) and that
> file does not use any #ifdefs, so leaving the dependency out is
> probably harmless.  Nevertheless, it is safer to include the
> dependency in case future code's behavior does depend on the
> build flags.

Is the PPC SHA1 code still needed?  Is it actually ever used?

We have block-sha1/* which is already about just as fast if not faster 
on most targets.  So I'd simply remove the ppc directory and its content 
which is the only remaining platform specific assembly code in the 
source tree.


Nicolas

Re: [PATCH git-gui 5/5] git-gui/Makefile: consolidate .FORCE-* targets

[Shawn O. Pearce]

Jonathan Nieder <jrnieder@gmail.com> wrote:
> Providing multiple targets to force a rebuild is unnecessary
> complication.
> 
> Avoid using a name that could conflict with future special
> targets in GNU make (a leading period followed by uppercase
> letters).
> 
> Cc: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks, applied.

-- 
Shawn.

Re: [PATCH 2/5] Makefile: use target-specific variable to pass flags to cc

[Jonathan Nieder]

Jonathan Nieder wrote:

> diff --git a/Makefile b/Makefile
> index ba4d071..81190a6 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1467,20 +1467,19 @@ shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell
>  strip: $(PROGRAMS) git$X
>  	$(STRIP) $(STRIP_OPTS) $(PROGRAMS) git$X
>  
> -git.o: git.c common-cmds.h GIT-CFLAGS
> -	$(QUIET_CC)$(CC) -DGIT_VERSION='"$(GIT_VERSION)"' \
> -		'-DGIT_HTML_PATH="$(htmldir_SQ)"' \
> -		$(ALL_CFLAGS) -o $@ -c $(filter %.c,$^)
> +git.o: common-cmds.h
> +git.o: ALL_CFLAGS += -DGIT_VERSION='"$(GIT_VERSION)"' \
> +	'-DGIT_HTML_PATH="$(htmldir_SQ)"'
>  
[...]

One annoying feature I wasn't thinking of: the values of
target-specific variables propagate to the dependencies of a target
(why? I can't imagine), and GIT-CFLAGS keeps on changing because of
this.

Maybe a new CMD_CFLAGS variable is needed for this, i.e. something
like the following squashed in.

diff --git a/Makefile b/Makefile
index 2580e23..d20e456 100644
--- a/Makefile
+++ b/Makefile
@@ -1468,7 +1468,7 @@ strip: $(PROGRAMS) git$X
 	$(STRIP) $(STRIP_OPTS) $(PROGRAMS) git$X
 
 git.o: common-cmds.h
-git.o: ALL_CFLAGS += -DGIT_VERSION='"$(GIT_VERSION)"' \
+git.o: CMD_CFLAGS += -DGIT_VERSION='"$(GIT_VERSION)"' \
 	'-DGIT_HTML_PATH="$(htmldir_SQ)"'
 
 git$X: git.o $(BUILTIN_OBJS) $(GITLIBS)
@@ -1476,7 +1476,7 @@ git$X: git.o $(BUILTIN_OBJS) $(GITLIBS)
 		$(BUILTIN_OBJS) $(ALL_LDFLAGS) $(LIBS)
 
 builtin-help.o: common-cmds.h
-builtin-help.o: ALL_CFLAGS += \
+builtin-help.o: CMD_CFLAGS += \
 	'-DGIT_HTML_PATH="$(htmldir_SQ)"' \
 	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
 	'-DGIT_INFO_PATH="$(infodir_SQ)"'
@@ -1630,28 +1630,31 @@ git.o git.spec \
 	$(patsubst %.perl,%,$(SCRIPT_PERL)) \
 	: GIT-VERSION-FILE
 
+# This can vary by target
+CMD_CFLAGS = $(ALL_CFLAGS)
+
 %.o: %.c GIT-CFLAGS
-	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) $<
+	$(QUIET_CC)$(CC) -o $*.o -c $(CMD_CFLAGS) $<
 %.s: %.c GIT-CFLAGS .FORCE-LISTING
-	$(QUIET_CC)$(CC) -S $(ALL_CFLAGS) $<
+	$(QUIET_CC)$(CC) -S $(CMD_CFLAGS) $<
 %.o: %.S GIT-CFLAGS
-	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) $<
+	$(QUIET_CC)$(CC) -o $*.o -c $(CMD_CFLAGS) $<
 
-exec_cmd.o: ALL_CFLAGS += \
+exec_cmd.o: CMD_CFLAGS += \
 	'-DGIT_EXEC_PATH="$(gitexecdir_SQ)"' \
 	'-DBINDIR="$(bindir_relative_SQ)"' \
 	'-DPREFIX="$(prefix_SQ)"'
 
-builtin-init-db.o: ALL_CFLAGS += \
+builtin-init-db.o: CMD_CFLAGS += \
 	-DDEFAULT_GIT_TEMPLATE_DIR='"$(template_dir_SQ)"'
 
-config.o: ALL_CFLAGS += -DETC_GITCONFIG='"$(ETC_GITCONFIG_SQ)"'
+config.o: CMD_CFLAGS += -DETC_GITCONFIG='"$(ETC_GITCONFIG_SQ)"'
 
-http.o: ALL_CFLAGS += -DGIT_USER_AGENT='"git/$(GIT_VERSION)"'
+http.o: CMD_CFLAGS += -DGIT_USER_AGENT='"git/$(GIT_VERSION)"'
 
 ifdef NO_EXPAT
 http-walker.o: http.h
-http-walker.o: ALL_CFLAGS += -DNO_EXPAT
+http-walker.o: CMD_CFLAGS += -DNO_EXPAT
 endif
 
 git-%$X: %.o $(GITLIBS)