Re: [PATCHv2 1/8] Makefile: apply dependencies consistently to sparse/asm targets

[Jonathan Nieder <jrnieder@gmail.com>]

Jeff King wrote:

>   1. The .sp and .s targets _do_ need the same -D macros that the .o
>      targets get.

Ah, you mean EXTRA_CPPFLAGS.  Yeah, that's also important, though the
patch doesn't have anything to do with it.

Some circuit in my mind missed that you meant EXTRA_CPPFLAGS and not a
file like GIT-CFLAGS.

[...]
>> It turns out that this patch is only about common-cmds.h, which was
>> the straightforward case.  Why not say so and save the reader from
>> having to think so hard? ;)
>
> Because I didn't realize it was the case at all. :) My intent was
> actually to clean up these lines so that they would be correct when I
> added GIT-VERSION-FILES and the like to them later.
>
> So I think my preference would be to tack on a note to the commit
> message saying "yeah, this doesn't do anything for meta-dependencies,
> but it doesn't hurt either". OK?

What is a meta-dependency?  I would find that even more confusing.

This change could be motivated more simply by saying that it prevents
"make git.sp", "make git.s", "make help.s", and "make builtin/help.s"
from failing when common-cmds.h doesn't exist yet, no?

The commit message could also say that it is improving consistency,
which is certainly valuable.

And a mention of EXTRA_CPPFLAGS and generated header files vs.
pre-existing header files could help explain that consistency.

But suggesting that we are supposed to ignore the FORCE just leaves
the reader wondering why the same patch does not also urgently need
to make additional changes such as the following, no?

	builtin/branch.o builtin/checkout.o builtin/clone.o \
	builtin/reset.o branch.o transport.o: branch.h

to

	builtin/branch.sp builtin/branch.o builtin/branch.s \
	builtin/checkout.sp builtin/checkout.o builtin/checkout.s \
	builtin/clone.sp builtin/clone.o builtin/clone.s \
	builtin/reset.sp builtin/reset.o builtin/reset.s \
	branch.sp branch.o branch.s \
	transport.sp transport.o transport.s: branch.h

Hoping that clarifies,
Jonathan