threads / patch / 61586

patch, 4 partsDocumentation: improve linting of manpage existence

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

## tl;dr

18 messages between Jun 5, 2024 and Jun 7, 2024. Diffs are folded; open one to read it.

replies: 17people: 3as markdown or json

Patrick Steinhardt· Jun 5, 2024, 05:16 UTC · lore
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
Patrick Steinhardt· Jun 5, 2024, 05:16 UTC · re: Patrick Steinhardt · lore

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

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
Show changes to 3 files +86 −36

Documentation/Makefile, Documentation/lint-manpages.sh, Makefile

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
Junio C Hamano· Jun 5, 2024, 05:20 UTC · re: Patrick Steinhardt · lore

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

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.

Patrick Steinhardt· Jun 5, 2024, 05:27 UTC · re: Junio C Hamano · lore

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

On Tue, Jun 04, 2024 at 10:20:54PM -0700, Junio C Hamano wrote:
Show 8 quoted lines
> 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
James Liu· Jun 6, 2024, 06:03 UTC · re: Patrick Steinhardt · lore

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

On Wed Jun 5, 2024 at 3:27 PM AEST, Patrick Steinhardt wrote:
Show 14 quoted lines
> 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

Junio C Hamano· Jun 6, 2024, 06:12 UTC · re: James Liu · lore

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

"James Liu" <james@jamesliu.io> writes:
Show 18 quoted lines
> 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.
Patrick Steinhardt· Jun 5, 2024, 05:16 UTC · re: Patrick Steinhardt · lore

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

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(-)
Show changes to Documentation/lint-manpages.sh +30 −5
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
Patrick Steinhardt· Jun 5, 2024, 05:16 UTC · re: Patrick Steinhardt · lore

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

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(+)
Show changes to .gitlab-ci.yml +9 −0
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
Patrick Steinhardt· Jun 5, 2024, 05:17 UTC · re: Patrick Steinhardt · lore

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

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(+)
Show changes to ci/test-documentation.sh +1 −0
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
Junio C Hamano· Jun 5, 2024, 05:24 UTC · re: Patrick Steinhardt · lore

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

Patrick Steinhardt <ps@pks.im> writes:
Show 8 quoted lines
> 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 ;-)

Patrick Steinhardt· Jun 6, 2024, 08:00 UTC · re: Patrick Steinhardt · lore

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

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
Patrick Steinhardt· Jun 6, 2024, 08:01 UTC · re: Patrick Steinhardt · lore

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

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
Show changes to 3 files +87 −36

Documentation/Makefile, Documentation/lint-manpages.sh, Makefile

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
Junio C Hamano· Jun 7, 2024, 16:51 UTC · re: Patrick Steinhardt · lore

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

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.
Show 5 quoted lines
> 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.
Show 5 quoted lines
> 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.

Show 13 quoted lines
> +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.

Patrick Steinhardt· Jun 6, 2024, 08:01 UTC · re: Patrick Steinhardt · lore

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

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(-)
Show changes to Documentation/lint-manpages.sh +33 −8
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
Patrick Steinhardt· Jun 6, 2024, 08:01 UTC · re: Patrick Steinhardt · lore

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

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(+)
Show changes to .gitlab-ci.yml +9 −0
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
Patrick Steinhardt· Jun 6, 2024, 08:01 UTC · re: Patrick Steinhardt · lore

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

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(+)
Show changes to ci/test-documentation.sh +1 −0
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
James Liu· Jun 7, 2024, 00:59 UTC · re: Patrick Steinhardt · lore

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

On Thu Jun 6, 2024 at 6:00 PM AEST, Patrick Steinhardt wrote:
Show 19 quoted lines
> 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

← back to recent threads