[PATCH v1 0/4] drop "name-rev --stdin" support

[PATCH v1 0/4] drop "name-rev --stdin" support

[Junio C Hamano]

[Administrivia]
    Yes, yes, I know I shouldn't be playing with shiny new toys
    during the pre-release freeze time.  But just to show others who
    may still be doing so that the first patch in this series has
    already be written to avoid duplicated and conflicting work, I
    am sending them out.  I have no intention to push the topic
    further before the final.

During Git 2.40 timeframe, we deprecated "--stdin" option of the
"name-rev" command in preference to "--annotate-stdin", and removed
the mention of it from the documentation.

Let's prepare for Git 3.0 to stop supporting it.

The real motive of these patches is not really about that option but
make sure we have, with WITH_BREAKING_CHANGES compilation knob,
enough support to keep preparing these changes.  The first patch
renames the WITHOUT_BREAKING_CHANGES prerequisite that unfortunately
invites double negations easily and changes existing users of it,
then two patches to a test script minimally modernizes it.  The last
step introduces the real change, guarded by WITH_BREAKING_CHANGES
compilation knob.

Junio C Hamano (4):
  t: introduce WITH_BREAKING_CHANGES prerequisite
  t6120: avoid hiding "git" exit status
  t6120: further modernize
  name-rev: remove "--stdin" support

 Documentation/BreakingChanges.adoc |  6 ++++++
 builtin/name-rev.c                 |  2 ++
 t/t5323-pack-redundant.sh          |  2 +-
 t/t5505-remote.sh                  |  6 +++---
 t/t5515-fetch-merge-logic.sh       |  2 +-
 t/t5516-fetch-push.sh              |  8 ++++----
 t/t6120-describe.sh                | 18 +++++++++++++-----
 t/test-lib.sh                      |  5 +++++
 8 files changed, 35 insertions(+), 14 deletions(-)

-- 
2.49.0-rc2-173-g4d16673c2b

[PATCH v1 1/4] t: introduce WITH_BREAKING_CHANGES prerequisite

[Junio C Hamano]

Earlier c5bc9a7f (Makefile: wire up build option for deprecated
features, 2025-01-22) made an unfortunate decision to introduce the
WITHOUT_BREAKING_CHANGES prerequisite to perform tests that ensure
the historical behaviour that may be different from what we will
have in the future.  It would inevitably invite doulbe negation when
we need to add tests to ensure the behaviour we want to have in the
future.

Introduce WITH_BREAKING_CHANGES prerequisite and replace the
existing uses of WITHOUT_BREAKING_CHANGES prerequisite.  Some
in-flight topics that add more uses of WITHOUT_BREAKING_CHANGES
would still need the old prerequisite, so let's keep its definition
for now while we'll eradicate its use.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t5323-pack-redundant.sh    | 2 +-
 t/t5505-remote.sh            | 6 +++---
 t/t5515-fetch-merge-logic.sh | 2 +-
 t/t5516-fetch-push.sh        | 8 ++++----
 t/test-lib.sh                | 5 +++++
 5 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/t/t5323-pack-redundant.sh b/t/t5323-pack-redundant.sh
index 688cd9706c..bc30bc9652 100755
--- a/t/t5323-pack-redundant.sh
+++ b/t/t5323-pack-redundant.sh
@@ -36,7 +36,7 @@ relationship between packs and objects is as follows:
 
 . ./test-lib.sh
 
-if ! test_have_prereq WITHOUT_BREAKING_CHANGES
+if test_have_prereq WITH_BREAKING_CHANGES
 then
 	skip_all='skipping git-pack-redundant tests; built with breaking changes'
 	test_done
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index bb7e0c6879..82fccf8e36 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -1123,7 +1123,7 @@ Pull: refs/heads/main:refs/heads/origin
 Pull: refs/heads/next:refs/heads/origin2
 EOF
 
-test_expect_success WITHOUT_BREAKING_CHANGES 'migrate a remote from named file in $GIT_DIR/remotes' '
+test_expect_success !WITH_BREAKING_CHANGES 'migrate a remote from named file in $GIT_DIR/remotes' '
 	git clone one five &&
 	origin_url=$(pwd)/one &&
 	(
@@ -1149,7 +1149,7 @@ test_expect_success WITHOUT_BREAKING_CHANGES 'migrate a remote from named file i
 	)
 '
 
-test_expect_success WITHOUT_BREAKING_CHANGES 'migrate a remote from named file in $GIT_DIR/branches' '
+test_expect_success !WITH_BREAKING_CHANGES 'migrate a remote from named file in $GIT_DIR/branches' '
 	git clone --template= one six &&
 	origin_url=$(pwd)/one &&
 	(
@@ -1165,7 +1165,7 @@ test_expect_success WITHOUT_BREAKING_CHANGES 'migrate a remote from named file i
 	)
 '
 
-test_expect_success WITHOUT_BREAKING_CHANGES 'migrate a remote from named file in $GIT_DIR/branches (2)' '
+test_expect_success !WITH_BREAKING_CHANGES 'migrate a remote from named file in $GIT_DIR/branches (2)' '
 	git clone --template= one seven &&
 	(
 		cd seven &&
diff --git a/t/t5515-fetch-merge-logic.sh b/t/t5515-fetch-merge-logic.sh
index 4e6026c611..8ac04d742c 100755
--- a/t/t5515-fetch-merge-logic.sh
+++ b/t/t5515-fetch-merge-logic.sh
@@ -104,7 +104,7 @@ test_expect_success setup '
 	git config remote.config-glob.fetch refs/heads/*:refs/remotes/rem/* &&
 	remotes="$remotes config-glob" &&
 
-	if test_have_prereq WITHOUT_BREAKING_CHANGES
+	if ! test_have_prereq WITH_BREAKING_CHANGES
 	then
 		mkdir -p .git/remotes &&
 		cat >.git/remotes/remote-explicit <<-\EOF &&
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 85ed049627..6e2b233157 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -975,7 +975,7 @@ test_expect_success 'allow push to HEAD of non-bare repository (config)' '
 	! grep "warning: updating the current branch" stderr
 '
 
-test_expect_success WITHOUT_BREAKING_CHANGES 'fetch with branches' '
+test_expect_success !WITH_BREAKING_CHANGES 'fetch with branches' '
 	mk_empty testrepo &&
 	git branch second $the_first_commit &&
 	git checkout second &&
@@ -991,7 +991,7 @@ test_expect_success WITHOUT_BREAKING_CHANGES 'fetch with branches' '
 	git checkout main
 '
 
-test_expect_success WITHOUT_BREAKING_CHANGES 'fetch with branches containing #' '
+test_expect_success !WITH_BREAKING_CHANGES 'fetch with branches containing #' '
 	mk_empty testrepo &&
 	mkdir testrepo/.git/branches &&
 	echo "..#second" > testrepo/.git/branches/branch2 &&
@@ -1005,7 +1005,7 @@ test_expect_success WITHOUT_BREAKING_CHANGES 'fetch with branches containing #'
 	git checkout main
 '
 
-test_expect_success WITHOUT_BREAKING_CHANGES 'push with branches' '
+test_expect_success !WITH_BREAKING_CHANGES 'push with branches' '
 	mk_empty testrepo &&
 	git checkout second &&
 
@@ -1022,7 +1022,7 @@ test_expect_success WITHOUT_BREAKING_CHANGES 'push with branches' '
 	)
 '
 
-test_expect_success WITHOUT_BREAKING_CHANGES 'push with branches containing #' '
+test_expect_success !WITH_BREAKING_CHANGES 'push with branches containing #' '
 	mk_empty testrepo &&
 
 	test_when_finished "rm -rf .git/branches" &&
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 9001ed3a64..12fe82f660 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1862,6 +1862,11 @@ test_lazy_prereq CURL '
 	curl --version
 '
 
+test_lazy_prereq WITH_BREAKING_CHANGES '
+	test -n "$WITH_BREAKING_CHANGES"
+'
+
+# DEPRECATED; DO NOT USE THIS IN NEW TESTS
 test_lazy_prereq WITHOUT_BREAKING_CHANGES '
 	test -z "$WITH_BREAKING_CHANGES"
 '
-- 
2.49.0-rc2-173-g4d16673c2b

Re: [PATCH v1 1/4] t: introduce WITH_BREAKING_CHANGES prerequisite

[Eric Sunshine]

On Mon, Mar 10, 2025 at 7:17 PM Junio C Hamano <gitster@pobox.com> wrote:
> Earlier c5bc9a7f (Makefile: wire up build option for deprecated
> features, 2025-01-22) made an unfortunate decision to introduce the
> WITHOUT_BREAKING_CHANGES prerequisite to perform tests that ensure
> the historical behaviour that may be different from what we will
> have in the future.  It would inevitably invite doulbe negation when
> we need to add tests to ensure the behaviour we want to have in the
> future.

s/doulbe/double/

> Introduce WITH_BREAKING_CHANGES prerequisite and replace the
> existing uses of WITHOUT_BREAKING_CHANGES prerequisite.  Some
> in-flight topics that add more uses of WITHOUT_BREAKING_CHANGES
> would still need the old prerequisite, so let's keep its definition
> for now while we'll eradicate its use.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Re: [PATCH v1 1/4] t: introduce WITH_BREAKING_CHANGES prerequisite

[Patrick Steinhardt]

On Mon, Mar 10, 2025 at 04:16:49PM -0700, Junio C Hamano wrote:
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 9001ed3a64..12fe82f660 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1862,6 +1862,11 @@ test_lazy_prereq CURL '
>  	curl --version
>  '
>  
> +test_lazy_prereq WITH_BREAKING_CHANGES '
> +	test -n "$WITH_BREAKING_CHANGES"
> +'
> +
> +# DEPRECATED; DO NOT USE THIS IN NEW TESTS
>  test_lazy_prereq WITHOUT_BREAKING_CHANGES '
>  	test -z "$WITH_BREAKING_CHANGES"
>  '

Do we maybe want to state that this can be removed once the next release
cycle is over? I find it to be a bit more actionable when stating hard
dates after which something can be dropped as it allows any drive-by
developers to act when they notice that Git v2.50 has been released
already.

Patrick

Re: [PATCH v1 1/4] t: introduce WITH_BREAKING_CHANGES prerequisite

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> On Mon, Mar 10, 2025 at 04:16:49PM -0700, Junio C Hamano wrote:
>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>> index 9001ed3a64..12fe82f660 100644
>> --- a/t/test-lib.sh
>> +++ b/t/test-lib.sh
>> @@ -1862,6 +1862,11 @@ test_lazy_prereq CURL '
>>  	curl --version
>>  '
>>  
>> +test_lazy_prereq WITH_BREAKING_CHANGES '
>> +	test -n "$WITH_BREAKING_CHANGES"
>> +'
>> +
>> +# DEPRECATED; DO NOT USE THIS IN NEW TESTS
>>  test_lazy_prereq WITHOUT_BREAKING_CHANGES '
>>  	test -z "$WITH_BREAKING_CHANGES"
>>  '
>
> Do we maybe want to state that this can be removed once the next release
> cycle is over?

Perhaps.  As 'seen' is pretty-much closed at this point and there is
nothing in flight that uses WITHOUT_ variant in there, v2 of this
series can just do without it, which may be simpler.

> I find it to be a bit more actionable when stating hard
> dates after which something can be dropped 

True, that is a good strategy for a transition that takes longer
time.

[PATCH v1 2/4] t6120: avoid hiding "git" exit status

[Junio C Hamano]

A handful of tests invoke "git" on the upstream side of a pipe,
hiding its exit status.  Correct them.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t6120-describe.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 76843a6169..dcb526e37d 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -292,13 +292,15 @@ test_expect_success 'name-rev --annotate-stdin' '
 		echo "$rev ($name)" >>expect.unsorted || return 1
 	done &&
 	sort <expect.unsorted >expect &&
-	git rev-list --all | git name-rev --annotate-stdin >actual.unsorted &&
+	git rev-list --all >list &&
+	git name-rev --annotate-stdin <list >actual.unsorted &&
 	sort <actual.unsorted >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'name-rev --stdin deprecated' "
-	git rev-list --all | git name-rev --stdin 2>actual &&
+	git rev-list --all >list &&
+	git name-rev --stdin <list 2>actual &&
 	grep -E 'warning: --stdin is deprecated' actual
 "
 
-- 
2.49.0-rc2-173-g4d16673c2b

[PATCH v1 3/4] t6120: further modernize

[Junio C Hamano]

There is absolutely no reason why a pattern given to grep to find
'warning: --stdin is deprecated' must be quoted within a pair of
single quotes, or the pattern to look for the literal string as ERE.

Quote the test body with a pair of single quotes like everybody
else, and quote the needle string in a pair of double quotes.  Also
use test_grep instead of "grep -E".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t6120-describe.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index dcb526e37d..71e261394a 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -298,11 +298,11 @@ test_expect_success 'name-rev --annotate-stdin' '
 	test_cmp expect actual
 '
 
-test_expect_success 'name-rev --stdin deprecated' "
+test_expect_success 'name-rev --stdin deprecated' '
 	git rev-list --all >list &&
 	git name-rev --stdin <list 2>actual &&
-	grep -E 'warning: --stdin is deprecated' actual
-"
+	test_grep "warning: --stdin is deprecated" actual
+'
 
 test_expect_success 'describe --contains with the exact tags' '
 	echo "A^0" >expect &&
-- 
2.49.0-rc2-173-g4d16673c2b

[PATCH v1 4/4] name-rev: remove "--stdin" support

[Junio C Hamano]

As part of Git 3.0, remove the hidden synonym for "--annotate-stdin"
for real.  As this does not change the fact that it used to be
called "--stdin" in older version of Git, keep that passage in the
documentation for "--annotate-stdin".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/BreakingChanges.adoc |  6 ++++++
 builtin/name-rev.c                 |  2 ++
 t/t6120-describe.sh                | 10 ++++++++--
 3 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/Documentation/BreakingChanges.adoc b/Documentation/BreakingChanges.adoc
index bdfad29d8a..61bdd586b9 100644
--- a/Documentation/BreakingChanges.adoc
+++ b/Documentation/BreakingChanges.adoc
@@ -178,6 +178,12 @@ references.
 +
 These features will be removed.
 
+* Support for "--stdin" option in the "name-rev" command was
+  deprecated (and hidden from the documentation) in the Git 2.40
+  timeframe, in preference to its synonym "--annotate-stdin".  Git 3.0
+  removes the support for "--stdin" altogether.
+
+
 == Superseded features that will not be deprecated
 
 Some features have gained newer replacements that aim to improve the design in
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index beac166b5c..3f49138551 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -578,11 +578,13 @@ int cmd_name_rev(int argc,
 				   N_("ignore refs matching <pattern>")),
 		OPT_GROUP(""),
 		OPT_BOOL(0, "all", &all, N_("list all commits reachable from all refs")),
+#ifndef WITH_BREAKING_CHANGES
 		OPT_BOOL_F(0,
 			   "stdin",
 			   &transform_stdin,
 			   N_("deprecated: use --annotate-stdin instead"),
 			   PARSE_OPT_HIDDEN),
+#endif /* WITH_BREAKING_CHANGES */
 		OPT_BOOL(0, "annotate-stdin", &annotate_stdin, N_("annotate text from stdin")),
 		OPT_BOOL(0, "undefined", &allow_undefined, N_("allow to print `undefined` names (default)")),
 		OPT_BOOL(0, "always",     &always,
diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 71e261394a..256ccaefb7 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -300,8 +300,14 @@ test_expect_success 'name-rev --annotate-stdin' '
 
 test_expect_success 'name-rev --stdin deprecated' '
 	git rev-list --all >list &&
-	git name-rev --stdin <list 2>actual &&
-	test_grep "warning: --stdin is deprecated" actual
+	if ! test_have_prereq WITH_BREAKING_CHANGES
+	then
+		git name-rev --stdin <list 2>actual &&
+		test_grep "warning: --stdin is deprecated" actual
+	else
+		test_must_fail git name-rev --stdin <list 2>actual &&
+		test_grep "unknown option .stdin." actual
+	fi
 '
 
 test_expect_success 'describe --contains with the exact tags' '
-- 
2.49.0-rc2-173-g4d16673c2b

Re: [PATCH v1 4/4] name-rev: remove "--stdin" support

[Patrick Steinhardt]

On Mon, Mar 10, 2025 at 04:16:52PM -0700, Junio C Hamano wrote:
> diff --git a/builtin/name-rev.c b/builtin/name-rev.c
> index beac166b5c..3f49138551 100644
> --- a/builtin/name-rev.c
> +++ b/builtin/name-rev.c
> @@ -578,11 +578,13 @@ int cmd_name_rev(int argc,
>  				   N_("ignore refs matching <pattern>")),
>  		OPT_GROUP(""),
>  		OPT_BOOL(0, "all", &all, N_("list all commits reachable from all refs")),
> +#ifndef WITH_BREAKING_CHANGES
>  		OPT_BOOL_F(0,
>  			   "stdin",
>  			   &transform_stdin,
>  			   N_("deprecated: use --annotate-stdin instead"),
>  			   PARSE_OPT_HIDDEN),
> +#endif /* WITH_BREAKING_CHANGES */
>  		OPT_BOOL(0, "annotate-stdin", &annotate_stdin, N_("annotate text from stdin")),
>  		OPT_BOOL(0, "undefined", &allow_undefined, N_("allow to print `undefined` names (default)")),
>  		OPT_BOOL(0, "always",     &always,

I was wondering whether we should also #ifdef `transform_stdin` and its
single user to more closely reflect what we would have once the feature
is fully removed.

Patrick

Re: [PATCH v1 4/4] name-rev: remove "--stdin" support

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> On Mon, Mar 10, 2025 at 04:16:52PM -0700, Junio C Hamano wrote:
>> diff --git a/builtin/name-rev.c b/builtin/name-rev.c
>> index beac166b5c..3f49138551 100644
>> --- a/builtin/name-rev.c
>> +++ b/builtin/name-rev.c
>> @@ -578,11 +578,13 @@ int cmd_name_rev(int argc,
>>  				   N_("ignore refs matching <pattern>")),
>>  		OPT_GROUP(""),
>>  		OPT_BOOL(0, "all", &all, N_("list all commits reachable from all refs")),
>> +#ifndef WITH_BREAKING_CHANGES
>>  		OPT_BOOL_F(0,
>>  			   "stdin",
>>  			   &transform_stdin,
>>  			   N_("deprecated: use --annotate-stdin instead"),
>>  			   PARSE_OPT_HIDDEN),
>> +#endif /* WITH_BREAKING_CHANGES */
>>  		OPT_BOOL(0, "annotate-stdin", &annotate_stdin, N_("annotate text from stdin")),
>>  		OPT_BOOL(0, "undefined", &allow_undefined, N_("allow to print `undefined` names (default)")),
>>  		OPT_BOOL(0, "always",     &always,
>
> I was wondering whether we should also #ifdef `transform_stdin` and its
> single user to more closely reflect what we would have once the feature
> is fully removed.

Absolutely.  That's a great point.

Thanks.

[PATCH v2 0/6] drop "name-rev --stdin" support

[Junio C Hamano]

During Git 2.40 timeframe, we deprecated the "--stdin" option of the
"name-rev" command in preference to the "--annotate-stdin" option,
and removed the mention of the former from the documentation.

Let's prepare for Git 3.0 to stop supporting it.

The real motive of these patches is not really about that particular
option but make sure we have, with WITH_BREAKING_CHANGES compilation
knob, enough support to keep preparing for these changes.

The first two patches are preliminary clean-up and enhancement.
We lacked documentation on test_lazy_prereq and we did not have a
good way to signal a prerequisite that no longer should be used,
both of which are remedied.

The third patch renames the WITHOUT_BREAKING_CHANGES prerequisite
that unfortunately invites double negations easily and changes
existing users of it.

Then two patches to a test script minimally modernizes it.  

The last step introduces the real change, guarded by
WITH_BREAKING_CHANGES compilation knob.  The resulting code is more
littered with #if[n]def compared to the previous round but it should
be clear which section of the code should go away once we do Git
3.0, which is the whole point of the WITH_BREAKING_CHANGES exercise.

Junio C Hamano (6):
  t: document test_lazy_prereq
  t: extend test_lazy_prereq
  t: introduce WITH_BREAKING_CHANGES prerequisite
  t6120: avoid hiding "git" exit status
  t6120: further modernize
  name-rev: remove "--stdin" support

 Documentation/BreakingChanges.adoc |  6 ++++++
 builtin/name-rev.c                 | 10 +++++++++-
 t/README                           | 25 ++++++++++++++++++++++++-
 t/t5323-pack-redundant.sh          |  2 +-
 t/t5505-remote.sh                  |  6 +++---
 t/t5515-fetch-merge-logic.sh       |  2 +-
 t/t5516-fetch-push.sh              |  8 ++++----
 t/t6120-describe.sh                | 18 +++++++++++++-----
 t/test-lib-functions.sh            |  5 +++++
 t/test-lib.sh                      |  7 ++++++-
 10 files changed, 72 insertions(+), 17 deletions(-)

-- 
2.49.0-rc2-181-g28e223d67e

[PATCH v2 1/6] t: document test_lazy_prereq

[Junio C Hamano]

The t/README file talked about test_set_prereq but lacked
explanation on test_lazy_prereq, which is a more modern way to
define prerequisites.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/README | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/t/README b/t/README
index 53e5b4a710..3ce9f5a393 100644
--- a/t/README
+++ b/t/README
@@ -818,7 +818,7 @@ Skipping tests
 --------------
 
 If you need to skip tests you should do so by using the three-arg form
-of the test_* functions (see the "Test harness library" section
+of the test_expect_* functions (see the "Test harness library" section
 below), e.g.:
 
     test_expect_success PERL 'I need Perl' '
@@ -965,6 +965,27 @@ see test-lib-functions.sh for the full list and their options.
 	    test_done
 	fi
 
+ - test_lazy_prereq <prereq> <script>
+
+   Declare the way to determine if a test prerequisite <prereq> is
+   satisified or not, but delay the actual determination until the
+   prerequisite is actually used by "test_have_prereq" or the
+   three-arg form of the test_expect_* functions.  For example, this
+   is how the SYMLINKS prerequisite is declared to see if the platform
+   supports symbolic links:
+
+	test_lazy_prereq SYMLINKS '
+		ln -s x y && test -h y
+	'
+
+   The script is lazily invoked when SYMLINKS prerequisite is first
+   queried by either "test_have_prereq SYMLINKS" or "test_expect_*
+   SYMLINKS ...".  The script is run in a temporary directory inside
+   a subshell, so you do not have to worry about removing temporary
+   files you create there.  When the script exits with status 0, the
+   prerequisite is set.  Exiting with non-zero status makes the
+   prerequisite unsatisified.
+
  - test_expect_code <exit-code> <command>
 
    Run a command and ensure that it exits with the given exit code.
-- 
2.49.0-rc2-181-g28e223d67e

[PATCH v2 2/6] t: extend test_lazy_prereq

[Junio C Hamano]

Allow test_lazy_prereq script to signal a programming error by
exiting with status 125 (like how bisect scripts do).  This is used
to signal a deprecated-and-then-removed prerequisite that should
never be used in tests anymore.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/README                | 6 ++++--
 t/test-lib-functions.sh | 5 +++++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/t/README b/t/README
index 3ce9f5a393..e9ffd9a81c 100644
--- a/t/README
+++ b/t/README
@@ -983,8 +983,10 @@ see test-lib-functions.sh for the full list and their options.
    SYMLINKS ...".  The script is run in a temporary directory inside
    a subshell, so you do not have to worry about removing temporary
    files you create there.  When the script exits with status 0, the
-   prerequisite is set.  Exiting with non-zero status makes the
-   prerequisite unsatisified.
+   prerequisite is set.  Exiting with non-zero status other than 125
+   makes the prerequisite unsatisified.  Exiting the script with 125
+   signals a programming error and is used to mark a prerequisite that
+   should not be used by test scripts.
 
  - test_expect_code <exit-code> <command>
 
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 79377bc0fc..16eaaaf4c3 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -773,6 +773,8 @@ mkdir -p "$TRASH_DIRECTORY/prereq-test-dir-'"$1"'" &&
 	rm -rf "$TRASH_DIRECTORY/prereq-test-dir-$1"
 	if test "$eval_ret" = 0; then
 		say >&3 "prerequisite $1 ok"
+	elif test "$eval_ret" = 125; then
+		:;
 	else
 		say >&3 "prerequisite $1 not satisfied"
 	fi
@@ -811,6 +813,9 @@ test_have_prereq () {
 				if test_run_lazy_prereq_ "$prerequisite" "$script"
 				then
 					test_set_prereq $prerequisite
+				elif test $? = 125
+				then
+					BUG "Do not use $prerequisite"
 				fi
 				lazily_tested_prereq="$lazily_tested_prereq$prerequisite "
 			esac
-- 
2.49.0-rc2-181-g28e223d67e

Re: [PATCH v2 2/6] t: extend test_lazy_prereq

[Patrick Steinhardt]

On Tue, Mar 11, 2025 at 02:25:01PM -0700, Junio C Hamano wrote:
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 79377bc0fc..16eaaaf4c3 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -773,6 +773,8 @@ mkdir -p "$TRASH_DIRECTORY/prereq-test-dir-'"$1"'" &&
>  	rm -rf "$TRASH_DIRECTORY/prereq-test-dir-$1"
>  	if test "$eval_ret" = 0; then
>  		say >&3 "prerequisite $1 ok"
> +	elif test "$eval_ret" = 125; then
> +		:;
>  	else
>  		say >&3 "prerequisite $1 not satisfied"
>  	fi

The semicolon in ":;" threw me off a bit. Am I missing why we need it or
is it superfluous?

> @@ -811,6 +813,9 @@ test_have_prereq () {
>  				if test_run_lazy_prereq_ "$prerequisite" "$script"
>  				then
>  					test_set_prereq $prerequisite
> +				elif test $? = 125
> +				then
> +					BUG "Do not use $prerequisite"
>  				fi
>  				lazily_tested_prereq="$lazily_tested_prereq$prerequisite "
>  			esac

Hm, okay. It feels quite close to overthinking the whole deprecation
cycle around prerequisites as it's nothing that we tend to do very
often. But on the other hand the implementation is trivial enough, so I
don't mind it much.

Patrick

Re: [PATCH v2 2/6] t: extend test_lazy_prereq

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> +	elif test "$eval_ret" = 125; then
>> +		:;
>>  	else
>>  		say >&3 "prerequisite $1 not satisfied"
>>  	fi
>
> The semicolon in ":;" threw me off a bit. Am I missing why we need it or
> is it superfluous?

The latter, of course -).

>> @@ -811,6 +813,9 @@ test_have_prereq () {
>>  				if test_run_lazy_prereq_ "$prerequisite" "$script"
>>  				then
>>  					test_set_prereq $prerequisite
>> +				elif test $? = 125
>> +				then
>> +					BUG "Do not use $prerequisite"
>>  				fi
>>  				lazily_tested_prereq="$lazily_tested_prereq$prerequisite "
>>  			esac
>
> Hm, okay. It feels quite close to overthinking the whole deprecation
> cycle around prerequisites as it's nothing that we tend to do very
> often. But on the other hand the implementation is trivial enough, so I
> don't mind it much.

I agree that this has nothing to do with breaking changes at the Git
3.0 boundary.  We just did not have good documentation for lazy
prerequisites, and we just did not have any good support for marking
a prerequisite should no longer be used.  [1/6] is for the former,
and [2/6] is for the latter.

We can avoid the magic 125 by adding a new helper like test_removed_prereq
and do this instead, which may be cleaner and simpler to reason about.

Another alternative that may make writing tests even be less error
prone but a bit more verbose is to introduce test_unset_prereq and
be explicit about unsatisfied prerequisites.  The effect of the
resulting system becomes larger to include detecting misspelled
prerequisites, and removed prerequisites would be detected as a
natural fallout from the same mechanism.

As we have >50 prerequisites defined with test_set_prereq and we'd
need to add 50 calls to test_unset_prereq to mark them as "known but
not satisified on this platform" to differentiate them from the ones
that are misspelt or removed, if we go that route.  I am not sure if
that is worth it.  Certainly not in the short term, but for a longer
term, if people ever misspelt a prerequisite SYMLINKS as SYMLINK and
wasted time wondering why their tests didn't trigger, it might be
worth it.  I dunno.

 t/test-lib-functions.sh | 14 ++++++++++++++
 t/test-lib.sh           |  5 +++--
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git c/t/test-lib-functions.sh w/t/test-lib-functions.sh
index 79377bc0fc..3903344fc1 100644
--- c/t/test-lib-functions.sh
+++ w/t/test-lib-functions.sh
@@ -751,7 +751,15 @@ test_set_prereq () {
 		;;
 	esac
 }
+
 satisfied_prereq=" "
+
+removed_prereq=
+# Mark a prerequisite deprecated-and-then-removed
+test_removed_prereq () {
+	removed_prereq="$removed_prereq$1 "
+}
+
 lazily_testable_prereq= lazily_tested_prereq=
 
 # Usage: test_lazy_prereq PREREQ 'script'
@@ -801,6 +809,12 @@ test_have_prereq () {
 			negative_prereq=
 		esac
 
+		case " $removed_prereq " in
+		*" $prerequisite "*)
+			BUG "Do not use $prerequisite"
+			;;
+		esac
+
 		case " $lazily_tested_prereq " in
 		*" $prerequisite "*)
 			;;
diff --git c/t/test-lib.sh w/t/test-lib.sh
index 9001ed3a64..c2c96f5e7a 100644
--- c/t/test-lib.sh
+++ w/t/test-lib.sh
@@ -1862,8 +1862,9 @@ test_lazy_prereq CURL '
 	curl --version
 '
 
-test_lazy_prereq WITHOUT_BREAKING_CHANGES '
-	test -z "$WITH_BREAKING_CHANGES"
+test_removed_prereq WITHOUT_BREAKING_CHANGES
+test_lazy_prereq WITH_BREAKING_CHANGES '
+	test -n "$WITH_BREAKING_CHANGES"
 '
 
 # SHA1 is a test if the hash algorithm in use is SHA-1.  This is both for tests

[PATCH v2 3/6] t: introduce WITH_BREAKING_CHANGES prerequisite

[Junio C Hamano]

Earlier c5bc9a7f (Makefile: wire up build option for deprecated
features, 2025-01-22) made an unfortunate decision to introduce the
WITHOUT_BREAKING_CHANGES prerequisite to perform tests that ensure
the historical behaviour that may be different from what we will
have in the future.  It would inevitably invite double-negation when
we need to add tests to ensure the behaviour we want to have in the
future.

Introduce WITH_BREAKING_CHANGES prerequisite and replace the
existing uses of WITHOUT_BREAKING_CHANGES prerequisite.  To catch
any future topics that add more uses of WITHOUT_BREAKING_CHANGES,
introduce a mechanism to mark a prerequisite not to be used, and
use it to mark the removed prerequisite as such.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t5323-pack-redundant.sh    | 2 +-
 t/t5505-remote.sh            | 6 +++---
 t/t5515-fetch-merge-logic.sh | 2 +-
 t/t5516-fetch-push.sh        | 8 ++++----
 t/test-lib.sh                | 7 ++++++-
 5 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/t/t5323-pack-redundant.sh b/t/t5323-pack-redundant.sh
index 688cd9706c..bc30bc9652 100755
--- a/t/t5323-pack-redundant.sh
+++ b/t/t5323-pack-redundant.sh
@@ -36,7 +36,7 @@ relationship between packs and objects is as follows:
 
 . ./test-lib.sh
 
-if ! test_have_prereq WITHOUT_BREAKING_CHANGES
+if test_have_prereq WITH_BREAKING_CHANGES
 then
 	skip_all='skipping git-pack-redundant tests; built with breaking changes'
 	test_done
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index bb7e0c6879..82fccf8e36 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -1123,7 +1123,7 @@ Pull: refs/heads/main:refs/heads/origin
 Pull: refs/heads/next:refs/heads/origin2
 EOF
 
-test_expect_success WITHOUT_BREAKING_CHANGES 'migrate a remote from named file in $GIT_DIR/remotes' '
+test_expect_success !WITH_BREAKING_CHANGES 'migrate a remote from named file in $GIT_DIR/remotes' '
 	git clone one five &&
 	origin_url=$(pwd)/one &&
 	(
@@ -1149,7 +1149,7 @@ test_expect_success WITHOUT_BREAKING_CHANGES 'migrate a remote from named file i
 	)
 '
 
-test_expect_success WITHOUT_BREAKING_CHANGES 'migrate a remote from named file in $GIT_DIR/branches' '
+test_expect_success !WITH_BREAKING_CHANGES 'migrate a remote from named file in $GIT_DIR/branches' '
 	git clone --template= one six &&
 	origin_url=$(pwd)/one &&
 	(
@@ -1165,7 +1165,7 @@ test_expect_success WITHOUT_BREAKING_CHANGES 'migrate a remote from named file i
 	)
 '
 
-test_expect_success WITHOUT_BREAKING_CHANGES 'migrate a remote from named file in $GIT_DIR/branches (2)' '
+test_expect_success !WITH_BREAKING_CHANGES 'migrate a remote from named file in $GIT_DIR/branches (2)' '
 	git clone --template= one seven &&
 	(
 		cd seven &&
diff --git a/t/t5515-fetch-merge-logic.sh b/t/t5515-fetch-merge-logic.sh
index 4e6026c611..8ac04d742c 100755
--- a/t/t5515-fetch-merge-logic.sh
+++ b/t/t5515-fetch-merge-logic.sh
@@ -104,7 +104,7 @@ test_expect_success setup '
 	git config remote.config-glob.fetch refs/heads/*:refs/remotes/rem/* &&
 	remotes="$remotes config-glob" &&
 
-	if test_have_prereq WITHOUT_BREAKING_CHANGES
+	if ! test_have_prereq WITH_BREAKING_CHANGES
 	then
 		mkdir -p .git/remotes &&
 		cat >.git/remotes/remote-explicit <<-\EOF &&
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 85ed049627..6e2b233157 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -975,7 +975,7 @@ test_expect_success 'allow push to HEAD of non-bare repository (config)' '
 	! grep "warning: updating the current branch" stderr
 '
 
-test_expect_success WITHOUT_BREAKING_CHANGES 'fetch with branches' '
+test_expect_success !WITH_BREAKING_CHANGES 'fetch with branches' '
 	mk_empty testrepo &&
 	git branch second $the_first_commit &&
 	git checkout second &&
@@ -991,7 +991,7 @@ test_expect_success WITHOUT_BREAKING_CHANGES 'fetch with branches' '
 	git checkout main
 '
 
-test_expect_success WITHOUT_BREAKING_CHANGES 'fetch with branches containing #' '
+test_expect_success !WITH_BREAKING_CHANGES 'fetch with branches containing #' '
 	mk_empty testrepo &&
 	mkdir testrepo/.git/branches &&
 	echo "..#second" > testrepo/.git/branches/branch2 &&
@@ -1005,7 +1005,7 @@ test_expect_success WITHOUT_BREAKING_CHANGES 'fetch with branches containing #'
 	git checkout main
 '
 
-test_expect_success WITHOUT_BREAKING_CHANGES 'push with branches' '
+test_expect_success !WITH_BREAKING_CHANGES 'push with branches' '
 	mk_empty testrepo &&
 	git checkout second &&
 
@@ -1022,7 +1022,7 @@ test_expect_success WITHOUT_BREAKING_CHANGES 'push with branches' '
 	)
 '
 
-test_expect_success WITHOUT_BREAKING_CHANGES 'push with branches containing #' '
+test_expect_success !WITH_BREAKING_CHANGES 'push with branches containing #' '
 	mk_empty testrepo &&
 
 	test_when_finished "rm -rf .git/branches" &&
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 9001ed3a64..fffbfb89ef 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1862,8 +1862,13 @@ test_lazy_prereq CURL '
 	curl --version
 '
 
+test_lazy_prereq WITH_BREAKING_CHANGES '
+	test -n "$WITH_BREAKING_CHANGES"
+'
+
 test_lazy_prereq WITHOUT_BREAKING_CHANGES '
-	test -z "$WITH_BREAKING_CHANGES"
+	# Signal that this prereq should not be used.
+	exit 125
 '
 
 # SHA1 is a test if the hash algorithm in use is SHA-1.  This is both for tests
-- 
2.49.0-rc2-181-g28e223d67e

Re: [PATCH v2 3/6] t: introduce WITH_BREAKING_CHANGES prerequisite

[Patrick Steinhardt]

On Tue, Mar 11, 2025 at 02:25:02PM -0700, Junio C Hamano wrote:
> Earlier c5bc9a7f (Makefile: wire up build option for deprecated
> features, 2025-01-22) made an unfortunate decision to introduce the
> WITHOUT_BREAKING_CHANGES prerequisite to perform tests that ensure
> the historical behaviour that may be different from what we will
> have in the future.  It would inevitably invite double-negation when
> we need to add tests to ensure the behaviour we want to have in the
> future.
> 
> Introduce WITH_BREAKING_CHANGES prerequisite and replace the
> existing uses of WITHOUT_BREAKING_CHANGES prerequisite.  To catch
> any future topics that add more uses of WITHOUT_BREAKING_CHANGES,
> introduce a mechanism to mark a prerequisite not to be used, and
> use it to mark the removed prerequisite as such.

Nit: the mechanism has already been introduced in the preceding commit.

Patrick

[PATCH v2 4/6] t6120: avoid hiding "git" exit status

[Junio C Hamano]

A handful of tests invoke "git" on the upstream side of a pipe,
hiding its exit status.  Correct them.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t6120-describe.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 76843a6169..dcb526e37d 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -292,13 +292,15 @@ test_expect_success 'name-rev --annotate-stdin' '
 		echo "$rev ($name)" >>expect.unsorted || return 1
 	done &&
 	sort <expect.unsorted >expect &&
-	git rev-list --all | git name-rev --annotate-stdin >actual.unsorted &&
+	git rev-list --all >list &&
+	git name-rev --annotate-stdin <list >actual.unsorted &&
 	sort <actual.unsorted >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'name-rev --stdin deprecated' "
-	git rev-list --all | git name-rev --stdin 2>actual &&
+	git rev-list --all >list &&
+	git name-rev --stdin <list 2>actual &&
 	grep -E 'warning: --stdin is deprecated' actual
 "
 
-- 
2.49.0-rc2-181-g28e223d67e

[PATCH v2 5/6] t6120: further modernize

[Junio C Hamano]

There is absolutely no reason why a pattern given to grep to find
'warning: --stdin is deprecated' must be quoted within a pair of
single quotes, or the pattern to look for the literal string as ERE.

Quote the test body with a pair of single quotes like everybody
else, and quote the needle string in a pair of double quotes.  Also
use test_grep instead of "grep -E".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t6120-describe.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index dcb526e37d..71e261394a 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -298,11 +298,11 @@ test_expect_success 'name-rev --annotate-stdin' '
 	test_cmp expect actual
 '
 
-test_expect_success 'name-rev --stdin deprecated' "
+test_expect_success 'name-rev --stdin deprecated' '
 	git rev-list --all >list &&
 	git name-rev --stdin <list 2>actual &&
-	grep -E 'warning: --stdin is deprecated' actual
-"
+	test_grep "warning: --stdin is deprecated" actual
+'
 
 test_expect_success 'describe --contains with the exact tags' '
 	echo "A^0" >expect &&
-- 
2.49.0-rc2-181-g28e223d67e

[PATCH v2 6/6] name-rev: remove "--stdin" support

[Junio C Hamano]

As part of Git 3.0, remove the hidden synonym for "--annotate-stdin"
for real.  As this does not change the fact that it used to be
called "--stdin" in older version of Git, keep that passage in the
documentation for "--annotate-stdin".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/BreakingChanges.adoc |  6 ++++++
 builtin/name-rev.c                 | 10 +++++++++-
 t/t6120-describe.sh                | 10 ++++++++--
 3 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/Documentation/BreakingChanges.adoc b/Documentation/BreakingChanges.adoc
index bdfad29d8a..61bdd586b9 100644
--- a/Documentation/BreakingChanges.adoc
+++ b/Documentation/BreakingChanges.adoc
@@ -178,6 +178,12 @@ references.
 +
 These features will be removed.
 
+* Support for "--stdin" option in the "name-rev" command was
+  deprecated (and hidden from the documentation) in the Git 2.40
+  timeframe, in preference to its synonym "--annotate-stdin".  Git 3.0
+  removes the support for "--stdin" altogether.
+
+
 == Superseded features that will not be deprecated
 
 Some features have gained newer replacements that aim to improve the design in
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index beac166b5c..65f867d7a4 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -567,7 +567,11 @@ int cmd_name_rev(int argc,
 {
 	struct mem_pool string_pool;
 	struct object_array revs = OBJECT_ARRAY_INIT;
-	int all = 0, annotate_stdin = 0, transform_stdin = 0, allow_undefined = 1, always = 0, peel_tag = 0;
+
+#ifndef WITH_BREAKING_CHANGES
+	int transform_stdin = 0;
+#endif
+	int all = 0, annotate_stdin = 0, allow_undefined = 1, always = 0, peel_tag = 0;
 	struct name_ref_data data = { 0, 0, STRING_LIST_INIT_NODUP, STRING_LIST_INIT_NODUP };
 	struct option opts[] = {
 		OPT_BOOL(0, "name-only", &data.name_only, N_("print only ref-based names (no object names)")),
@@ -578,11 +582,13 @@ int cmd_name_rev(int argc,
 				   N_("ignore refs matching <pattern>")),
 		OPT_GROUP(""),
 		OPT_BOOL(0, "all", &all, N_("list all commits reachable from all refs")),
+#ifndef WITH_BREAKING_CHANGES
 		OPT_BOOL_F(0,
 			   "stdin",
 			   &transform_stdin,
 			   N_("deprecated: use --annotate-stdin instead"),
 			   PARSE_OPT_HIDDEN),
+#endif /* WITH_BREAKING_CHANGES */
 		OPT_BOOL(0, "annotate-stdin", &annotate_stdin, N_("annotate text from stdin")),
 		OPT_BOOL(0, "undefined", &allow_undefined, N_("allow to print `undefined` names (default)")),
 		OPT_BOOL(0, "always",     &always,
@@ -597,12 +603,14 @@ int cmd_name_rev(int argc,
 	git_config(git_default_config, NULL);
 	argc = parse_options(argc, argv, prefix, opts, name_rev_usage, 0);
 
+#ifndef WITH_BREAKING_CHANGES
 	if (transform_stdin) {
 		warning("--stdin is deprecated. Please use --annotate-stdin instead, "
 					"which is functionally equivalent.\n"
 					"This option will be removed in a future release.");
 		annotate_stdin = 1;
 	}
+#endif
 
 	if (all + annotate_stdin + !!argc > 1) {
 		error("Specify either a list, or --all, not both!");
diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 71e261394a..256ccaefb7 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -300,8 +300,14 @@ test_expect_success 'name-rev --annotate-stdin' '
 
 test_expect_success 'name-rev --stdin deprecated' '
 	git rev-list --all >list &&
-	git name-rev --stdin <list 2>actual &&
-	test_grep "warning: --stdin is deprecated" actual
+	if ! test_have_prereq WITH_BREAKING_CHANGES
+	then
+		git name-rev --stdin <list 2>actual &&
+		test_grep "warning: --stdin is deprecated" actual
+	else
+		test_must_fail git name-rev --stdin <list 2>actual &&
+		test_grep "unknown option .stdin." actual
+	fi
 '
 
 test_expect_success 'describe --contains with the exact tags' '
-- 
2.49.0-rc2-181-g28e223d67e