[PATCH] t0450: fix test for out-of-tree builds

[PATCH] t0450: fix test for out-of-tree builds

[Toon Claes]

When using Meson, builds are out-of-tree and $GIT_BUILD_DIR gets set to
the path where the build output is landing. To locate the Documentation
sources, test 't0450' was using that path.

Modify test 't0450' to use `$GIT_SOURCE_DIR/Documentation` to find the
documentation sources.

Signed-off-by: Toon Claes <toon@iotcl.com>
---
 t/t0450-txt-doc-vs-help.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t0450-txt-doc-vs-help.sh b/t/t0450-txt-doc-vs-help.sh
index 2f7504ae7e..da2d0af5b0 100755
--- a/t/t0450-txt-doc-vs-help.sh
+++ b/t/t0450-txt-doc-vs-help.sh
@@ -41,7 +41,7 @@ help_to_synopsis () {
 }
 
 builtin_to_adoc () {
-       echo "$GIT_BUILD_DIR/Documentation/git-$1.adoc"
+       echo "$GIT_SOURCE_DIR/Documentation/git-$1.adoc"
 }
 
 adoc_to_synopsis () {
-- 
2.50.1.327.g047016eb4a

Re: [PATCH] t0450: fix test for out-of-tree builds

[Patrick Steinhardt]

On Mon, Aug 04, 2025 at 09:30:02AM +0200, Toon Claes wrote:
> When using Meson, builds are out-of-tree and $GIT_BUILD_DIR gets set to
> the path where the build output is landing. To locate the Documentation
> sources, test 't0450' was using that path.
> 
> Modify test 't0450' to use `$GIT_SOURCE_DIR/Documentation` to find the
> documentation sources.
> 
> Signed-off-by: Toon Claes <toon@iotcl.com>
> ---
>  t/t0450-txt-doc-vs-help.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/t/t0450-txt-doc-vs-help.sh b/t/t0450-txt-doc-vs-help.sh
> index 2f7504ae7e..da2d0af5b0 100755
> --- a/t/t0450-txt-doc-vs-help.sh
> +++ b/t/t0450-txt-doc-vs-help.sh
> @@ -41,7 +41,7 @@ help_to_synopsis () {
>  }
>  
>  builtin_to_adoc () {
> -       echo "$GIT_BUILD_DIR/Documentation/git-$1.adoc"
> +       echo "$GIT_SOURCE_DIR/Documentation/git-$1.adoc"
>  }

Ok, the change itself looks reasonable to me. One question that the
commit message doesn't answer though is why this didn't cause the test
to fail. I think the answer is that we have the following loop:

	while read builtin
	do
		...

		adoc="$(builtin_to_adoc "$builtin")" &&
		preq="$(echo BUILTIN_ADOC_$builtin | tr '[:lower:]-' '[:upper:]_')" &&

		if test -f "$adoc"
		then
			test_set_prereq "$preq"
		fi &&

		...
	done <builtins

So we explicitly check wether the ".adoc" file exists, and if it doesn't
we don't have its prereq. All subsequent tests then use that prereq, so
we skip all of those tests with Meson entirely.

Which indicates that this prereq is overly loose: ideally we should not
skip such tests, but rather print an error that something is fishy. I
suspect that we have the prereq in place because there are some builtins
that don't have a corresponding manpage though.

Maybe this is something we could explore: what breaks if we remove the
prereq entirely? And if this breakage is limited to a small number of
builtins we can maybe use an explicit skip-list like we already do with
"t/t0450/adoc-help-mismatches".

Thanks!

Patrick

Re: [PATCH] t0450: fix test for out-of-tree builds

[Toon Claes]

Patrick Steinhardt <ps@pks.im> writes:

> Ok, the change itself looks reasonable to me. One question that the
> commit message doesn't answer though is why this didn't cause the test
> to fail.

Ah yes, I should have pointed that out in the commit message. But as you
say, in short it simply skips the tests is if cannot find the .adoc.

> [snip]

> Which indicates that this prereq is overly loose: ideally we should not
> skip such tests, but rather print an error that something is fishy. I
> suspect that we have the prereq in place because there are some builtins
> that don't have a corresponding manpage though.
>
> Maybe this is something we could explore: what breaks if we remove the
> prereq entirely? And if this breakage is limited to a small number of
> builtins we can maybe use an explicit skip-list like we already do with
> "t/t0450/adoc-help-mismatches".

I think that's a great change we should make on top. Being explicit
about which subcommands do not require documentation is better than
simply letting them slide through. Every new subcommand would require
documentation, and if not so, adding them to a list somewhere makes it
clear why.

-- 
Cheers,
Toon

[PATCH v2 0/2] Harden test t0450-txt-doc-vs-help

[Toon Claes]

These patches add a few improvements to test t0450-txt-doc-vs-help.

When using Meson, some test cases seem to have been skipped. This
happened because it was looking for .adoc files in the wrong location.
The first patch fixes that issue.

But, because these tests were skipped silently (when using Meson), I did
not notice breakage in another patch series of mine. To harden the test,
the second patch adds an allowlist.

Toon Claes (2):
  t0450: fix test for out-of-tree builds
  t0450: add allowlist for builtins with missing .adoc

 t/t0450-txt-doc-vs-help.sh | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Range-diff against v1:
1:  184290765e = 1:  cc1dd21b1c t0450: fix test for out-of-tree builds
-:  ---------- > 2:  b8c6d88c47 t0450: add allowlist for builtins with missing .adoc

base-commit: 2c2ba49d55ff26c1082b8137b1ec5eeccb4337d1
prerequisite-patch-id: 0000000000000000000000000000000000000000
--
2.50.1.327.g047016eb4a

[PATCH v2 1/2] t0450: fix test for out-of-tree builds

[Toon Claes]

When using Meson, builds are out-of-tree and $GIT_BUILD_DIR gets set to
the path where the build output is landing. To locate the Documentation
sources, test 't0450' was using that path.

Modify test 't0450' to use `$GIT_SOURCE_DIR/Documentation` to find the
documentation sources.

Signed-off-by: Toon Claes <toon@iotcl.com>
---
 t/t0450-txt-doc-vs-help.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t0450-txt-doc-vs-help.sh b/t/t0450-txt-doc-vs-help.sh
index 2f7504ae7e..da2d0af5b0 100755
--- a/t/t0450-txt-doc-vs-help.sh
+++ b/t/t0450-txt-doc-vs-help.sh
@@ -41,7 +41,7 @@ help_to_synopsis () {
 }
 
 builtin_to_adoc () {
-       echo "$GIT_BUILD_DIR/Documentation/git-$1.adoc"
+       echo "$GIT_SOURCE_DIR/Documentation/git-$1.adoc"
 }
 
 adoc_to_synopsis () {
-- 
2.50.1.327.g047016eb4a

[PATCH v2 2/2] t0450: add allowlist for builtins with missing .adoc

[Toon Claes]

Before we were silently skipping all builtins that don't have a matching
.adoc file. This is overly loose and might skip documentation files
when it shouldn't, for example when there was a typo in the filename.

To ensure no new builtins are added without documentation, add an
allowlist: t0450/adoc-missing. In this file only builtin commands that
do *not* have a corresponding .adoc file shall be listed. If there is a
mismatch, fail the test. This should force future contributions to
either add an .adoc, or add the builtin name to the allowlist file.

Signed-off-by: Toon Claes <toon@iotcl.com>
---
 t/t0450-txt-doc-vs-help.sh | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/t/t0450-txt-doc-vs-help.sh b/t/t0450-txt-doc-vs-help.sh
index da2d0af5b0..980130be78 100755
--- a/t/t0450-txt-doc-vs-help.sh
+++ b/t/t0450-txt-doc-vs-help.sh
@@ -112,10 +112,16 @@ do
 	adoc="$(builtin_to_adoc "$builtin")" &&
 	preq="$(echo BUILTIN_ADOC_$builtin | tr '[:lower:]-' '[:upper:]_')" &&
 
-	if test -f "$adoc"
+	# if and only if *.adoc is missing, builtin shall be listed in t0450/adoc-missing
+	result=success
+	if grep -q "^$builtin$" "$TEST_DIRECTORY"/t0450/adoc-missing
 	then
 		test_set_prereq "$preq"
+		result=failure
 	fi &&
+	test_expect_$result "$builtin appropriately marked as having missing .adoc" '
+		test -f "$adoc"
+	'
 
 	# *.adoc output assertions
 	test_expect_success "$preq" "$builtin *.adoc SYNOPSIS has dashed labels" '
-- 
2.50.1.327.g047016eb4a

Re: [PATCH v2 2/2] t0450: add allowlist for builtins with missing .adoc

[Junio C Hamano]

Toon Claes <toon@iotcl.com> writes:

> Before we were silently skipping all builtins that don't have a matching
> .adoc file. This is overly loose and might skip documentation files
> when it shouldn't, for example when there was a typo in the filename.
>
> To ensure no new builtins are added without documentation, add an
> allowlist: t0450/adoc-missing. In this file only builtin commands that
> do *not* have a corresponding .adoc file shall be listed. If there is a
> mismatch, fail the test. This should force future contributions to
> either add an .adoc, or add the builtin name to the allowlist file.
>
> Signed-off-by: Toon Claes <toon@iotcl.com>
> ---
>  t/t0450-txt-doc-vs-help.sh | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)

Forgot to add something?

>
> diff --git a/t/t0450-txt-doc-vs-help.sh b/t/t0450-txt-doc-vs-help.sh
> index da2d0af5b0..980130be78 100755
> --- a/t/t0450-txt-doc-vs-help.sh
> +++ b/t/t0450-txt-doc-vs-help.sh
> @@ -112,10 +112,16 @@ do
>  	adoc="$(builtin_to_adoc "$builtin")" &&
>  	preq="$(echo BUILTIN_ADOC_$builtin | tr '[:lower:]-' '[:upper:]_')" &&
>  
> -	if test -f "$adoc"
> +	# if and only if *.adoc is missing, builtin shall be listed in t0450/adoc-missing
> +	result=success
> +	if grep -q "^$builtin$" "$TEST_DIRECTORY"/t0450/adoc-missing
>  	then
>  		test_set_prereq "$preq"
> +		result=failure
>  	fi &&
> +	test_expect_$result "$builtin appropriately marked as having missing .adoc" '
> +		test -f "$adoc"
> +	'
>  
>  	# *.adoc output assertions
>  	test_expect_success "$preq" "$builtin *.adoc SYNOPSIS has dashed labels" '

Re: [PATCH v2 2/2] t0450: add allowlist for builtins with missing .adoc

[Patrick Steinhardt]

On Fri, Aug 08, 2025 at 03:07:09PM -0700, Junio C Hamano wrote:
> Toon Claes <toon@iotcl.com> writes:
> 
> > Before we were silently skipping all builtins that don't have a matching
> > .adoc file. This is overly loose and might skip documentation files
> > when it shouldn't, for example when there was a typo in the filename.
> >
> > To ensure no new builtins are added without documentation, add an
> > allowlist: t0450/adoc-missing. In this file only builtin commands that
> > do *not* have a corresponding .adoc file shall be listed. If there is a
> > mismatch, fail the test. This should force future contributions to
> > either add an .adoc, or add the builtin name to the allowlist file.
> >
> > Signed-off-by: Toon Claes <toon@iotcl.com>
> > ---
> >  t/t0450-txt-doc-vs-help.sh | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> Forgot to add something?

Indeed. Toon is currently out of office, so I had a look at what it
takes. The below patch is what I ended up with -- note that I also had
to reverse the `grep` condition to set the prereq in the else branch.

Let me know whether you're fine with just squashing these changes in or
whether I shall send another version.

Thanks!

Patrick

diff --git a/t/t0450-txt-doc-vs-help.sh b/t/t0450-txt-doc-vs-help.sh
index da2d0af5b0..e12e18f97f 100755
--- a/t/t0450-txt-doc-vs-help.sh
+++ b/t/t0450-txt-doc-vs-help.sh
@@ -112,10 +112,19 @@ do
 	adoc="$(builtin_to_adoc "$builtin")" &&
 	preq="$(echo BUILTIN_ADOC_$builtin | tr '[:lower:]-' '[:upper:]_')" &&
 
-	if test -f "$adoc"
+	# If and only if *.adoc is missing, builtin shall be listed in t0450/adoc-missing.
+	if grep -q "^$builtin$" "$TEST_DIRECTORY"/t0450/adoc-missing
 	then
+		test_expect_success "$builtin appropriately marked as not having .adoc" '
+			! test -f "$adoc"
+		'
+	else
 		test_set_prereq "$preq"
-	fi &&
+
+		test_expect_success "$builtin appropriately marked as having .adoc" '
+			test -f "$adoc"
+		'
+	fi
 
 	# *.adoc output assertions
 	test_expect_success "$preq" "$builtin *.adoc SYNOPSIS has dashed labels" '
diff --git a/t/t0450/adoc-missing b/t/t0450/adoc-missing
new file mode 100644
index 0000000000..1ec9f8dcf3
--- /dev/null
+++ b/t/t0450/adoc-missing
@@ -0,0 +1,9 @@
+checkout--worker
+merge-ours
+merge-recursive
+merge-recursive-ours
+merge-recursive-theirs
+merge-subtree
+pickaxe
+submodule--helper
+upload-archive--writer

Re: [PATCH v2 2/2] t0450: add allowlist for builtins with missing .adoc

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> > To ensure no new builtins are added without documentation, add an
>> > allowlist: t0450/adoc-missing...
>> > ...
>> >  t/t0450-txt-doc-vs-help.sh | 8 +++++++-
>> >  1 file changed, 7 insertions(+), 1 deletion(-)
>> 
>> Forgot to add something?
>
> Indeed. Toon is currently out of office, so I had a look at what it
> takes. The below patch is what I ended up with -- note that I also had
> to reverse the `grep` condition to set the prereq in the else branch.
>
> Let me know whether you're fine with just squashing these changes in or
> whether I shall send another version.

I've marked this topic in the What's cooking report to be expecting
a reroll after 2.51 final gets tagged, which has now done.  If the
fixup! sitting at the tip of the topic is good to Toon's eyes, then
I can squash it in and mark the topic for 'next' without waiting for
a reroll.  If not, please do send in a hopefully small and final
update.

Thanks.

From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 12 Aug 2025 14:52:31 +0200
Subject: [PATCH] fixup! t0450: add allowlist for builtins with missing .adoc

---
 t/t0450-txt-doc-vs-help.sh | 17 ++++++++++-------
 t/t0450/adoc-missing       |  9 +++++++++
 2 files changed, 19 insertions(+), 7 deletions(-)
 create mode 100644 t/t0450/adoc-missing

diff --git a/t/t0450-txt-doc-vs-help.sh b/t/t0450-txt-doc-vs-help.sh
index 980130be78..e12e18f97f 100755
--- a/t/t0450-txt-doc-vs-help.sh
+++ b/t/t0450-txt-doc-vs-help.sh
@@ -112,16 +112,19 @@ do
 	adoc="$(builtin_to_adoc "$builtin")" &&
 	preq="$(echo BUILTIN_ADOC_$builtin | tr '[:lower:]-' '[:upper:]_')" &&
 
-	# if and only if *.adoc is missing, builtin shall be listed in t0450/adoc-missing
-	result=success
+	# If and only if *.adoc is missing, builtin shall be listed in t0450/adoc-missing.
 	if grep -q "^$builtin$" "$TEST_DIRECTORY"/t0450/adoc-missing
 	then
+		test_expect_success "$builtin appropriately marked as not having .adoc" '
+			! test -f "$adoc"
+		'
+	else
 		test_set_prereq "$preq"
-		result=failure
-	fi &&
-	test_expect_$result "$builtin appropriately marked as having missing .adoc" '
-		test -f "$adoc"
-	'
+
+		test_expect_success "$builtin appropriately marked as having .adoc" '
+			test -f "$adoc"
+		'
+	fi
 
 	# *.adoc output assertions
 	test_expect_success "$preq" "$builtin *.adoc SYNOPSIS has dashed labels" '
diff --git a/t/t0450/adoc-missing b/t/t0450/adoc-missing
new file mode 100644
index 0000000000..1ec9f8dcf3
--- /dev/null
+++ b/t/t0450/adoc-missing
@@ -0,0 +1,9 @@
+checkout--worker
+merge-ours
+merge-recursive
+merge-recursive-ours
+merge-recursive-theirs
+merge-subtree
+pickaxe
+submodule--helper
+upload-archive--writer
-- 
2.51.0-262-gbae8ff527a

Re: [PATCH v2 2/2] t0450: add allowlist for builtins with missing .adoc

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Patrick Steinhardt <ps@pks.im> writes:
>
>>> > To ensure no new builtins are added without documentation, add an
>>> > allowlist: t0450/adoc-missing...
>>> > ...
>>> >  t/t0450-txt-doc-vs-help.sh | 8 +++++++-
>>> >  1 file changed, 7 insertions(+), 1 deletion(-)
>>> 
>>> Forgot to add something?
>>
>> Indeed. Toon is currently out of office, so I had a look at what it
>> takes. The below patch is what I ended up with -- note that I also had
>> to reverse the `grep` condition to set the prereq in the else branch.
>>
>> Let me know whether you're fine with just squashing these changes in or
>> whether I shall send another version.
>
> I've marked this topic in the What's cooking report to be expecting
> a reroll after 2.51 final gets tagged, which has now done.  If the
> fixup! sitting at the tip of the topic is good to Toon's eyes, then
> I can squash it in and mark the topic for 'next' without waiting for
> a reroll.  If not, please do send in a hopefully small and final
> update.
>
> Thanks.

Toon, did you have a chance to take a look at Patrick's update?  Can
we move forward by squashing it into your [2/2]?

Thanks.


>
> From: Patrick Steinhardt <ps@pks.im>
> Date: Tue, 12 Aug 2025 14:52:31 +0200
> Subject: [PATCH] fixup! t0450: add allowlist for builtins with missing .adoc
>
> ---
>  t/t0450-txt-doc-vs-help.sh | 17 ++++++++++-------
>  t/t0450/adoc-missing       |  9 +++++++++
>  2 files changed, 19 insertions(+), 7 deletions(-)
>  create mode 100644 t/t0450/adoc-missing
>
> diff --git a/t/t0450-txt-doc-vs-help.sh b/t/t0450-txt-doc-vs-help.sh
> index 980130be78..e12e18f97f 100755
> --- a/t/t0450-txt-doc-vs-help.sh
> +++ b/t/t0450-txt-doc-vs-help.sh
> @@ -112,16 +112,19 @@ do
>  	adoc="$(builtin_to_adoc "$builtin")" &&
>  	preq="$(echo BUILTIN_ADOC_$builtin | tr '[:lower:]-' '[:upper:]_')" &&
>  
> -	# if and only if *.adoc is missing, builtin shall be listed in t0450/adoc-missing
> -	result=success
> +	# If and only if *.adoc is missing, builtin shall be listed in t0450/adoc-missing.
>  	if grep -q "^$builtin$" "$TEST_DIRECTORY"/t0450/adoc-missing
>  	then
> +		test_expect_success "$builtin appropriately marked as not having .adoc" '
> +			! test -f "$adoc"
> +		'
> +	else
>  		test_set_prereq "$preq"
> -		result=failure
> -	fi &&
> -	test_expect_$result "$builtin appropriately marked as having missing .adoc" '
> -		test -f "$adoc"
> -	'
> +
> +		test_expect_success "$builtin appropriately marked as having .adoc" '
> +			test -f "$adoc"
> +		'
> +	fi
>  
>  	# *.adoc output assertions
>  	test_expect_success "$preq" "$builtin *.adoc SYNOPSIS has dashed labels" '
> diff --git a/t/t0450/adoc-missing b/t/t0450/adoc-missing
> new file mode 100644
> index 0000000000..1ec9f8dcf3
> --- /dev/null
> +++ b/t/t0450/adoc-missing
> @@ -0,0 +1,9 @@
> +checkout--worker
> +merge-ours
> +merge-recursive
> +merge-recursive-ours
> +merge-recursive-theirs
> +merge-subtree
> +pickaxe
> +submodule--helper
> +upload-archive--writer

Re: [PATCH v2 2/2] t0450: add allowlist for builtins with missing .adoc

[Toon Claes]

--text follows this line--
Junio C Hamano <gitster@pobox.com> writes:

> Toon, did you have a chance to take a look at Patrick's update?  Can
> we move forward by squashing it into your [2/2]?

Hi Junio,

Yes, I've reviewed the changes, and they look good to me. I like the
code more than the /clever/ approach I was attempting. My code succeeded
on my machine, but it didn't on CI (sorry, I only saw that too late, I
was rushing into my time off).

Thanks both!

--
Toon

Re: [PATCH v2 2/2] t0450: add allowlist for builtins with missing .adoc

[Junio C Hamano]

Toon Claes <toon@iotcl.com> writes:

> --text follows this line--
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Toon, did you have a chance to take a look at Patrick's update?  Can
>> we move forward by squashing it into your [2/2]?
>
> Hi Junio,
>
> Yes, I've reviewed the changes, and they look good to me. I like the
> code more than the /clever/ approach I was attempting. My code succeeded
> on my machine, but it didn't on CI (sorry, I only saw that too late, I
> was rushing into my time off).
>
> Thanks both!

OK, then let me squash that into the base patch and mark the topic
for 'next'.

Thanks.