Re: [PATCH] Makefile: read configuration earlier

On Wed, Jul 29, 2026 at 10:59:44PM +0000, brian m. carlson wrote:

> When building with WITH_BREAKING_CHANGES, we need that option set before
> we generate the list of binaries to build, since it affects whether
> git-whatchanged is built.  That in turn, affects whether t1517 passes,
> since it does not if we are in breaking-changes mode and git-whatchanged
> or git-pack-redundant exist.  Load the configuration settings earlier in
> the Makefile so that we properly honor this value when building.
> 
> Signed-off-by: brian m. carlson <[email protected]>
> ---
> I noticed that Peff's patches didn't quite fix the problem for me and I
> think we need this on top to make the tests pass properly.

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:

> --- a/Makefile
> +++ b/Makefile
> @@ -781,6 +781,10 @@ clean-perl-script:
>  clean-python-script:
>  	$(RM) $(SCRIPT_PYTHON_GEN)
>  
> +include config.mak.uname
> +-include config.mak.autogen
> +-include config.mak
> +

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