Failing tests with WITH_BREAKING_CHANGES

Failing tests with WITH_BREAKING_CHANGES

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 990 bytes --]

I have the following in `config.mak`:

----
DEVELOPER=1
CC=clang
GENERATE_COMPILATION_DATABASE=yes
WITH_RUST=1
USE_ASCIIDOCTOR=1
WITH_BREAKING_CHANGES=1
----

In this configuration, I've noticed some tests failing:

----
t0014-alias.sh                                   (Wstat: 256 (exited 1) Tests: 23 Failed: 2)
  Failed tests:  4, 8
  Non-zero exit status: 1
t1517-outside-repo.sh                            (Wstat: 256 (exited 1) Tests: 404 Failed: 2)
  Failed tests:  248-249
  Non-zero exit status: 1
----

These don't occur if I remove `WITH_BREAKING_CHANGES=1`, so they appear
to be related to that option.  However, I know we have a CI job for that
case, so it's unclear to me why these tests are failing; perhaps the CI
job is not testing what we think it's testing.

I noticed this because I plan to send out a series soon based on that
option and obviously I want to run the testsuite first.
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 325 bytes --]

Re: Failing tests with WITH_BREAKING_CHANGES

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> I have the following in `config.mak`:
>
> ----
> DEVELOPER=1
> CC=clang
> GENERATE_COMPILATION_DATABASE=yes
> WITH_RUST=1
> USE_ASCIIDOCTOR=1
> WITH_BREAKING_CHANGES=1
> ----
>
> In this configuration, I've noticed some tests failing:
>
> ----
> t0014-alias.sh                                   (Wstat: 256 (exited 1) Tests: 23 Failed: 2)
>   Failed tests:  4, 8
>   Non-zero exit status: 1
> t1517-outside-repo.sh                            (Wstat: 256 (exited 1) Tests: 404 Failed: 2)
>   Failed tests:  248-249
>   Non-zero exit status: 1
> ----
>
> These don't occur if I remove `WITH_BREAKING_CHANGES=1`, so they appear
> to be related to that option.  However, I know we have a CI job for that
> case, so it's unclear to me why these tests are failing; perhaps the CI
> job is not testing what we think it's testing.

Does not immediately ring a bell for me.  All four integration
branches are OK in my builds.

> I noticed this because I plan to send out a series soon based on that
> option and obviously I want to run the testsuite first.

Re: Failing tests with WITH_BREAKING_CHANGES

[Phillip Wood]

Hi brian

On 28/07/2026 01:46, brian m. carlson wrote:
> I have the following in `config.mak`:
> 
> ----
> DEVELOPER=1
> CC=clang
> GENERATE_COMPILATION_DATABASE=yes
> WITH_RUST=1
> USE_ASCIIDOCTOR=1
> WITH_BREAKING_CHANGES=1
> ----
> 
> In this configuration, I've noticed some tests failing:
> 
> ----
> t0014-alias.sh                                   (Wstat: 256 (exited 1) Tests: 23 Failed: 2)
>    Failed tests:  4, 8
>    Non-zero exit status: 1
> t1517-outside-repo.sh                            (Wstat: 256 (exited 1) Tests: 404 Failed: 2)
>    Failed tests:  248-249
>    Non-zero exit status: 1
> ----

I find t1517 fails quite often for me due to cruft from a previous build 
when a different branch was checked out. I wonder if there is a command 
that is no-longer built by WITH_BREAKING_CHANGES whose executable still 
exists in the build directory from a previous build. Its not clear to me 
why the alias tests might be failing though.

Thanks

Phillip

> These don't occur if I remove `WITH_BREAKING_CHANGES=1`, so they appear
> to be related to that option.  However, I know we have a CI job for that
> case, so it's unclear to me why these tests are failing; perhaps the CI
> job is not testing what we think it's testing.
> 
> I noticed this because I plan to send out a series soon based on that
> option and obviously I want to run the testsuite first.

Re: Failing tests with WITH_BREAKING_CHANGES

[Jeff King]

On Tue, Jul 28, 2026 at 02:31:03PM +0100, Phillip Wood wrote:

> I find t1517 fails quite often for me due to cruft from a previous build
> when a different branch was checked out. I wonder if there is a command that
> is no-longer built by WITH_BREAKING_CHANGES whose executable still exists in
> the build directory from a previous build. Its not clear to me why the alias
> tests might be failing though.

It's the same reason. We test looping through deprecated aliases using
whatchanged and pack-redundant. When those are builtin but deprecated
(like now) we allow aliases. After the breaking-changes split, those
names are not special at all, and they are subject to the usual alias
rules. If there is crufty git-whatchanged in your build directory, then
that is an "external command" unknown to Git and you are not allowed to
alias over it.

The test in t0014 that covers this should be removed after the breaking
changes actually land (those commands won't handled specially, so it's
not different than the normal alias loop detection).

But we are in a funny limbo now for WITH_BREAKING_CHANGES. Possibly we
could pull the value out of GIT-BUILD-OPTIONS (which I guess happens
already via the environment) and use a prereq to skip the test.

-Peff

[PATCH 0/2] fix serial tests without/with breaking-changes

[Jeff King]

On Tue, Jul 28, 2026 at 09:55:32AM -0400, Jeff King wrote:

> But we are in a funny limbo now for WITH_BREAKING_CHANGES. Possibly we
> could pull the value out of GIT-BUILD-OPTIONS (which I guess happens
> already via the environment) and use a prereq to skip the test.

I think we can do even a bit better. Leaving aside
WITH_BREAKING_CHANGES, we should consider what will eventually happen to
these tests when those deprecated commands go away. I think we want to
keep them in preparation for when we have more deprecated commands.

So here's a patch.

It doesn't address the t1517 issue at all. That test can also be
confused by older build products, but I don't think deprecation is
particularly related.  It comes from going to an old version where some
now-vanished command doesn't pass the "-h" test and then traveling back
to the present. Probably it could be more careful about what it
considers a valid command. Right now it checks "git --list-cmds", but in
theory it could be using a list generated from the Makefile.

  [1/2]: t0014: factor out choice of deprecated commands
  [2/2]: t0014: generate deprecated command names dynamically

 t/t0014-alias.sh | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

-Peff

[PATCH 1/2] t0014: factor out choice of deprecated commands

[Jeff King]

We have a few tests related to aliasing deprecated commands which use
"whatchanged" and "pack-redundant", as these are the only two deprecated
commands we have. Let's pull those names into variables so that we can
refactor the tests without relying on the specific names.

Signed-off-by: Jeff King <peff@peff.net>
---
I pulled this into its own patch because it's so noisy, but it could be
squashed with the subsequent one.

 t/t0014-alias.sh | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/t/t0014-alias.sh b/t/t0014-alias.sh
index 5144b0effd..9d7c737355 100755
--- a/t/t0014-alias.sh
+++ b/t/t0014-alias.sh
@@ -27,17 +27,20 @@ test_expect_success 'looping aliases - internal execution' '
 	test_grep "^fatal: alias loop detected: expansion of" output
 '
 
+deprecated1=whatchanged
+deprecated2=pack-redundant
+
 test_expect_success 'looping aliases - deprecated builtins' '
-	test_config alias.whatchanged pack-redundant &&
-	test_config alias.pack-redundant whatchanged &&
+	test_config alias.$deprecated1 $deprecated2 &&
+	test_config alias.$deprecated2 $deprecated1 &&
 	cat >expect <<-EOF &&
-	${SQ}whatchanged${SQ} is aliased to ${SQ}pack-redundant${SQ}
-	${SQ}pack-redundant${SQ} is aliased to ${SQ}whatchanged${SQ}
-	fatal: alias loop detected: expansion of ${SQ}whatchanged${SQ} does not terminate:
-	  whatchanged <==
-	  pack-redundant ==>
+	${SQ}$deprecated1${SQ} is aliased to ${SQ}$deprecated2${SQ}
+	${SQ}$deprecated2${SQ} is aliased to ${SQ}$deprecated1${SQ}
+	fatal: alias loop detected: expansion of ${SQ}$deprecated1${SQ} does not terminate:
+	  $deprecated1 <==
+	  $deprecated2 ==>
 	EOF
-	test_must_fail git whatchanged -h 2>actual &&
+	test_must_fail git $deprecated1 -h 2>actual &&
 	test_cmp expect actual
 '
 
@@ -90,8 +93,8 @@ test_expect_success 'can alias-shadow via two deprecated builtins' '
 	# some git(1) commands will fail... (see above)
 	test_might_fail git status -h >expect &&
 	test_file_not_empty expect &&
-	test_might_fail git -c alias.whatchanged=pack-redundant \
-		-c alias.pack-redundant=status whatchanged -h >actual &&
+	test_might_fail git -c alias.$deprecated1=$deprecated2 \
+		-c alias.$deprecated2=status $deprecated1 -h >actual &&
 	test_cmp expect actual
 '
 
-- 
2.55.0.749.g30c495c7a6

Re: [PATCH 1/2] t0014: factor out choice of deprecated commands

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> We have a few tests related to aliasing deprecated commands which use
> "whatchanged" and "pack-redundant", as these are the only two deprecated
> commands we have. Let's pull those names into variables so that we can
> refactor the tests without relying on the specific names.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I pulled this into its own patch because it's so noisy, but it could be
> squashed with the subsequent one.

The knee-jerk reaction I got after reading the above explanation
before the morning caffeine fully taking effect and without looking
at [2/2] is "we may have parameterized the exact command names, but
I cannot tell what value this change has, as the fact that we have
exactly two deprecated commands is still hardcoded in the test".

If the point of this change is that even if we ever deprecated a
third command, this test does not need to care about it, then I can
understand it is perfectly fine to have the hardcoded "this test
uses two deprecated commands" while parameterizing which two
commands are used.  But then the log message may be a bit
misleading.  I dunno.

I am very sure that I will be enlightened when I read [2/2], though
;-)

>  t/t0014-alias.sh | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/t/t0014-alias.sh b/t/t0014-alias.sh
> index 5144b0effd..9d7c737355 100755
> --- a/t/t0014-alias.sh
> +++ b/t/t0014-alias.sh
> @@ -27,17 +27,20 @@ test_expect_success 'looping aliases - internal execution' '
>  	test_grep "^fatal: alias loop detected: expansion of" output
>  '
>  
> +deprecated1=whatchanged
> +deprecated2=pack-redundant
> +
>  test_expect_success 'looping aliases - deprecated builtins' '
> -	test_config alias.whatchanged pack-redundant &&
> -	test_config alias.pack-redundant whatchanged &&
> +	test_config alias.$deprecated1 $deprecated2 &&
> +	test_config alias.$deprecated2 $deprecated1 &&
>  	cat >expect <<-EOF &&
> -	${SQ}whatchanged${SQ} is aliased to ${SQ}pack-redundant${SQ}
> -	${SQ}pack-redundant${SQ} is aliased to ${SQ}whatchanged${SQ}
> -	fatal: alias loop detected: expansion of ${SQ}whatchanged${SQ} does not terminate:
> -	  whatchanged <==
> -	  pack-redundant ==>
> +	${SQ}$deprecated1${SQ} is aliased to ${SQ}$deprecated2${SQ}
> +	${SQ}$deprecated2${SQ} is aliased to ${SQ}$deprecated1${SQ}
> +	fatal: alias loop detected: expansion of ${SQ}$deprecated1${SQ} does not terminate:
> +	  $deprecated1 <==
> +	  $deprecated2 ==>
>  	EOF
> -	test_must_fail git whatchanged -h 2>actual &&
> +	test_must_fail git $deprecated1 -h 2>actual &&
>  	test_cmp expect actual
>  '
>  
> @@ -90,8 +93,8 @@ test_expect_success 'can alias-shadow via two deprecated builtins' '
>  	# some git(1) commands will fail... (see above)
>  	test_might_fail git status -h >expect &&
>  	test_file_not_empty expect &&
> -	test_might_fail git -c alias.whatchanged=pack-redundant \
> -		-c alias.pack-redundant=status whatchanged -h >actual &&
> +	test_might_fail git -c alias.$deprecated1=$deprecated2 \
> +		-c alias.$deprecated2=status $deprecated1 -h >actual &&
>  	test_cmp expect actual
>  '

[PATCH 2/2] t0014: generate deprecated command names dynamically

[Jeff King]

We have a few tests related to aliasing of deprecated commands. They use
whatchanged and pack-redundant because those are the only two deprecated
commands we have. Eventually those commands will be removed, at which
point these tests will be checking nothing useful (they'll just be
regular aliases, which we already cover in other tests).

We could remove them at that point, but the code to handle deprecated
commands will still remain. We probably do want to keep the tests around
for the eventual day that we deprecate more commands. So let's ask Git
for its list of deprecated commands, and if we don't have any, skip
those tests.

This also prevents an annoying corner case when your build directory
contains old build products. Right now those commands are marked as
deprecated builtins and treated specially; we allow aliases and never
look for them as dashed external commands. But after they are removed,
they aren't special anymore. If your directory happens to contain
hardlinks from the build of an older version, that confuses Git: it sees
the old hardlinks in place, thinks those are actual external commands,
and refuses to allow aliasing.

You can see that today like this:

  make
  make WITH_BREAKING_CHANGES=1 test

The first "make" creates git-whatchanged as a hardlink to Git, and the
second does not clean it up (it doesn't know about the whatchanged
command at all anymore). t0014 fails because Git won't create an alias
to the "external" whatchanged command.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t0014-alias.sh | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/t/t0014-alias.sh b/t/t0014-alias.sh
index 9d7c737355..cbc447b481 100755
--- a/t/t0014-alias.sh
+++ b/t/t0014-alias.sh
@@ -27,10 +27,15 @@ test_expect_success 'looping aliases - internal execution' '
 	test_grep "^fatal: alias loop detected: expansion of" output
 '
 
-deprecated1=whatchanged
-deprecated2=pack-redundant
+test_expect_success 'detect deprecated commands' '
+	git --list-cmds=deprecated >deprecated &&
+	if read deprecated1 && read deprecated2
+	then
+		test_set_prereq HAVE_DEPRECATED
+	fi <deprecated
+'
 
-test_expect_success 'looping aliases - deprecated builtins' '
+test_expect_success HAVE_DEPRECATED 'looping aliases - deprecated builtins' '
 	test_config alias.$deprecated1 $deprecated2 &&
 	test_config alias.$deprecated2 $deprecated1 &&
 	cat >expect <<-EOF &&
@@ -89,7 +94,7 @@ test_expect_success 'can alias-shadow deprecated builtins' '
 	done
 '
 
-test_expect_success 'can alias-shadow via two deprecated builtins' '
+test_expect_success HAVE_DEPRECATED 'can alias-shadow via two deprecated builtins' '
 	# some git(1) commands will fail... (see above)
 	test_might_fail git status -h >expect &&
 	test_file_not_empty expect &&
-- 
2.55.0.749.g30c495c7a6

Re: [PATCH 2/2] t0014: generate deprecated command names dynamically

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> We have a few tests related to aliasing of deprecated commands. They use
> whatchanged and pack-redundant because those are the only two deprecated
> commands we have. Eventually those commands will be removed, at which
> point these tests will be checking nothing useful (they'll just be
> regular aliases, which we already cover in other tests).
>
> We could remove them at that point, but the code to handle deprecated
> commands will still remain. We probably do want to keep the tests around
> for the eventual day that we deprecate more commands. So let's ask Git
> for its list of deprecated commands, and if we don't have any, skip
> those tests.

Ah, now I understand.  So HAVE_DEPRECATED prerequisite guards tests
that require at least two deprecated commands, so that we can test
cases with aliases that involve two commands among deprecated ones
referring to each other.  Obviously, with 0 or 1 deprecated commands,
there is no point to perform such tests.

Makes sense.

Thanks.

> diff --git a/t/t0014-alias.sh b/t/t0014-alias.sh
> index 9d7c737355..cbc447b481 100755
> --- a/t/t0014-alias.sh
> +++ b/t/t0014-alias.sh
> @@ -27,10 +27,15 @@ test_expect_success 'looping aliases - internal execution' '
>  	test_grep "^fatal: alias loop detected: expansion of" output
>  '
>  
> -deprecated1=whatchanged
> -deprecated2=pack-redundant
> +test_expect_success 'detect deprecated commands' '
> +	git --list-cmds=deprecated >deprecated &&
> +	if read deprecated1 && read deprecated2
> +	then
> +		test_set_prereq HAVE_DEPRECATED
> +	fi <deprecated
> +'
>  
> -test_expect_success 'looping aliases - deprecated builtins' '
> +test_expect_success HAVE_DEPRECATED 'looping aliases - deprecated builtins' '
>  	test_config alias.$deprecated1 $deprecated2 &&
>  	test_config alias.$deprecated2 $deprecated1 &&
>  	cat >expect <<-EOF &&
> @@ -89,7 +94,7 @@ test_expect_success 'can alias-shadow deprecated builtins' '
>  	done
>  '
>  
> -test_expect_success 'can alias-shadow via two deprecated builtins' '
> +test_expect_success HAVE_DEPRECATED 'can alias-shadow via two deprecated builtins' '
>  	# some git(1) commands will fail... (see above)
>  	test_might_fail git status -h >expect &&
>  	test_file_not_empty expect &&

Re: [PATCH 2/2] t0014: generate deprecated command names dynamically

[Jeff King]

On Tue, Jul 28, 2026 at 09:01:53AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > We have a few tests related to aliasing of deprecated commands. They use
> > whatchanged and pack-redundant because those are the only two deprecated
> > commands we have. Eventually those commands will be removed, at which
> > point these tests will be checking nothing useful (they'll just be
> > regular aliases, which we already cover in other tests).
> >
> > We could remove them at that point, but the code to handle deprecated
> > commands will still remain. We probably do want to keep the tests around
> > for the eventual day that we deprecate more commands. So let's ask Git
> > for its list of deprecated commands, and if we don't have any, skip
> > those tests.
> 
> Ah, now I understand.  So HAVE_DEPRECATED prerequisite guards tests
> that require at least two deprecated commands, so that we can test
> cases with aliases that involve two commands among deprecated ones
> referring to each other.  Obviously, with 0 or 1 deprecated commands,
> there is no point to perform such tests.

Yeah. Sorry, maybe splitting the two just made it more confusing (it was
really to make the diff a bit less heinous). I'm OK if you want to just
squash them together (using the commit message from the second).

I suspect we could _probably_ rewrite the "looping aliases" test to also
run when there's only 1 deprecated command (just looping on itself). But
since we have two now, and plan to have zero later, I don't know that
it's worth the effort of doing so.

-Peff

Re: [PATCH 2/2] t0014: generate deprecated command names dynamically

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1858 bytes --]

On 2026-07-28 at 14:38:45, Jeff King wrote:
> We have a few tests related to aliasing of deprecated commands. They use
> whatchanged and pack-redundant because those are the only two deprecated
> commands we have. Eventually those commands will be removed, at which
> point these tests will be checking nothing useful (they'll just be
> regular aliases, which we already cover in other tests).
> 
> We could remove them at that point, but the code to handle deprecated
> commands will still remain. We probably do want to keep the tests around
> for the eventual day that we deprecate more commands. So let's ask Git
> for its list of deprecated commands, and if we don't have any, skip
> those tests.
> 
> This also prevents an annoying corner case when your build directory
> contains old build products. Right now those commands are marked as
> deprecated builtins and treated specially; we allow aliases and never
> look for them as dashed external commands. But after they are removed,
> they aren't special anymore. If your directory happens to contain
> hardlinks from the build of an older version, that confuses Git: it sees
> the old hardlinks in place, thinks those are actual external commands,
> and refuses to allow aliasing.
> 
> You can see that today like this:
> 
>   make
>   make WITH_BREAKING_CHANGES=1 test
> 
> The first "make" creates git-whatchanged as a hardlink to Git, and the
> second does not clean it up (it doesn't know about the whatchanged
> command at all anymore). t0014 fails because Git won't create an alias
> to the "external" whatchanged command.

These patches look sensible.  I was planning to spend some time this
morning investigating more since I woke up early, but I appreciate you
sending some patches in to fix them.
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 325 bytes --]

Re: Failing tests with WITH_BREAKING_CHANGES

[Phillip Wood]

On 28/07/2026 14:55, Jeff King wrote:
> On Tue, Jul 28, 2026 at 02:31:03PM +0100, Phillip Wood wrote:
> 
>> I find t1517 fails quite often for me due to cruft from a previous build
>> when a different branch was checked out. I wonder if there is a command that
>> is no-longer built by WITH_BREAKING_CHANGES whose executable still exists in
>> the build directory from a previous build. Its not clear to me why the alias
>> tests might be failing though.
> 
> It's the same reason. We test looping through deprecated aliases using
> whatchanged and pack-redundant. When those are builtin but deprecated
> (like now) we allow aliases. After the breaking-changes split, those
> names are not special at all, and they are subject to the usual alias
> rules. If there is crufty git-whatchanged in your build directory, then
> that is an "external command" unknown to Git and you are not allowed to
> alias over it.

Oh, of course - thanks for explaining that. Thanks for fixing the tests 
as well, I've only skimmed them but they seemed to make sense.

Phillip

> The test in t0014 that covers this should be removed after the breaking
> changes actually land (those commands won't handled specially, so it's
> not different than the normal alias loop detection).
> 
> But we are in a funny limbo now for WITH_BREAKING_CHANGES. Possibly we
> could pull the value out of GIT-BUILD-OPTIONS (which I guess happens
> already via the environment) and use a prereq to skip the test.
> 
> -Peff
> 

[PATCH] Makefile: read configuration earlier

[brian m. carlson]

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 <sandals@crustytoothpaste.net>
---
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.

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

diff --git a/Makefile b/Makefile
index 98e995e4be..6bfa461aeb 100644
--- 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
+
 SCRIPTS = $(SCRIPT_SH_GEN) \
 	  $(SCRIPT_PERL_GEN) \
 	  $(SCRIPT_PYTHON_GEN) \
@@ -1050,10 +1054,6 @@ GIT-SPATCH-DEFINES: FORCE
 		echo "$$FLAGS" >GIT-SPATCH-DEFINES; \
             fi
 
-include config.mak.uname
--include config.mak.autogen
--include config.mak
-
 ifdef DEVELOPER
 include config.mak.dev
 endif

Re: [PATCH] Makefile: read configuration earlier

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> 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 <sandals@crustytoothpaste.net>
> ---
> 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.
>
>  Makefile | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

This is a scary patch because its correctness depends on what is
between lines 780-1050.  It turns out that this now lets config.mak*
to set quite a lot of symbols to affect the outcome:

 * PROGRAM_OBJS, BUILT_INS, TEST_BUILTIN_OBJS
 * WITH_BREAKING_CHANGES
 * SHELL_PATH
 * PERL_PATH
 * PYTHON_PATH
 * NO_RUST
 * DEBUG
 * uname_S?????
 * SPARSE_FLAGS
 * SPATCH_INCLUDE_FLAGS

Especially curious is that currently there is this bit:

	ifeq ($(uname_S),Windows)
	RUST_LIB_NAME = gitcore.lib
	else
	RUST_LIB_NAME = libgitcore.a
	endif

that comes WAY BEFORE config.mak.uname is included.  If the location
to include these files matter, then how could this bit have been
working?  I have no idea and since I have no access to Windows
development box so I wouldn't know.

> diff --git a/Makefile b/Makefile
> index 98e995e4be..6bfa461aeb 100644
> --- 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
> +
>  SCRIPTS = $(SCRIPT_SH_GEN) \
>  	  $(SCRIPT_PERL_GEN) \
>  	  $(SCRIPT_PYTHON_GEN) \
> @@ -1050,10 +1054,6 @@ GIT-SPATCH-DEFINES: FORCE
>  		echo "$$FLAGS" >GIT-SPATCH-DEFINES; \
>              fi
>  
> -include config.mak.uname
> --include config.mak.autogen
> --include config.mak
> -
>  ifdef DEVELOPER
>  include config.mak.dev
>  endif

Re: [PATCH] Makefile: read configuration earlier

[Jeff King]

On Wed, Jul 29, 2026 at 09:10:15PM -0700, Junio C Hamano wrote:

> This is a scary patch because its correctness depends on what is
> between lines 780-1050.  It turns out that this now lets config.mak*
> to set quite a lot of symbols to affect the outcome:
> 
>  * PROGRAM_OBJS, BUILT_INS, TEST_BUILTIN_OBJS
>  * WITH_BREAKING_CHANGES
>  * SHELL_PATH
>  * PERL_PATH
>  * PYTHON_PATH
>  * NO_RUST
>  * DEBUG
>  * uname_S?????
>  * SPARSE_FLAGS
>  * SPATCH_INCLUDE_FLAGS

Yes, though to some degree config.mak can already manipulate those after
the fact. There are other breakages, though (see the CFLAGS one I showed
elsewhere in the thread).

> Especially curious is that currently there is this bit:
> 
> 	ifeq ($(uname_S),Windows)
> 	RUST_LIB_NAME = gitcore.lib
> 	else
> 	RUST_LIB_NAME = libgitcore.a
> 	endif
> 
> that comes WAY BEFORE config.mak.uname is included.  If the location
> to include these files matter, then how could this bit have been
> working?  I have no idea and since I have no access to Windows
> development box so I wouldn't know.

Yeah, that seems totally wrong to me. Likewise this bit right above it:

  ifndef NO_RUST
  ifdef DEBUG
  RUST_BUILD_CONFIG = debug
  else
  RUST_BUILD_CONFIG = release
  endif

has the same problem brian is fixing for BREAKING_CHANGES. It will work
for "make NO_RUST=1", but not if you put NO_RUST into config.mak. That
said, I don't know why that NO_RUST check is there at all. It is not a
problem to set a flag that nobody looks at. So it may be a bug without a
visible effect. ;)

-Peff

Re: [PATCH] Makefile: read configuration earlier

[Jeff King]

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 <sandals@crustytoothpaste.net>
> ---
> 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