{"thread":{"id":"66075","subject":"Failing tests with WITH_BREAKING_CHANGES","startedAt":"2026-07-28T00:46:40Z","lastAt":"2026-07-30T11:57:47Z","messageCount":16,"participants":["brian m. carlson","Junio C Hamano","Phillip Wood","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"549105","messageId":"amf76F4wxlboLz_A@fruit.crustytoothpaste.net","threadId":"66075","inReplyTo":null,"subject":"Failing tests with WITH_BREAKING_CHANGES","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2026-07-28T00:46:33Z","receivedAt":"2026-07-28T00:46:40Z","isPatch":false,"body":"I have the following in `config.mak`:\n\n----\nDEVELOPER=1\nCC=clang\nGENERATE_COMPILATION_DATABASE=yes\nWITH_RUST=1\nUSE_ASCIIDOCTOR=1\nWITH_BREAKING_CHANGES=1\n----\n\nIn this configuration, I've noticed some tests failing:\n\n----\nt0014-alias.sh                                   (Wstat: 256 (exited 1) Tests: 23 Failed: 2)\n  Failed tests:  4, 8\n  Non-zero exit status: 1\nt1517-outside-repo.sh                            (Wstat: 256 (exited 1) Tests: 404 Failed: 2)\n  Failed tests:  248-249\n  Non-zero exit status: 1\n----\n\nThese don't occur if I remove `WITH_BREAKING_CHANGES=1`, so they appear\nto be related to that option.  However, I know we have a CI job for that\ncase, so it's unclear to me why these tests are failing; perhaps the CI\njob is not testing what we think it's testing.\n\nI noticed this because I plan to send out a series soon based on that\noption and obviously I want to run the testsuite first.\n-- \nbrian m. carlson (they/them)\nToronto, Ontario, CA\n"},{"id":"549106","messageId":"xmqqwlugexn3.fsf@gitster.g","threadId":"66075","inReplyTo":"amf76F4wxlboLz_A@fruit.crustytoothpaste.net","subject":"Re: Failing tests with WITH_BREAKING_CHANGES","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-28T01:00:32Z","receivedAt":"2026-07-28T01:00:34Z","isPatch":false,"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n> I have the following in `config.mak`:\n>\n> ----\n> DEVELOPER=1\n> CC=clang\n> GENERATE_COMPILATION_DATABASE=yes\n> WITH_RUST=1\n> USE_ASCIIDOCTOR=1\n> WITH_BREAKING_CHANGES=1\n> ----\n>\n> In this configuration, I've noticed some tests failing:\n>\n> ----\n> t0014-alias.sh                                   (Wstat: 256 (exited 1) Tests: 23 Failed: 2)\n>   Failed tests:  4, 8\n>   Non-zero exit status: 1\n> t1517-outside-repo.sh                            (Wstat: 256 (exited 1) Tests: 404 Failed: 2)\n>   Failed tests:  248-249\n>   Non-zero exit status: 1\n> ----\n>\n> These don't occur if I remove `WITH_BREAKING_CHANGES=1`, so they appear\n> to be related to that option.  However, I know we have a CI job for that\n> case, so it's unclear to me why these tests are failing; perhaps the CI\n> job is not testing what we think it's testing.\n\nDoes not immediately ring a bell for me.  All four integration\nbranches are OK in my builds.\n\n> I noticed this because I plan to send out a series soon based on that\n> option and obviously I want to run the testsuite first.\n"},{"id":"549125","messageId":"758dbec3-7657-4342-8b74-7e59cdf88b5e@gmail.com","threadId":"66075","inReplyTo":"amf76F4wxlboLz_A@fruit.crustytoothpaste.net","subject":"Re: Failing tests with WITH_BREAKING_CHANGES","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-28T13:31:03Z","receivedAt":"2026-07-28T13:31:09Z","isPatch":false,"body":"Hi brian\n\nOn 28/07/2026 01:46, brian m. carlson wrote:\n> I have the following in `config.mak`:\n> \n> ----\n> DEVELOPER=1\n> CC=clang\n> GENERATE_COMPILATION_DATABASE=yes\n> WITH_RUST=1\n> USE_ASCIIDOCTOR=1\n> WITH_BREAKING_CHANGES=1\n> ----\n> \n> In this configuration, I've noticed some tests failing:\n> \n> ----\n> t0014-alias.sh                                   (Wstat: 256 (exited 1) Tests: 23 Failed: 2)\n>    Failed tests:  4, 8\n>    Non-zero exit status: 1\n> t1517-outside-repo.sh                            (Wstat: 256 (exited 1) Tests: 404 Failed: 2)\n>    Failed tests:  248-249\n>    Non-zero exit status: 1\n> ----\n\nI find t1517 fails quite often for me due to cruft from a previous build \nwhen a different branch was checked out. I wonder if there is a command \nthat is no-longer built by WITH_BREAKING_CHANGES whose executable still \nexists in the build directory from a previous build. Its not clear to me \nwhy the alias tests might be failing though.\n\nThanks\n\nPhillip\n\n> These don't occur if I remove `WITH_BREAKING_CHANGES=1`, so they appear\n> to be related to that option.  However, I know we have a CI job for that\n> case, so it's unclear to me why these tests are failing; perhaps the CI\n> job is not testing what we think it's testing.\n> \n> I noticed this because I plan to send out a series soon based on that\n> option and obviously I want to run the testsuite first.\n\n"},{"id":"549130","messageId":"20260728135532.GA11894@coredump.intra.peff.net","threadId":"66075","inReplyTo":"758dbec3-7657-4342-8b74-7e59cdf88b5e@gmail.com","subject":"Re: Failing tests with WITH_BREAKING_CHANGES","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-07-28T13:55:32Z","receivedAt":"2026-07-28T13:55:40Z","isPatch":false,"body":"On Tue, Jul 28, 2026 at 02:31:03PM +0100, Phillip Wood wrote:\n\n> I find t1517 fails quite often for me due to cruft from a previous build\n> when a different branch was checked out. I wonder if there is a command that\n> is no-longer built by WITH_BREAKING_CHANGES whose executable still exists in\n> the build directory from a previous build. Its not clear to me why the alias\n> tests might be failing though.\n\nIt's the same reason. We test looping through deprecated aliases using\nwhatchanged and pack-redundant. When those are builtin but deprecated\n(like now) we allow aliases. After the breaking-changes split, those\nnames are not special at all, and they are subject to the usual alias\nrules. If there is crufty git-whatchanged in your build directory, then\nthat is an \"external command\" unknown to Git and you are not allowed to\nalias over it.\n\nThe test in t0014 that covers this should be removed after the breaking\nchanges actually land (those commands won't handled specially, so it's\nnot different than the normal alias loop detection).\n\nBut we are in a funny limbo now for WITH_BREAKING_CHANGES. Possibly we\ncould pull the value out of GIT-BUILD-OPTIONS (which I guess happens\nalready via the environment) and use a prereq to skip the test.\n\n-Peff\n"},{"id":"549132","messageId":"20260728143653.GB11894@coredump.intra.peff.net","threadId":"66075","inReplyTo":"20260728135532.GA11894@coredump.intra.peff.net","subject":"[PATCH 0/2] fix serial tests without/with breaking-changes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-07-28T14:36:53Z","receivedAt":"2026-07-28T14:36:55Z","isPatch":true,"body":"On Tue, Jul 28, 2026 at 09:55:32AM -0400, Jeff King wrote:\n\n> But we are in a funny limbo now for WITH_BREAKING_CHANGES. Possibly we\n> could pull the value out of GIT-BUILD-OPTIONS (which I guess happens\n> already via the environment) and use a prereq to skip the test.\n\nI think we can do even a bit better. Leaving aside\nWITH_BREAKING_CHANGES, we should consider what will eventually happen to\nthese tests when those deprecated commands go away. I think we want to\nkeep them in preparation for when we have more deprecated commands.\n\nSo here's a patch.\n\nIt doesn't address the t1517 issue at all. That test can also be\nconfused by older build products, but I don't think deprecation is\nparticularly related.  It comes from going to an old version where some\nnow-vanished command doesn't pass the \"-h\" test and then traveling back\nto the present. Probably it could be more careful about what it\nconsiders a valid command. Right now it checks \"git --list-cmds\", but in\ntheory it could be using a list generated from the Makefile.\n\n  [1/2]: t0014: factor out choice of deprecated commands\n  [2/2]: t0014: generate deprecated command names dynamically\n\n t/t0014-alias.sh | 32 ++++++++++++++++++++------------\n 1 file changed, 20 insertions(+), 12 deletions(-)\n\n-Peff\n"},{"id":"549133","messageId":"20260728143726.GA41686@coredump.intra.peff.net","threadId":"66075","inReplyTo":"20260728143653.GB11894@coredump.intra.peff.net","subject":"[PATCH 1/2] t0014: factor out choice of deprecated commands","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-07-28T14:37:26Z","receivedAt":"2026-07-28T14:37:28Z","isPatch":true,"body":"We have a few tests related to aliasing deprecated commands which use\n\"whatchanged\" and \"pack-redundant\", as these are the only two deprecated\ncommands we have. Let's pull those names into variables so that we can\nrefactor the tests without relying on the specific names.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI pulled this into its own patch because it's so noisy, but it could be\nsquashed with the subsequent one.\n\n t/t0014-alias.sh | 23 +++++++++++++----------\n 1 file changed, 13 insertions(+), 10 deletions(-)\n\ndiff --git a/t/t0014-alias.sh b/t/t0014-alias.sh\nindex 5144b0effd..9d7c737355 100755\n--- a/t/t0014-alias.sh\n+++ b/t/t0014-alias.sh\n@@ -27,17 +27,20 @@ test_expect_success 'looping aliases - internal execution' '\n \ttest_grep \"^fatal: alias loop detected: expansion of\" output\n '\n \n+deprecated1=whatchanged\n+deprecated2=pack-redundant\n+\n test_expect_success 'looping aliases - deprecated builtins' '\n-\ttest_config alias.whatchanged pack-redundant &&\n-\ttest_config alias.pack-redundant whatchanged &&\n+\ttest_config alias.$deprecated1 $deprecated2 &&\n+\ttest_config alias.$deprecated2 $deprecated1 &&\n \tcat >expect <<-EOF &&\n-\t${SQ}whatchanged${SQ} is aliased to ${SQ}pack-redundant${SQ}\n-\t${SQ}pack-redundant${SQ} is aliased to ${SQ}whatchanged${SQ}\n-\tfatal: alias loop detected: expansion of ${SQ}whatchanged${SQ} does not terminate:\n-\t  whatchanged <==\n-\t  pack-redundant ==>\n+\t${SQ}$deprecated1${SQ} is aliased to ${SQ}$deprecated2${SQ}\n+\t${SQ}$deprecated2${SQ} is aliased to ${SQ}$deprecated1${SQ}\n+\tfatal: alias loop detected: expansion of ${SQ}$deprecated1${SQ} does not terminate:\n+\t  $deprecated1 <==\n+\t  $deprecated2 ==>\n \tEOF\n-\ttest_must_fail git whatchanged -h 2>actual &&\n+\ttest_must_fail git $deprecated1 -h 2>actual &&\n \ttest_cmp expect actual\n '\n \n@@ -90,8 +93,8 @@ test_expect_success 'can alias-shadow via two deprecated builtins' '\n \t# some git(1) commands will fail... (see above)\n \ttest_might_fail git status -h >expect &&\n \ttest_file_not_empty expect &&\n-\ttest_might_fail git -c alias.whatchanged=pack-redundant \\\n-\t\t-c alias.pack-redundant=status whatchanged -h >actual &&\n+\ttest_might_fail git -c alias.$deprecated1=$deprecated2 \\\n+\t\t-c alias.$deprecated2=status $deprecated1 -h >actual &&\n \ttest_cmp expect actual\n '\n \n-- \n2.55.0.749.g30c495c7a6\n\n"},{"id":"549134","messageId":"20260728143845.GB41686@coredump.intra.peff.net","threadId":"66075","inReplyTo":"20260728143653.GB11894@coredump.intra.peff.net","subject":"[PATCH 2/2] t0014: generate deprecated command names dynamically","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-07-28T14:38:45Z","receivedAt":"2026-07-28T14:38:47Z","isPatch":true,"body":"We have a few tests related to aliasing of deprecated commands. They use\nwhatchanged and pack-redundant because those are the only two deprecated\ncommands we have. Eventually those commands will be removed, at which\npoint these tests will be checking nothing useful (they'll just be\nregular aliases, which we already cover in other tests).\n\nWe could remove them at that point, but the code to handle deprecated\ncommands will still remain. We probably do want to keep the tests around\nfor the eventual day that we deprecate more commands. So let's ask Git\nfor its list of deprecated commands, and if we don't have any, skip\nthose tests.\n\nThis also prevents an annoying corner case when your build directory\ncontains old build products. Right now those commands are marked as\ndeprecated builtins and treated specially; we allow aliases and never\nlook for them as dashed external commands. But after they are removed,\nthey aren't special anymore. If your directory happens to contain\nhardlinks from the build of an older version, that confuses Git: it sees\nthe old hardlinks in place, thinks those are actual external commands,\nand refuses to allow aliasing.\n\nYou can see that today like this:\n\n  make\n  make WITH_BREAKING_CHANGES=1 test\n\nThe first \"make\" creates git-whatchanged as a hardlink to Git, and the\nsecond does not clean it up (it doesn't know about the whatchanged\ncommand at all anymore). t0014 fails because Git won't create an alias\nto the \"external\" whatchanged command.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t0014-alias.sh | 13 +++++++++----\n 1 file changed, 9 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t0014-alias.sh b/t/t0014-alias.sh\nindex 9d7c737355..cbc447b481 100755\n--- a/t/t0014-alias.sh\n+++ b/t/t0014-alias.sh\n@@ -27,10 +27,15 @@ test_expect_success 'looping aliases - internal execution' '\n \ttest_grep \"^fatal: alias loop detected: expansion of\" output\n '\n \n-deprecated1=whatchanged\n-deprecated2=pack-redundant\n+test_expect_success 'detect deprecated commands' '\n+\tgit --list-cmds=deprecated >deprecated &&\n+\tif read deprecated1 && read deprecated2\n+\tthen\n+\t\ttest_set_prereq HAVE_DEPRECATED\n+\tfi <deprecated\n+'\n \n-test_expect_success 'looping aliases - deprecated builtins' '\n+test_expect_success HAVE_DEPRECATED 'looping aliases - deprecated builtins' '\n \ttest_config alias.$deprecated1 $deprecated2 &&\n \ttest_config alias.$deprecated2 $deprecated1 &&\n \tcat >expect <<-EOF &&\n@@ -89,7 +94,7 @@ test_expect_success 'can alias-shadow deprecated builtins' '\n \tdone\n '\n \n-test_expect_success 'can alias-shadow via two deprecated builtins' '\n+test_expect_success HAVE_DEPRECATED 'can alias-shadow via two deprecated builtins' '\n \t# some git(1) commands will fail... (see above)\n \ttest_might_fail git status -h >expect &&\n \ttest_file_not_empty expect &&\n-- \n2.55.0.749.g30c495c7a6\n"},{"id":"549147","messageId":"xmqqwlufds3w.fsf@gitster.g","threadId":"66075","inReplyTo":"20260728143726.GA41686@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] t0014: factor out choice of deprecated commands","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-28T15:57:39Z","receivedAt":"2026-07-28T15:57:42Z","isPatch":true,"body":"Jeff King <peff@peff.net> writes:\n\n> We have a few tests related to aliasing deprecated commands which use\n> \"whatchanged\" and \"pack-redundant\", as these are the only two deprecated\n> commands we have. Let's pull those names into variables so that we can\n> refactor the tests without relying on the specific names.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> I pulled this into its own patch because it's so noisy, but it could be\n> squashed with the subsequent one.\n\nThe knee-jerk reaction I got after reading the above explanation\nbefore the morning caffeine fully taking effect and without looking\nat [2/2] is \"we may have parameterized the exact command names, but\nI cannot tell what value this change has, as the fact that we have\nexactly two deprecated commands is still hardcoded in the test\".\n\nIf the point of this change is that even if we ever deprecated a\nthird command, this test does not need to care about it, then I can\nunderstand it is perfectly fine to have the hardcoded \"this test\nuses two deprecated commands\" while parameterizing which two\ncommands are used.  But then the log message may be a bit\nmisleading.  I dunno.\n\nI am very sure that I will be enlightened when I read [2/2], though\n;-)\n\n>  t/t0014-alias.sh | 23 +++++++++++++----------\n>  1 file changed, 13 insertions(+), 10 deletions(-)\n>\n> diff --git a/t/t0014-alias.sh b/t/t0014-alias.sh\n> index 5144b0effd..9d7c737355 100755\n> --- a/t/t0014-alias.sh\n> +++ b/t/t0014-alias.sh\n> @@ -27,17 +27,20 @@ test_expect_success 'looping aliases - internal execution' '\n>  \ttest_grep \"^fatal: alias loop detected: expansion of\" output\n>  '\n>  \n> +deprecated1=whatchanged\n> +deprecated2=pack-redundant\n> +\n>  test_expect_success 'looping aliases - deprecated builtins' '\n> -\ttest_config alias.whatchanged pack-redundant &&\n> -\ttest_config alias.pack-redundant whatchanged &&\n> +\ttest_config alias.$deprecated1 $deprecated2 &&\n> +\ttest_config alias.$deprecated2 $deprecated1 &&\n>  \tcat >expect <<-EOF &&\n> -\t${SQ}whatchanged${SQ} is aliased to ${SQ}pack-redundant${SQ}\n> -\t${SQ}pack-redundant${SQ} is aliased to ${SQ}whatchanged${SQ}\n> -\tfatal: alias loop detected: expansion of ${SQ}whatchanged${SQ} does not terminate:\n> -\t  whatchanged <==\n> -\t  pack-redundant ==>\n> +\t${SQ}$deprecated1${SQ} is aliased to ${SQ}$deprecated2${SQ}\n> +\t${SQ}$deprecated2${SQ} is aliased to ${SQ}$deprecated1${SQ}\n> +\tfatal: alias loop detected: expansion of ${SQ}$deprecated1${SQ} does not terminate:\n> +\t  $deprecated1 <==\n> +\t  $deprecated2 ==>\n>  \tEOF\n> -\ttest_must_fail git whatchanged -h 2>actual &&\n> +\ttest_must_fail git $deprecated1 -h 2>actual &&\n>  \ttest_cmp expect actual\n>  '\n>  \n> @@ -90,8 +93,8 @@ test_expect_success 'can alias-shadow via two deprecated builtins' '\n>  \t# some git(1) commands will fail... (see above)\n>  \ttest_might_fail git status -h >expect &&\n>  \ttest_file_not_empty expect &&\n> -\ttest_might_fail git -c alias.whatchanged=pack-redundant \\\n> -\t\t-c alias.pack-redundant=status whatchanged -h >actual &&\n> +\ttest_might_fail git -c alias.$deprecated1=$deprecated2 \\\n> +\t\t-c alias.$deprecated2=status $deprecated1 -h >actual &&\n>  \ttest_cmp expect actual\n>  '\n"},{"id":"549148","messageId":"xmqqse53drwu.fsf@gitster.g","threadId":"66075","inReplyTo":"20260728143845.GB41686@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] t0014: generate deprecated command names dynamically","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-28T16:01:53Z","receivedAt":"2026-07-28T16:01:56Z","isPatch":true,"body":"Jeff King <peff@peff.net> writes:\n\n> We have a few tests related to aliasing of deprecated commands. They use\n> whatchanged and pack-redundant because those are the only two deprecated\n> commands we have. Eventually those commands will be removed, at which\n> point these tests will be checking nothing useful (they'll just be\n> regular aliases, which we already cover in other tests).\n>\n> We could remove them at that point, but the code to handle deprecated\n> commands will still remain. We probably do want to keep the tests around\n> for the eventual day that we deprecate more commands. So let's ask Git\n> for its list of deprecated commands, and if we don't have any, skip\n> those tests.\n\nAh, now I understand.  So HAVE_DEPRECATED prerequisite guards tests\nthat require at least two deprecated commands, so that we can test\ncases with aliases that involve two commands among deprecated ones\nreferring to each other.  Obviously, with 0 or 1 deprecated commands,\nthere is no point to perform such tests.\n\nMakes sense.\n\nThanks.\n\n> diff --git a/t/t0014-alias.sh b/t/t0014-alias.sh\n> index 9d7c737355..cbc447b481 100755\n> --- a/t/t0014-alias.sh\n> +++ b/t/t0014-alias.sh\n> @@ -27,10 +27,15 @@ test_expect_success 'looping aliases - internal execution' '\n>  \ttest_grep \"^fatal: alias loop detected: expansion of\" output\n>  '\n>  \n> -deprecated1=whatchanged\n> -deprecated2=pack-redundant\n> +test_expect_success 'detect deprecated commands' '\n> +\tgit --list-cmds=deprecated >deprecated &&\n> +\tif read deprecated1 && read deprecated2\n> +\tthen\n> +\t\ttest_set_prereq HAVE_DEPRECATED\n> +\tfi <deprecated\n> +'\n>  \n> -test_expect_success 'looping aliases - deprecated builtins' '\n> +test_expect_success HAVE_DEPRECATED 'looping aliases - deprecated builtins' '\n>  \ttest_config alias.$deprecated1 $deprecated2 &&\n>  \ttest_config alias.$deprecated2 $deprecated1 &&\n>  \tcat >expect <<-EOF &&\n> @@ -89,7 +94,7 @@ test_expect_success 'can alias-shadow deprecated builtins' '\n>  \tdone\n>  '\n>  \n> -test_expect_success 'can alias-shadow via two deprecated builtins' '\n> +test_expect_success HAVE_DEPRECATED 'can alias-shadow via two deprecated builtins' '\n>  \t# some git(1) commands will fail... (see above)\n>  \ttest_might_fail git status -h >expect &&\n>  \ttest_file_not_empty expect &&\n"},{"id":"549149","messageId":"20260728161949.GD639637@coredump.intra.peff.net","threadId":"66075","inReplyTo":"xmqqse53drwu.fsf@gitster.g","subject":"Re: [PATCH 2/2] t0014: generate deprecated command names dynamically","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-07-28T16:19:49Z","receivedAt":"2026-07-28T16:19:51Z","isPatch":true,"body":"On Tue, Jul 28, 2026 at 09:01:53AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > We have a few tests related to aliasing of deprecated commands. They use\n> > whatchanged and pack-redundant because those are the only two deprecated\n> > commands we have. Eventually those commands will be removed, at which\n> > point these tests will be checking nothing useful (they'll just be\n> > regular aliases, which we already cover in other tests).\n> >\n> > We could remove them at that point, but the code to handle deprecated\n> > commands will still remain. We probably do want to keep the tests around\n> > for the eventual day that we deprecate more commands. So let's ask Git\n> > for its list of deprecated commands, and if we don't have any, skip\n> > those tests.\n> \n> Ah, now I understand.  So HAVE_DEPRECATED prerequisite guards tests\n> that require at least two deprecated commands, so that we can test\n> cases with aliases that involve two commands among deprecated ones\n> referring to each other.  Obviously, with 0 or 1 deprecated commands,\n> there is no point to perform such tests.\n\nYeah. Sorry, maybe splitting the two just made it more confusing (it was\nreally to make the diff a bit less heinous). I'm OK if you want to just\nsquash them together (using the commit message from the second).\n\nI suspect we could _probably_ rewrite the \"looping aliases\" test to also\nrun when there's only 1 deprecated command (just looping on itself). But\nsince we have two now, and plan to have zero later, I don't know that\nit's worth the effort of doing so.\n\n-Peff\n"},{"id":"549165","messageId":"amkbLnNCeeziWATm@fruit.crustytoothpaste.net","threadId":"66075","inReplyTo":"20260728143845.GB41686@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] t0014: generate deprecated command names dynamically","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2026-07-28T21:12:15Z","receivedAt":"2026-07-28T21:12:17Z","isPatch":true,"body":"On 2026-07-28 at 14:38:45, Jeff King wrote:\n> We have a few tests related to aliasing of deprecated commands. They use\n> whatchanged and pack-redundant because those are the only two deprecated\n> commands we have. Eventually those commands will be removed, at which\n> point these tests will be checking nothing useful (they'll just be\n> regular aliases, which we already cover in other tests).\n> \n> We could remove them at that point, but the code to handle deprecated\n> commands will still remain. We probably do want to keep the tests around\n> for the eventual day that we deprecate more commands. So let's ask Git\n> for its list of deprecated commands, and if we don't have any, skip\n> those tests.\n> \n> This also prevents an annoying corner case when your build directory\n> contains old build products. Right now those commands are marked as\n> deprecated builtins and treated specially; we allow aliases and never\n> look for them as dashed external commands. But after they are removed,\n> they aren't special anymore. If your directory happens to contain\n> hardlinks from the build of an older version, that confuses Git: it sees\n> the old hardlinks in place, thinks those are actual external commands,\n> and refuses to allow aliasing.\n> \n> You can see that today like this:\n> \n>   make\n>   make WITH_BREAKING_CHANGES=1 test\n> \n> The first \"make\" creates git-whatchanged as a hardlink to Git, and the\n> second does not clean it up (it doesn't know about the whatchanged\n> command at all anymore). t0014 fails because Git won't create an alias\n> to the \"external\" whatchanged command.\n\nThese patches look sensible.  I was planning to spend some time this\nmorning investigating more since I woke up early, but I appreciate you\nsending some patches in to fix them.\n-- \nbrian m. carlson (they/them)\nToronto, Ontario, CA\n"},{"id":"549214","messageId":"1ee46199-b895-4f5e-ba2b-030fb2e47852@gmail.com","threadId":"66075","inReplyTo":"20260728135532.GA11894@coredump.intra.peff.net","subject":"Re: Failing tests with WITH_BREAKING_CHANGES","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-29T15:25:42Z","receivedAt":"2026-07-29T15:25:45Z","isPatch":false,"body":"On 28/07/2026 14:55, Jeff King wrote:\n> On Tue, Jul 28, 2026 at 02:31:03PM +0100, Phillip Wood wrote:\n> \n>> I find t1517 fails quite often for me due to cruft from a previous build\n>> when a different branch was checked out. I wonder if there is a command that\n>> is no-longer built by WITH_BREAKING_CHANGES whose executable still exists in\n>> the build directory from a previous build. Its not clear to me why the alias\n>> tests might be failing though.\n> \n> It's the same reason. We test looping through deprecated aliases using\n> whatchanged and pack-redundant. When those are builtin but deprecated\n> (like now) we allow aliases. After the breaking-changes split, those\n> names are not special at all, and they are subject to the usual alias\n> rules. If there is crufty git-whatchanged in your build directory, then\n> that is an \"external command\" unknown to Git and you are not allowed to\n> alias over it.\n\nOh, of course - thanks for explaining that. Thanks for fixing the tests \nas well, I've only skimmed them but they seemed to make sense.\n\nPhillip\n\n> The test in t0014 that covers this should be removed after the breaking\n> changes actually land (those commands won't handled specially, so it's\n> not different than the normal alias loop detection).\n> \n> But we are in a funny limbo now for WITH_BREAKING_CHANGES. Possibly we\n> could pull the value out of GIT-BUILD-OPTIONS (which I guess happens\n> already via the environment) and use a prereq to skip the test.\n> \n> -Peff\n> \n\n"},{"id":"549244","messageId":"20260729225944.1364947-1-sandals@crustytoothpaste.net","threadId":"66075","inReplyTo":"20260728135532.GA11894@coredump.intra.peff.net","subject":"[PATCH] Makefile: read configuration earlier","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2026-07-29T22:59:44Z","receivedAt":"2026-07-29T23:09:10Z","isPatch":true,"body":"When building with WITH_BREAKING_CHANGES, we need that option set before\nwe generate the list of binaries to build, since it affects whether\ngit-whatchanged is built.  That in turn, affects whether t1517 passes,\nsince it does not if we are in breaking-changes mode and git-whatchanged\nor git-pack-redundant exist.  Load the configuration settings earlier in\nthe Makefile so that we properly honor this value when building.\n\nSigned-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n---\nI noticed that Peff's patches didn't quite fix the problem for me and I\nthink we need this on top to make the tests pass properly.\n\n Makefile | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 98e995e4be..6bfa461aeb 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -781,6 +781,10 @@ clean-perl-script:\n clean-python-script:\n \t$(RM) $(SCRIPT_PYTHON_GEN)\n \n+include config.mak.uname\n+-include config.mak.autogen\n+-include config.mak\n+\n SCRIPTS = $(SCRIPT_SH_GEN) \\\n \t  $(SCRIPT_PERL_GEN) \\\n \t  $(SCRIPT_PYTHON_GEN) \\\n@@ -1050,10 +1054,6 @@ GIT-SPATCH-DEFINES: FORCE\n \t\techo \"$$FLAGS\" >GIT-SPATCH-DEFINES; \\\n             fi\n \n-include config.mak.uname\n--include config.mak.autogen\n--include config.mak\n-\n ifdef DEVELOPER\n include config.mak.dev\n endif\n"},{"id":"549259","messageId":"xmqqh5lhm82g.fsf@gitster.g","threadId":"66075","inReplyTo":"20260729225944.1364947-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH] Makefile: read configuration earlier","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-30T04:10:15Z","receivedAt":"2026-07-30T04:10:18Z","isPatch":true,"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n> When building with WITH_BREAKING_CHANGES, we need that option set before\n> we generate the list of binaries to build, since it affects whether\n> git-whatchanged is built.  That in turn, affects whether t1517 passes,\n> since it does not if we are in breaking-changes mode and git-whatchanged\n> or git-pack-redundant exist.  Load the configuration settings earlier in\n> the Makefile so that we properly honor this value when building.\n>\n> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n> ---\n> I noticed that Peff's patches didn't quite fix the problem for me and I\n> think we need this on top to make the tests pass properly.\n>\n>  Makefile | 8 ++++----\n>  1 file changed, 4 insertions(+), 4 deletions(-)\n\nThis is a scary patch because its correctness depends on what is\nbetween lines 780-1050.  It turns out that this now lets config.mak*\nto set quite a lot of symbols to affect the outcome:\n\n * PROGRAM_OBJS, BUILT_INS, TEST_BUILTIN_OBJS\n * WITH_BREAKING_CHANGES\n * SHELL_PATH\n * PERL_PATH\n * PYTHON_PATH\n * NO_RUST\n * DEBUG\n * uname_S?????\n * SPARSE_FLAGS\n * SPATCH_INCLUDE_FLAGS\n\nEspecially curious is that currently there is this bit:\n\n\tifeq ($(uname_S),Windows)\n\tRUST_LIB_NAME = gitcore.lib\n\telse\n\tRUST_LIB_NAME = libgitcore.a\n\tendif\n\nthat comes WAY BEFORE config.mak.uname is included.  If the location\nto include these files matter, then how could this bit have been\nworking?  I have no idea and since I have no access to Windows\ndevelopment box so I wouldn't know.\n\n> diff --git a/Makefile b/Makefile\n> index 98e995e4be..6bfa461aeb 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -781,6 +781,10 @@ clean-perl-script:\n>  clean-python-script:\n>  \t$(RM) $(SCRIPT_PYTHON_GEN)\n>  \n> +include config.mak.uname\n> +-include config.mak.autogen\n> +-include config.mak\n> +\n>  SCRIPTS = $(SCRIPT_SH_GEN) \\\n>  \t  $(SCRIPT_PERL_GEN) \\\n>  \t  $(SCRIPT_PYTHON_GEN) \\\n> @@ -1050,10 +1054,6 @@ GIT-SPATCH-DEFINES: FORCE\n>  \t\techo \"$$FLAGS\" >GIT-SPATCH-DEFINES; \\\n>              fi\n>  \n> -include config.mak.uname\n> --include config.mak.autogen\n> --include config.mak\n> -\n>  ifdef DEVELOPER\n>  include config.mak.dev\n>  endif\n"},{"id":"549294","messageId":"20260730115425.GA1871609@coredump.intra.peff.net","threadId":"66075","inReplyTo":"20260729225944.1364947-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH] Makefile: read configuration earlier","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-07-30T11:54:25Z","receivedAt":"2026-07-30T11:54:32Z","isPatch":true,"body":"On Wed, Jul 29, 2026 at 10:59:44PM +0000, brian m. carlson wrote:\n\n> When building with WITH_BREAKING_CHANGES, we need that option set before\n> we generate the list of binaries to build, since it affects whether\n> git-whatchanged is built.  That in turn, affects whether t1517 passes,\n> since it does not if we are in breaking-changes mode and git-whatchanged\n> or git-pack-redundant exist.  Load the configuration settings earlier in\n> the Makefile so that we properly honor this value when building.\n> \n> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n> ---\n> I noticed that Peff's patches didn't quite fix the problem for me and I\n> think we need this on top to make the tests pass properly.\n\nYeah, I didn't touch anything with t1517, as I couldn't reproduce the\nproblem here. I'm still a bit puzzled.\n\nThere is definitely a problem here, which is that WITH_BREAKING_CHANGES\nis not respected correctly from the config.mak inclusion. I think you\nalready know most of this, but just to demonstrate the breakage:\n\n  1. A normal build is fine. If we delete whatchanged and rebuild it,\n     that works, and it is present in the commands list.\n\n       $ make\n       [copious output]\n\n       $ rm -f git-whatchanged\n       $ make git-whatchanged\n           BUILTIN git-whatchanged\n\n       $ ./git --list-cmds=main | grep whatchanged\n       whatchanged\n\n  2. If we specify WITH_BREAKING_CHANGES on the command line, that is\n     used by the whole Makefile and everything works. We can't rebuild\n     the command (it is not even a target!) and it is not present in the\n     builtin commands list.\n\n       $ make WITH_BREAKING_CHANGES=1\n       [copious output]\n\n       $ rm -f git-whatchanged\n       $ make WITH_BREAKING_CHANGES=1 git-whatchanged\n       make: *** No rule to make target 'git-whatchanged'.  Stop.\n\n       $ ./git --list-cmds=main | grep whatchanged\n       [no output]\n\n  3. And now using config.mak, we _do_ still build it (because the\n     conditional around BUILT_INS comes earlier than the config.mak\n     inclusion), but it is not present in the commands list (because the\n     -D logic to pass to the program comes later).\n\n      $ echo WITH_BREAKING_CHANGES=1 >>config.mak\n      $ make\n      [copious output]\n\n      $ rm -f git-whatchanged\n      $ make git-whatchanged\n          BUILTIN git-whatchanged\n\n      $ ./git --list-cmds=main | grep whatchanged\n      [no output]\n\nSo we've half-respected it; we built the file (really the hardlink) but\nthe code doesn't know its there. But the part that puzzles me is why\nt1517 would be unhappy with that. It uses --list-cmds=main to get the\nlist of commands to check. So it will not know about whatchanged at all,\nand it doesn't care if the hardlink is there or not (whether from this\nbug, or from a previous build).\n\nWhat would be catastrophic is going the _other_ way. If we failed to\nbuild but included it in the commands list, then t1517 would barf.  But\nI can't see a way for that to happen.\n\nSo I do think there's a bug here that we should fix, but I'm just\nconfused how it has any visible effects (at least for t1517; it would\nhave triggered the alias problems in t0014 I think).\n\nAs for the solution:\n\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -781,6 +781,10 @@ clean-perl-script:\n>  clean-python-script:\n>  \t$(RM) $(SCRIPT_PYTHON_GEN)\n>  \n> +include config.mak.uname\n> +-include config.mak.autogen\n> +-include config.mak\n> +\n\nI think this is much too early to include those files. Just as a\nconcrete example, try this:\n\n  echo \"CFLAGS = --break-the-build\" >>config.mak\n  make\n\nBefore your patch, we'd use those CFLAGS and the build will immediately\nfail. But after, we do not respect it at all! We need those inclusions\nto come after we set up default values, so the last-one-wins behavior\ncan kick in. And many of those default values come after the BUILT_INS\nsetup we care about.\n\nI think the simplest solution is just to pull the \"whatchanged\" line out\nfrom the main BUILT_INS setup and handle it conditionally below. There's\nalready precedence for that (e.g., the way we conditionally add\nhttp-fetch and http-push to PROGRAMS/PROGRAM_OBJS later on).\n\n-Peff\n"},{"id":"549295","messageId":"20260730115745.GB1871609@coredump.intra.peff.net","threadId":"66075","inReplyTo":"xmqqh5lhm82g.fsf@gitster.g","subject":"Re: [PATCH] Makefile: read configuration earlier","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-07-30T11:57:45Z","receivedAt":"2026-07-30T11:57:47Z","isPatch":true,"body":"On Wed, Jul 29, 2026 at 09:10:15PM -0700, Junio C Hamano wrote:\n\n> This is a scary patch because its correctness depends on what is\n> between lines 780-1050.  It turns out that this now lets config.mak*\n> to set quite a lot of symbols to affect the outcome:\n> \n>  * PROGRAM_OBJS, BUILT_INS, TEST_BUILTIN_OBJS\n>  * WITH_BREAKING_CHANGES\n>  * SHELL_PATH\n>  * PERL_PATH\n>  * PYTHON_PATH\n>  * NO_RUST\n>  * DEBUG\n>  * uname_S?????\n>  * SPARSE_FLAGS\n>  * SPATCH_INCLUDE_FLAGS\n\nYes, though to some degree config.mak can already manipulate those after\nthe fact. There are other breakages, though (see the CFLAGS one I showed\nelsewhere in the thread).\n\n> Especially curious is that currently there is this bit:\n> \n> \tifeq ($(uname_S),Windows)\n> \tRUST_LIB_NAME = gitcore.lib\n> \telse\n> \tRUST_LIB_NAME = libgitcore.a\n> \tendif\n> \n> that comes WAY BEFORE config.mak.uname is included.  If the location\n> to include these files matter, then how could this bit have been\n> working?  I have no idea and since I have no access to Windows\n> development box so I wouldn't know.\n\nYeah, that seems totally wrong to me. Likewise this bit right above it:\n\n  ifndef NO_RUST\n  ifdef DEBUG\n  RUST_BUILD_CONFIG = debug\n  else\n  RUST_BUILD_CONFIG = release\n  endif\n\nhas the same problem brian is fixing for BREAKING_CHANGES. It will work\nfor \"make NO_RUST=1\", but not if you put NO_RUST into config.mak. That\nsaid, I don't know why that NO_RUST check is there at all. It is not a\nproblem to set a flag that nobody looks at. So it may be a bug without a\nvisible effect. ;)\n\n-Peff\n"}]}