Yeah, I didn't touch anything with t1517, as I couldn't reproduce the problem here. I'm still a bit puzzled.
There is definitely a problem here, which is that WITH_BREAKING_CHANGES is not respected correctly from the config.mak inclusion. I think you already know most of this, but just to demonstrate the breakage:
1. A normal build is fine. If we delete whatchanged and rebuild it,
that works, and it is present in the commands list. $ make
[copious output] $ rm -f git-whatchanged
$ make git-whatchanged
BUILTIN git-whatchanged $ ./git --list-cmds=main | grep whatchanged
whatchanged 2. If we specify WITH_BREAKING_CHANGES on the command line, that is
used by the whole Makefile and everything works. We can't rebuild
the command (it is not even a target!) and it is not present in the
builtin commands list. $ make WITH_BREAKING_CHANGES=1
[copious output] $ rm -f git-whatchanged
$ make WITH_BREAKING_CHANGES=1 git-whatchanged
make: *** No rule to make target 'git-whatchanged'. Stop. $ ./git --list-cmds=main | grep whatchanged
[no output] 3. And now using config.mak, we _do_ still build it (because the
conditional around BUILT_INS comes earlier than the config.mak
inclusion), but it is not present in the commands list (because the
-D logic to pass to the program comes later). $ echo WITH_BREAKING_CHANGES=1 >>config.mak
$ make
[copious output] $ rm -f git-whatchanged
$ make git-whatchanged
BUILTIN git-whatchanged $ ./git --list-cmds=main | grep whatchanged
[no output]So we've half-respected it; we built the file (really the hardlink) but the code doesn't know its there. But the part that puzzles me is why t1517 would be unhappy with that. It uses --list-cmds=main to get the list of commands to check. So it will not know about whatchanged at all, and it doesn't care if the hardlink is there or not (whether from this bug, or from a previous build).
What would be catastrophic is going the _other_ way. If we failed to build but included it in the commands list, then t1517 would barf. But I can't see a way for that to happen.
So I do think there's a bug here that we should fix, but I'm just confused how it has any visible effects (at least for t1517; it would have triggered the alias problems in t0014 I think).
As for the solution:
I think this is much too early to include those files. Just as a concrete example, try this:
echo "CFLAGS = --break-the-build" >>config.mak
make
Before your patch, we'd use those CFLAGS and the build will immediately fail. But after, we do not respect it at all! We need those inclusions to come after we set up default values, so the last-one-wins behavior can kick in. And many of those default values come after the BUILT_INS setup we care about.
I think the simplest solution is just to pull the "whatchanged" line out from the main BUILT_INS setup and handle it conditionally below. There's already precedence for that (e.g., the way we conditionally add http-fetch and http-push to PROGRAMS/PROGRAM_OBJS later on).
-Peff