[PATCH 0/4] Documentation: improve linting of manpage existence

[PATCH 0/4] Documentation: improve linting of manpage existence

[Patrick Steinhardt]

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

Hi,

in [1], Junio noticed that `make check-docs` was failing for a recent
patch series of mine that introduces the new git-refs(1) command.
Curiously though, while the checks do print errors, the Makefile target
itself succeeded and thus did not make the corresponding CI job fail.

This patch series fixes that issue and also refactors the infrastructure
such that it fits better into our existing set of linter targets for our
documentation. Finally, this series then adds a job to GitLab CI that is
equivalent to the job we already have on GitHub Actions.

[1]: <xmqqjzj9czop.fsf@gitster.g>

Patrick

Patrick Steinhardt (4):
  Makefile: extract script to lint missing/extraneous manpages
  Documentation/lint-manpages: bubble up errors
  gitlab-ci: add job to run `make check-docs`
  ci/test-documentation: work around SyntaxWarning in Python 3.12

 .gitlab-ci.yml                 |   9 +++
 Documentation/Makefile         |   4 ++
 Documentation/lint-manpages.sh | 107 +++++++++++++++++++++++++++++++++
 Makefile                       |  36 -----------
 ci/test-documentation.sh       |   1 +
 5 files changed, 121 insertions(+), 36 deletions(-)
 create mode 100755 Documentation/lint-manpages.sh

-- 
2.45.2.409.g7b0defb391.dirty


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

[PATCH 1/4] Makefile: extract script to lint missing/extraneous manpages

[Patrick Steinhardt]

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

The "check-docs" target of our top-level Makefile fulfills two different
roles. For one it runs the "lint-docs" target of the "Documentation/"
Makefile. And second it performs some checks of whether there are any
manpages that are missing or extraneous via some inline scripts.

The second set of checks feels quite misplaced in the top-level Makefile
as it would fit in much better with our "lint-docs" target. Back when
the checks were introduced in 8c989ec528 (Makefile: $(MAKE) check-docs,
2006-04-13), that target did not yet exist though.

Furthermore, the script makes use of several Makefile variables which
are defined in the top-level Makefile, which makes it hard to access
their contents from elsewhere. There is a trick though that we already
use in "check-builtins.sh" to gain access: we can create an ad-hoc
Makefile that has an extra target to print those variables.

Pull out the script into a separate "lint-manpages.sh" script by using
that trick. Wire up that script via the "lint-docs" target. For one,
normal shell scripts are way easier to reason about than those which are
embedded in a Makefile. Second, it allows one to easily execute the
script standalone without any of the other checks.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/Makefile         |  4 ++
 Documentation/lint-manpages.sh | 82 ++++++++++++++++++++++++++++++++++
 Makefile                       | 36 ---------------
 3 files changed, 86 insertions(+), 36 deletions(-)
 create mode 100755 Documentation/lint-manpages.sh

diff --git a/Documentation/Makefile b/Documentation/Makefile
index a04da672c6..a3868a462f 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -485,12 +485,16 @@ $(LINT_DOCS_FSCK_MSGIDS): ../fsck.h fsck-msgids.txt
 
 lint-docs-fsck-msgids: $(LINT_DOCS_FSCK_MSGIDS)
 
+lint-docs-manpages:
+	$(QUIET_GEN)./lint-manpages.sh
+
 ## Lint: list of targets above
 .PHONY: lint-docs
 lint-docs: lint-docs-fsck-msgids
 lint-docs: lint-docs-gitlink
 lint-docs: lint-docs-man-end-blurb
 lint-docs: lint-docs-man-section-order
+lint-docs: lint-docs-manpages
 
 ifeq ($(wildcard po/Makefile),po/Makefile)
 doc-l10n install-l10n::
diff --git a/Documentation/lint-manpages.sh b/Documentation/lint-manpages.sh
new file mode 100755
index 0000000000..f720a3f3d6
--- /dev/null
+++ b/Documentation/lint-manpages.sh
@@ -0,0 +1,82 @@
+#!/usr/bin/env bash
+
+cd "$(dirname "${BASH_SOURCE[0]}")"/..
+
+extract_variable () {
+	(
+		cat Makefile
+		cat <<EOF
+print_variable:
+	\$(foreach b,\$($1),echo XXX \$(b:\$X=) YYY;)
+EOF
+	) |
+	make -f - print_variable 2>/dev/null |
+	sed -n -e 's/.*XXX \(.*\) YYY.*/\1/p'
+}
+
+check_missing_docs () {
+	for v in $BUILT_INS
+	do
+		case "$v" in
+		git-merge-octopus) continue;;
+		git-merge-ours) continue;;
+		git-merge-recursive) continue;;
+		git-merge-resolve) continue;;
+		git-merge-subtree) continue;;
+		git-fsck-objects) continue;;
+		git-init-db) continue;;
+		git-remote-*) continue;;
+		git-stage) continue;;
+		git-legacy-*) continue;;
+		git-?*--?* ) continue ;;
+		esac
+
+		if ! test -f "Documentation/$v.txt"
+		then
+			echo "no doc: $v"
+		fi
+
+		if ! sed -e '1,/^### command list/d' -e '/^#/d' command-list.txt |
+			grep -q "^$v[ 	]"
+		then
+			case "$v" in
+			git)
+				;;
+			*)
+				echo "no link: $v";;
+			esac
+		fi
+	done
+}
+
+check_extraneous_docs () {
+	local commands="$(printf "%s\n" "$ALL_COMMANDS" "$BUILT_INS" "$EXCLUDED_PROGRAMS")"
+
+	while read how cmd
+	do
+		if ! [[ $commands = *"$cmd"* ]]
+		then
+			echo "removed but $how: $cmd"
+		fi
+	done < <(
+		sed -e '1,/^### command list/d' \
+		    -e '/^#/d' \
+		    -e '/guide$/d' \
+		    -e '/interfaces$/d' \
+		    -e 's/[ 	].*//' \
+		    -e 's/^/listed /' command-list.txt
+		make -C Documentation print-man1 |
+		grep '\.txt$' |
+		sed -e 's|^|documented |' \
+		    -e 's/\.txt//'
+	)
+}
+
+BUILT_INS="$(extract_variable BUILT_INS)"
+ALL_COMMANDS="$(extract_variable ALL_COMMANDS)"
+EXCLUDED_PROGRAMS="$(extract_variable EXCLUDED_PROGRAMS)"
+
+{
+	check_missing_docs
+	check_extraneous_docs
+} | sort
diff --git a/Makefile b/Makefile
index 59d98ba688..84e2aa9686 100644
--- a/Makefile
+++ b/Makefile
@@ -3757,42 +3757,6 @@ ALL_COMMANDS += scalar
 .PHONY: check-docs
 check-docs::
 	$(MAKE) -C Documentation lint-docs
-	@(for v in $(patsubst %$X,%,$(ALL_COMMANDS)); \
-	do \
-		case "$$v" in \
-		git-merge-octopus | git-merge-ours | git-merge-recursive | \
-		git-merge-resolve | git-merge-subtree | \
-		git-fsck-objects | git-init-db | \
-		git-remote-* | git-stage | git-legacy-* | \
-		git-?*--?* ) continue ;; \
-		esac ; \
-		test -f "Documentation/$$v.txt" || \
-		echo "no doc: $$v"; \
-		sed -e '1,/^### command list/d' -e '/^#/d' command-list.txt | \
-		grep -q "^$$v[ 	]" || \
-		case "$$v" in \
-		git) ;; \
-		*) echo "no link: $$v";; \
-		esac ; \
-	done; \
-	( \
-		sed -e '1,/^### command list/d' \
-		    -e '/^#/d' \
-		    -e '/guide$$/d' \
-		    -e '/interfaces$$/d' \
-		    -e 's/[ 	].*//' \
-		    -e 's/^/listed /' command-list.txt; \
-		$(MAKE) -C Documentation print-man1 | \
-		grep '\.txt$$' | \
-		sed -e 's|^|documented |' \
-		    -e 's/\.txt//'; \
-	) | while read how cmd; \
-	do \
-		case " $(patsubst %$X,%,$(ALL_COMMANDS) $(BUILT_INS) $(EXCLUDED_PROGRAMS)) " in \
-		*" $$cmd "*)	;; \
-		*) echo "removed but $$how: $$cmd" ;; \
-		esac; \
-	done ) | sort
 
 ### Make sure built-ins do not have dups and listed in git.c
 #
-- 
2.45.2.409.g7b0defb391.dirty


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

[PATCH 2/4] Documentation/lint-manpages: bubble up errors

[Patrick Steinhardt]

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

The "lint-manpages.sh" script does not return an error in case any of
its checks fail. While this is faithful to the implementation that we
had as part of the "check-docs" target before the preceding commit, it
makes it hard to spot any violations of the rules via the corresponding
CI job, which will of course exit successfully, too.

Adapt the script to bubble up errors.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/lint-manpages.sh | 35 +++++++++++++++++++++++++++++-----
 1 file changed, 30 insertions(+), 5 deletions(-)

diff --git a/Documentation/lint-manpages.sh b/Documentation/lint-manpages.sh
index f720a3f3d6..c7ae8ee17a 100755
--- a/Documentation/lint-manpages.sh
+++ b/Documentation/lint-manpages.sh
@@ -15,6 +15,8 @@ EOF
 }
 
 check_missing_docs () {
+	local ret=0
+
 	for v in $BUILT_INS
 	do
 		case "$v" in
@@ -34,6 +36,7 @@ check_missing_docs () {
 		if ! test -f "Documentation/$v.txt"
 		then
 			echo "no doc: $v"
+			ret=1
 		fi
 
 		if ! sed -e '1,/^### command list/d' -e '/^#/d' command-list.txt |
@@ -43,20 +46,26 @@ check_missing_docs () {
 			git)
 				;;
 			*)
-				echo "no link: $v";;
+				echo "no link: $v"
+				ret=1
+				;;
 			esac
 		fi
 	done
+
+	return $ret
 }
 
 check_extraneous_docs () {
 	local commands="$(printf "%s\n" "$ALL_COMMANDS" "$BUILT_INS" "$EXCLUDED_PROGRAMS")"
+	local ret=0
 
 	while read how cmd
 	do
 		if ! [[ $commands = *"$cmd"* ]]
 		then
 			echo "removed but $how: $cmd"
+			ret=1
 		fi
 	done < <(
 		sed -e '1,/^### command list/d' \
@@ -70,13 +79,29 @@ check_extraneous_docs () {
 		sed -e 's|^|documented |' \
 		    -e 's/\.txt//'
 	)
+
+	return $ret
 }
 
 BUILT_INS="$(extract_variable BUILT_INS)"
 ALL_COMMANDS="$(extract_variable ALL_COMMANDS)"
 EXCLUDED_PROGRAMS="$(extract_variable EXCLUDED_PROGRAMS)"
 
-{
-	check_missing_docs
-	check_extraneous_docs
-} | sort
+findings=$(
+	if ! check_missing_docs
+	then
+		ret=1
+	fi
+
+	if ! check_extraneous_docs
+	then
+		ret=1
+	fi
+
+	exit $ret
+)
+ret=$?
+
+echo "$findings" | sort
+
+exit $ret
-- 
2.45.2.409.g7b0defb391.dirty


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

[PATCH 3/4] gitlab-ci: add job to run `make check-docs`

[Patrick Steinhardt]

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

Add another job to execute `make check-docs`, which lints our
documentation and makes sure that expected manpages exist. This job
mirrors the same job that we already have for GitHub Actions.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 .gitlab-ci.yml | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index f676959ca0..37b991e080 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -122,3 +122,12 @@ check-whitespace:
     - ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
   rules:
     - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
+
+documentation:
+  image: ubuntu:latest
+  variables:
+    jobname: Documentation
+  before_script:
+    - ./ci/install-dependencies.sh
+  script:
+    - ./ci/test-documentation.sh
-- 
2.45.2.409.g7b0defb391.dirty


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

[PATCH 4/4] ci/test-documentation: work around SyntaxWarning in Python 3.12

[Patrick Steinhardt]

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

In Python 3.6, unrecognized escape sequences in regular expressions
started to produce a DeprecationWarning [1]. In Python 3.12, this was
upgraded to a SyntaxWarning and will eventually be raised even further
to a SyntaxError. We indirectly hit such unrecognized escape sequences
via Asciidoc, which results in a bunch of warnings:

    $ asciidoc -o /dev/null git-cat-file.txt
    <unknown>:1: SyntaxWarning: invalid escape sequence '\S'
    <unknown>:1: SyntaxWarning: invalid escape sequence '\S'

This in turn causes our "ci/test-documentation.sh" script to fail, as it
checks that stderr of `make doc` is empty.

These escape sequences seem to be part of Asciidoc itself. In the long
term, we should probably consider dropping support for Asciidoc in favor
of Asciidoctor. Upstream also considers itself to be legacy software and
recommends to move away from it [2]:

    It is suggested that unless you specifically require the AsciiDoc.py
    toolchain, you should find a processor that handles the modern
    AsciiDoc syntax.

For now though, let's expand its lifetime a little bit more by filtering
out these new warnings. We should probably reconsider once the warnings
are upgraded to errors by Python.

[1]: https://docs.python.org/3/reference/lexical_analysis.html#string-and-bytes-literals
[2]: https://github.com/asciidoc-py/asciidoc-py/blob/6d9f76cff0dc3b7ca21bdd570200f8518464d99b/README.md#asciidocpy

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/test-documentation.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ci/test-documentation.sh b/ci/test-documentation.sh
index de41888430..02b3af3941 100755
--- a/ci/test-documentation.sh
+++ b/ci/test-documentation.sh
@@ -11,6 +11,7 @@ filter_log () {
 	    -e '/^    \* new asciidoc flags$/d' \
 	    -e '/stripped namespace before processing/d' \
 	    -e '/Attributed.*IDs for element/d' \
+	    -e '/SyntaxWarning: invalid escape sequence/d' \
 	    "$1"
 }
 
-- 
2.45.2.409.g7b0defb391.dirty


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

Re: [PATCH 1/4] Makefile: extract script to lint missing/extraneous manpages

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> +++ b/Documentation/lint-manpages.sh
> @@ -0,0 +1,82 @@
> +#!/usr/bin/env bash

I do not see much bash-ism here.  Unless absolutely needed, please
use "#!/bin/sh" instead.

Re: [PATCH 2/4] Documentation/lint-manpages: bubble up errors

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> +echo "$findings" | sort
> + ...
> +exit $ret

All make sense.

Re: [PATCH 4/4] ci/test-documentation: work around SyntaxWarning in Python 3.12

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> These escape sequences seem to be part of Asciidoc itself. In the long
> term, we should probably consider dropping support for Asciidoc in favor
> of Asciidoctor. Upstream also considers itself to be legacy software and
> recommends to move away from it [2]:
> ...
> For now though, let's expand its lifetime a little bit more by filtering
> out these new warnings. We should probably reconsider once the warnings
> are upgraded to errors by Python.

Sounds sensible.  We can throw it in to the Git 3.0 list, perhaps
;-)

Re: [PATCH 1/4] Makefile: extract script to lint missing/extraneous manpages

[Patrick Steinhardt]

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

On Tue, Jun 04, 2024 at 10:20:54PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > +++ b/Documentation/lint-manpages.sh
> > @@ -0,0 +1,82 @@
> > +#!/usr/bin/env bash
> 
> I do not see much bash-ism here.  Unless absolutely needed, please
> use "#!/bin/sh" instead.

Ah, true. I initially did have some bash-isms, but got rid of them. Will
adapt.

Patrick

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

Re: [PATCH 1/4] Makefile: extract script to lint missing/extraneous manpages

[James Liu]

On Wed Jun 5, 2024 at 3:27 PM AEST, Patrick Steinhardt wrote:
> On Tue, Jun 04, 2024 at 10:20:54PM -0700, Junio C Hamano wrote:
> > Patrick Steinhardt <ps@pks.im> writes:
> > 
> > > +++ b/Documentation/lint-manpages.sh
> > > @@ -0,0 +1,82 @@
> > > +#!/usr/bin/env bash
> > 
> > I do not see much bash-ism here.  Unless absolutely needed, please
> > use "#!/bin/sh" instead.
>
> Ah, true. I initially did have some bash-isms, but got rid of them. Will
> adapt.
>
> Patrick

It looks like the script fails to run under /bin/sh:
https://gitlab.com/gitlab-org/git/-/jobs/7021474555#L4365

Cheers,
James

Re: [PATCH 1/4] Makefile: extract script to lint missing/extraneous manpages

[Junio C Hamano]

"James Liu" <james@jamesliu.io> writes:

> On Wed Jun 5, 2024 at 3:27 PM AEST, Patrick Steinhardt wrote:
>> On Tue, Jun 04, 2024 at 10:20:54PM -0700, Junio C Hamano wrote:
>> > Patrick Steinhardt <ps@pks.im> writes:
>> > 
>> > > +++ b/Documentation/lint-manpages.sh
>> > > @@ -0,0 +1,82 @@
>> > > +#!/usr/bin/env bash
>> > 
>> > I do not see much bash-ism here.  Unless absolutely needed, please
>> > use "#!/bin/sh" instead.
>>
>> Ah, true. I initially did have some bash-isms, but got rid of them. Will
>> adapt.
>>
>> Patrick
>
> It looks like the script fails to run under /bin/sh:
> https://gitlab.com/gitlab-org/git/-/jobs/7021474555#L4365

Nice to know.  In any case, the original was an inline scriptlet in
the Makefile and should have been happily working for folks whose
/bin/sh is different from bash, so the version that is extracted out
shouldn't have to rely on bashisms.

Thanks.

[PATCH v2 0/4] Documentation: improve linting for manpage existence

[Patrick Steinhardt]

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

Hi,

this is the second version of my patch series that improves linting for
the existence and/or absence of manpages.

Changes compared to v1:

  - Converted the extracted script to be compatible with POSIX shells.

  - Fix an empty line being printed when there are no findings.

  - Silence the patched-in Makefile recipe, which otherwise leads to one
    value existing twice in the converted output.

  - Fix `check_missing_docs()` to use `ALL_COMMANDS`, not `BUILT_INS`.

Thanks!

Patrick

Patrick Steinhardt (4):
  Makefile: extract script to lint missing/extraneous manpages
  Documentation/lint-manpages: bubble up errors
  gitlab-ci: add job to run `make check-docs`
  ci/test-documentation: work around SyntaxWarning in Python 3.12

 .gitlab-ci.yml                 |   9 +++
 Documentation/Makefile         |   4 ++
 Documentation/lint-manpages.sh | 108 +++++++++++++++++++++++++++++++++
 Makefile                       |  36 -----------
 ci/test-documentation.sh       |   1 +
 5 files changed, 122 insertions(+), 36 deletions(-)
 create mode 100755 Documentation/lint-manpages.sh

Range-diff against v1:
1:  b06088a2ff ! 1:  489a6eaf2d Makefile: extract script to lint missing/extraneous manpages
    @@ Documentation/Makefile: $(LINT_DOCS_FSCK_MSGIDS): ../fsck.h fsck-msgids.txt
     
      ## Documentation/lint-manpages.sh (new) ##
     @@
    -+#!/usr/bin/env bash
    -+
    -+cd "$(dirname "${BASH_SOURCE[0]}")"/..
    ++#!/bin/sh
     +
     +extract_variable () {
     +	(
    -+		cat Makefile
    ++		cat ../Makefile
     +		cat <<EOF
     +print_variable:
    -+	\$(foreach b,\$($1),echo XXX \$(b:\$X=) YYY;)
    ++	@\$(foreach b,\$($1),echo XXX \$(b:\$X=) YYY;)
     +EOF
     +	) |
    -+	make -f - print_variable 2>/dev/null |
    ++	make -C .. -f - print_variable 2>/dev/null |
     +	sed -n -e 's/.*XXX \(.*\) YYY.*/\1/p'
     +}
     +
     +check_missing_docs () {
    -+	for v in $BUILT_INS
    ++	for v in $ALL_COMMANDS
     +	do
     +		case "$v" in
     +		git-merge-octopus) continue;;
    @@ Documentation/lint-manpages.sh (new)
     +		git-?*--?* ) continue ;;
     +		esac
     +
    -+		if ! test -f "Documentation/$v.txt"
    ++		if ! test -f "$v.txt"
     +		then
     +			echo "no doc: $v"
     +		fi
     +
    -+		if ! sed -e '1,/^### command list/d' -e '/^#/d' command-list.txt |
    ++		if ! sed -e '1,/^### command list/d' -e '/^#/d' ../command-list.txt |
     +			grep -q "^$v[ 	]"
     +		then
     +			case "$v" in
    @@ Documentation/lint-manpages.sh (new)
     +}
     +
     +check_extraneous_docs () {
    -+	local commands="$(printf "%s\n" "$ALL_COMMANDS" "$BUILT_INS" "$EXCLUDED_PROGRAMS")"
    -+
    -+	while read how cmd
    -+	do
    -+		if ! [[ $commands = *"$cmd"* ]]
    -+		then
    -+			echo "removed but $how: $cmd"
    -+		fi
    -+	done < <(
    ++	(
     +		sed -e '1,/^### command list/d' \
     +		    -e '/^#/d' \
     +		    -e '/guide$/d' \
     +		    -e '/interfaces$/d' \
     +		    -e 's/[ 	].*//' \
    -+		    -e 's/^/listed /' command-list.txt
    -+		make -C Documentation print-man1 |
    ++		    -e 's/^/listed /' ../command-list.txt
    ++		make print-man1 |
     +		grep '\.txt$' |
     +		sed -e 's|^|documented |' \
     +		    -e 's/\.txt//'
    ++	) | (
    ++		all_commands="$(printf "%s " "$ALL_COMMANDS" "$BUILT_INS" "$EXCLUDED_PROGRAMS" | tr '\n' ' ')"
    ++
    ++		while read how cmd
    ++		do
    ++			case " $all_commands " in
    ++			*" $cmd "*) ;;
    ++			*)
    ++				echo "removed but $how: $cmd";;
    ++			esac
    ++		done
     +	)
     +}
     +
2:  b39a780d33 ! 2:  9f21c305b9 Documentation/lint-manpages: bubble up errors
    @@ Commit message
     
      ## Documentation/lint-manpages.sh ##
     @@ Documentation/lint-manpages.sh: EOF
    + 	sed -n -e 's/.*XXX \(.*\) YYY.*/\1/p'
      }
      
    - check_missing_docs () {
    -+	local ret=0
    +-check_missing_docs () {
    ++check_missing_docs () (
    ++	ret=0
     +
    - 	for v in $BUILT_INS
    + 	for v in $ALL_COMMANDS
      	do
      		case "$v" in
     @@ Documentation/lint-manpages.sh: check_missing_docs () {
    - 		if ! test -f "Documentation/$v.txt"
    + 		if ! test -f "$v.txt"
      		then
      			echo "no doc: $v"
     +			ret=1
      		fi
      
    - 		if ! sed -e '1,/^### command list/d' -e '/^#/d' command-list.txt |
    + 		if ! sed -e '1,/^### command list/d' -e '/^#/d' ../command-list.txt |
     @@ Documentation/lint-manpages.sh: check_missing_docs () {
      			git)
      				;;
    @@ Documentation/lint-manpages.sh: check_missing_docs () {
      			esac
      		fi
      	done
    +-}
     +
    -+	return $ret
    - }
    ++	exit $ret
    ++)
      
      check_extraneous_docs () {
    - 	local commands="$(printf "%s\n" "$ALL_COMMANDS" "$BUILT_INS" "$EXCLUDED_PROGRAMS")"
    -+	local ret=0
    - 
    - 	while read how cmd
    - 	do
    - 		if ! [[ $commands = *"$cmd"* ]]
    - 		then
    - 			echo "removed but $how: $cmd"
    -+			ret=1
    - 		fi
    - 	done < <(
    - 		sed -e '1,/^### command list/d' \
    + 	(
     @@ Documentation/lint-manpages.sh: check_extraneous_docs () {
    - 		sed -e 's|^|documented |' \
      		    -e 's/\.txt//'
    - 	)
    + 	) | (
    + 		all_commands="$(printf "%s " "$ALL_COMMANDS" "$BUILT_INS" "$EXCLUDED_PROGRAMS" | tr '\n' ' ')"
    ++		ret=0
    + 
    + 		while read how cmd
    + 		do
    + 			case " $all_commands " in
    + 			*" $cmd "*) ;;
    + 			*)
    +-				echo "removed but $how: $cmd";;
    ++				echo "removed but $how: $cmd"
    ++				ret=1;;
    + 			esac
    + 		done
     +
    -+	return $ret
    ++		exit $ret
    + 	)
      }
      
    - BUILT_INS="$(extract_variable BUILT_INS)"
    +@@ Documentation/lint-manpages.sh: BUILT_INS="$(extract_variable BUILT_INS)"
      ALL_COMMANDS="$(extract_variable ALL_COMMANDS)"
      EXCLUDED_PROGRAMS="$(extract_variable EXCLUDED_PROGRAMS)"
      
    @@ Documentation/lint-manpages.sh: check_extraneous_docs () {
     +)
     +ret=$?
     +
    -+echo "$findings" | sort
    ++printf "%s" "$findings" | sort
     +
     +exit $ret
3:  a44d57a732 = 3:  d6d3628797 gitlab-ci: add job to run `make check-docs`
4:  c758b45282 = 4:  c13fed9ebf ci/test-documentation: work around SyntaxWarning in Python 3.12
-- 
2.45.2.409.g7b0defb391.dirty


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

[PATCH v2 1/4] Makefile: extract script to lint missing/extraneous manpages

[Patrick Steinhardt]

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

The "check-docs" target of our top-level Makefile fulfills two different
roles. For one it runs the "lint-docs" target of the "Documentation/"
Makefile. And second it performs some checks of whether there are any
manpages that are missing or extraneous via some inline scripts.

The second set of checks feels quite misplaced in the top-level Makefile
as it would fit in much better with our "lint-docs" target. Back when
the checks were introduced in 8c989ec528 (Makefile: $(MAKE) check-docs,
2006-04-13), that target did not yet exist though.

Furthermore, the script makes use of several Makefile variables which
are defined in the top-level Makefile, which makes it hard to access
their contents from elsewhere. There is a trick though that we already
use in "check-builtins.sh" to gain access: we can create an ad-hoc
Makefile that has an extra target to print those variables.

Pull out the script into a separate "lint-manpages.sh" script by using
that trick. Wire up that script via the "lint-docs" target. For one,
normal shell scripts are way easier to reason about than those which are
embedded in a Makefile. Second, it allows one to easily execute the
script standalone without any of the other checks.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/Makefile         |  4 ++
 Documentation/lint-manpages.sh | 83 ++++++++++++++++++++++++++++++++++
 Makefile                       | 36 ---------------
 3 files changed, 87 insertions(+), 36 deletions(-)
 create mode 100755 Documentation/lint-manpages.sh

diff --git a/Documentation/Makefile b/Documentation/Makefile
index a04da672c6..a3868a462f 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -485,12 +485,16 @@ $(LINT_DOCS_FSCK_MSGIDS): ../fsck.h fsck-msgids.txt
 
 lint-docs-fsck-msgids: $(LINT_DOCS_FSCK_MSGIDS)
 
+lint-docs-manpages:
+	$(QUIET_GEN)./lint-manpages.sh
+
 ## Lint: list of targets above
 .PHONY: lint-docs
 lint-docs: lint-docs-fsck-msgids
 lint-docs: lint-docs-gitlink
 lint-docs: lint-docs-man-end-blurb
 lint-docs: lint-docs-man-section-order
+lint-docs: lint-docs-manpages
 
 ifeq ($(wildcard po/Makefile),po/Makefile)
 doc-l10n install-l10n::
diff --git a/Documentation/lint-manpages.sh b/Documentation/lint-manpages.sh
new file mode 100755
index 0000000000..0abb4e0b4c
--- /dev/null
+++ b/Documentation/lint-manpages.sh
@@ -0,0 +1,83 @@
+#!/bin/sh
+
+extract_variable () {
+	(
+		cat ../Makefile
+		cat <<EOF
+print_variable:
+	@\$(foreach b,\$($1),echo XXX \$(b:\$X=) YYY;)
+EOF
+	) |
+	make -C .. -f - print_variable 2>/dev/null |
+	sed -n -e 's/.*XXX \(.*\) YYY.*/\1/p'
+}
+
+check_missing_docs () {
+	for v in $ALL_COMMANDS
+	do
+		case "$v" in
+		git-merge-octopus) continue;;
+		git-merge-ours) continue;;
+		git-merge-recursive) continue;;
+		git-merge-resolve) continue;;
+		git-merge-subtree) continue;;
+		git-fsck-objects) continue;;
+		git-init-db) continue;;
+		git-remote-*) continue;;
+		git-stage) continue;;
+		git-legacy-*) continue;;
+		git-?*--?* ) continue ;;
+		esac
+
+		if ! test -f "$v.txt"
+		then
+			echo "no doc: $v"
+		fi
+
+		if ! sed -e '1,/^### command list/d' -e '/^#/d' ../command-list.txt |
+			grep -q "^$v[ 	]"
+		then
+			case "$v" in
+			git)
+				;;
+			*)
+				echo "no link: $v";;
+			esac
+		fi
+	done
+}
+
+check_extraneous_docs () {
+	(
+		sed -e '1,/^### command list/d' \
+		    -e '/^#/d' \
+		    -e '/guide$/d' \
+		    -e '/interfaces$/d' \
+		    -e 's/[ 	].*//' \
+		    -e 's/^/listed /' ../command-list.txt
+		make print-man1 |
+		grep '\.txt$' |
+		sed -e 's|^|documented |' \
+		    -e 's/\.txt//'
+	) | (
+		all_commands="$(printf "%s " "$ALL_COMMANDS" "$BUILT_INS" "$EXCLUDED_PROGRAMS" | tr '\n' ' ')"
+
+		while read how cmd
+		do
+			case " $all_commands " in
+			*" $cmd "*) ;;
+			*)
+				echo "removed but $how: $cmd";;
+			esac
+		done
+	)
+}
+
+BUILT_INS="$(extract_variable BUILT_INS)"
+ALL_COMMANDS="$(extract_variable ALL_COMMANDS)"
+EXCLUDED_PROGRAMS="$(extract_variable EXCLUDED_PROGRAMS)"
+
+{
+	check_missing_docs
+	check_extraneous_docs
+} | sort
diff --git a/Makefile b/Makefile
index 59d98ba688..84e2aa9686 100644
--- a/Makefile
+++ b/Makefile
@@ -3757,42 +3757,6 @@ ALL_COMMANDS += scalar
 .PHONY: check-docs
 check-docs::
 	$(MAKE) -C Documentation lint-docs
-	@(for v in $(patsubst %$X,%,$(ALL_COMMANDS)); \
-	do \
-		case "$$v" in \
-		git-merge-octopus | git-merge-ours | git-merge-recursive | \
-		git-merge-resolve | git-merge-subtree | \
-		git-fsck-objects | git-init-db | \
-		git-remote-* | git-stage | git-legacy-* | \
-		git-?*--?* ) continue ;; \
-		esac ; \
-		test -f "Documentation/$$v.txt" || \
-		echo "no doc: $$v"; \
-		sed -e '1,/^### command list/d' -e '/^#/d' command-list.txt | \
-		grep -q "^$$v[ 	]" || \
-		case "$$v" in \
-		git) ;; \
-		*) echo "no link: $$v";; \
-		esac ; \
-	done; \
-	( \
-		sed -e '1,/^### command list/d' \
-		    -e '/^#/d' \
-		    -e '/guide$$/d' \
-		    -e '/interfaces$$/d' \
-		    -e 's/[ 	].*//' \
-		    -e 's/^/listed /' command-list.txt; \
-		$(MAKE) -C Documentation print-man1 | \
-		grep '\.txt$$' | \
-		sed -e 's|^|documented |' \
-		    -e 's/\.txt//'; \
-	) | while read how cmd; \
-	do \
-		case " $(patsubst %$X,%,$(ALL_COMMANDS) $(BUILT_INS) $(EXCLUDED_PROGRAMS)) " in \
-		*" $$cmd "*)	;; \
-		*) echo "removed but $$how: $$cmd" ;; \
-		esac; \
-	done ) | sort
 
 ### Make sure built-ins do not have dups and listed in git.c
 #
-- 
2.45.2.409.g7b0defb391.dirty


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

[PATCH v2 2/4] Documentation/lint-manpages: bubble up errors

[Patrick Steinhardt]

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

The "lint-manpages.sh" script does not return an error in case any of
its checks fail. While this is faithful to the implementation that we
had as part of the "check-docs" target before the preceding commit, it
makes it hard to spot any violations of the rules via the corresponding
CI job, which will of course exit successfully, too.

Adapt the script to bubble up errors.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/lint-manpages.sh | 41 +++++++++++++++++++++++++++-------
 1 file changed, 33 insertions(+), 8 deletions(-)

diff --git a/Documentation/lint-manpages.sh b/Documentation/lint-manpages.sh
index 0abb4e0b4c..92cfc0a15a 100755
--- a/Documentation/lint-manpages.sh
+++ b/Documentation/lint-manpages.sh
@@ -12,7 +12,9 @@ EOF
 	sed -n -e 's/.*XXX \(.*\) YYY.*/\1/p'
 }
 
-check_missing_docs () {
+check_missing_docs () (
+	ret=0
+
 	for v in $ALL_COMMANDS
 	do
 		case "$v" in
@@ -32,6 +34,7 @@ check_missing_docs () {
 		if ! test -f "$v.txt"
 		then
 			echo "no doc: $v"
+			ret=1
 		fi
 
 		if ! sed -e '1,/^### command list/d' -e '/^#/d' ../command-list.txt |
@@ -41,11 +44,15 @@ check_missing_docs () {
 			git)
 				;;
 			*)
-				echo "no link: $v";;
+				echo "no link: $v"
+				ret=1
+				;;
 			esac
 		fi
 	done
-}
+
+	exit $ret
+)
 
 check_extraneous_docs () {
 	(
@@ -61,15 +68,19 @@ check_extraneous_docs () {
 		    -e 's/\.txt//'
 	) | (
 		all_commands="$(printf "%s " "$ALL_COMMANDS" "$BUILT_INS" "$EXCLUDED_PROGRAMS" | tr '\n' ' ')"
+		ret=0
 
 		while read how cmd
 		do
 			case " $all_commands " in
 			*" $cmd "*) ;;
 			*)
-				echo "removed but $how: $cmd";;
+				echo "removed but $how: $cmd"
+				ret=1;;
 			esac
 		done
+
+		exit $ret
 	)
 }
 
@@ -77,7 +88,21 @@ BUILT_INS="$(extract_variable BUILT_INS)"
 ALL_COMMANDS="$(extract_variable ALL_COMMANDS)"
 EXCLUDED_PROGRAMS="$(extract_variable EXCLUDED_PROGRAMS)"
 
-{
-	check_missing_docs
-	check_extraneous_docs
-} | sort
+findings=$(
+	if ! check_missing_docs
+	then
+		ret=1
+	fi
+
+	if ! check_extraneous_docs
+	then
+		ret=1
+	fi
+
+	exit $ret
+)
+ret=$?
+
+printf "%s" "$findings" | sort
+
+exit $ret
-- 
2.45.2.409.g7b0defb391.dirty


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

[PATCH v2 3/4] gitlab-ci: add job to run `make check-docs`

[Patrick Steinhardt]

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

Add another job to execute `make check-docs`, which lints our
documentation and makes sure that expected manpages exist. This job
mirrors the same job that we already have for GitHub Actions.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 .gitlab-ci.yml | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index f676959ca0..37b991e080 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -122,3 +122,12 @@ check-whitespace:
     - ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
   rules:
     - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
+
+documentation:
+  image: ubuntu:latest
+  variables:
+    jobname: Documentation
+  before_script:
+    - ./ci/install-dependencies.sh
+  script:
+    - ./ci/test-documentation.sh
-- 
2.45.2.409.g7b0defb391.dirty


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

[PATCH v2 4/4] ci/test-documentation: work around SyntaxWarning in Python 3.12

[Patrick Steinhardt]

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

In Python 3.6, unrecognized escape sequences in regular expressions
started to produce a DeprecationWarning [1]. In Python 3.12, this was
upgraded to a SyntaxWarning and will eventually be raised even further
to a SyntaxError. We indirectly hit such unrecognized escape sequences
via Asciidoc, which results in a bunch of warnings:

    $ asciidoc -o /dev/null git-cat-file.txt
    <unknown>:1: SyntaxWarning: invalid escape sequence '\S'
    <unknown>:1: SyntaxWarning: invalid escape sequence '\S'

This in turn causes our "ci/test-documentation.sh" script to fail, as it
checks that stderr of `make doc` is empty.

These escape sequences seem to be part of Asciidoc itself. In the long
term, we should probably consider dropping support for Asciidoc in favor
of Asciidoctor. Upstream also considers itself to be legacy software and
recommends to move away from it [2]:

    It is suggested that unless you specifically require the AsciiDoc.py
    toolchain, you should find a processor that handles the modern
    AsciiDoc syntax.

For now though, let's expand its lifetime a little bit more by filtering
out these new warnings. We should probably reconsider once the warnings
are upgraded to errors by Python.

[1]: https://docs.python.org/3/reference/lexical_analysis.html#string-and-bytes-literals
[2]: https://github.com/asciidoc-py/asciidoc-py/blob/6d9f76cff0dc3b7ca21bdd570200f8518464d99b/README.md#asciidocpy

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/test-documentation.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ci/test-documentation.sh b/ci/test-documentation.sh
index de41888430..02b3af3941 100755
--- a/ci/test-documentation.sh
+++ b/ci/test-documentation.sh
@@ -11,6 +11,7 @@ filter_log () {
 	    -e '/^    \* new asciidoc flags$/d' \
 	    -e '/stripped namespace before processing/d' \
 	    -e '/Attributed.*IDs for element/d' \
+	    -e '/SyntaxWarning: invalid escape sequence/d' \
 	    "$1"
 }
 
-- 
2.45.2.409.g7b0defb391.dirty


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

Re: [PATCH v2 0/4] Documentation: improve linting for manpage existence

[James Liu]

On Thu Jun 6, 2024 at 6:00 PM AEST, Patrick Steinhardt wrote:
> Hi,
>
> this is the second version of my patch series that improves linting for
> the existence and/or absence of manpages.
>
> Changes compared to v1:
>
>   - Converted the extracted script to be compatible with POSIX shells.
>
>   - Fix an empty line being printed when there are no findings.
>
>   - Silence the patched-in Makefile recipe, which otherwise leads to one
>     value existing twice in the converted output.
>
>   - Fix `check_missing_docs()` to use `ALL_COMMANDS`, not `BUILT_INS`.
>
> Thanks!
>
> Patrick

Thanks Patrick, this looks good to me! Learnt a few shell tricks too.

Cheers,
James

Re: [PATCH v2 1/4] Makefile: extract script to lint missing/extraneous manpages

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> The "check-docs" target of our top-level Makefile fulfills two different
> roles. For one it runs the "lint-docs" target of the "Documentation/"
> Makefile. And second it performs some checks of whether there are any
> manpages that are missing or extraneous via some inline scripts.

Yes.

> The second set of checks feels quite misplaced in the top-level Makefile
> as it would fit in much better with our "lint-docs" target. Back when
> the checks were introduced in 8c989ec528 (Makefile: $(MAKE) check-docs,
> 2006-04-13), that target did not yet exist though.

Correct.

> Furthermore, the script makes use of several Makefile variables which
> are defined in the top-level Makefile, which makes it hard to access
> their contents from elsewhere. There is a trick though that we already
> use in "check-builtins.sh" to gain access: we can create an ad-hoc
> Makefile that has an extra target to print those variables.

Yes.

> Pull out the script into a separate "lint-manpages.sh" script by using
> that trick. Wire up that script via the "lint-docs" target. For one,
> normal shell scripts are way easier to reason about than those which are
> embedded in a Makefile. Second, it allows one to easily execute the
> script standalone without any of the other checks.

Nicely done.  We might want to stop doing "make print-man1" in the
lint script, remove "print-man1" target from Documentation/Makefile,
and instead extract MAN1_TXT using the same "it is unlikely to have
an output line enclosing something in between XXX and YYY" trick,
but it may not be worth it.

> +check_missing_docs () {
> +...
> +		git-merge-octopus) continue;;
> +		git-merge-ours) continue;;
> +		git-merge-recursive) continue;;
> +		git-merge-resolve) continue;;
> +		git-merge-subtree) continue;;
> +		git-fsck-objects) continue;;
> +		git-init-db) continue;;
> +		git-remote-*) continue;;
> +		git-stage) continue;;
> +		git-legacy-*) continue;;
> +		git-?*--?* ) continue ;;

This looks easier to maintain than the original by listing one
pattern per line.  Again, nicely done.