diff mbox series

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

Message ID b06088a2ff65a3455f0f5db2a9b752901f2af14b.1717564310.git.ps@pks.im (mailing list archive)
State New
Headers show
Series Documentation: improve linting of manpage existence | expand

Commit Message

Patrick Steinhardt June 5, 2024, 5:16 a.m. UTC
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

Comments

Junio C Hamano June 5, 2024, 5:20 a.m. UTC | #1
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 June 5, 2024, 5:27 a.m. UTC | #2
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
James Liu June 6, 2024, 6:03 a.m. UTC | #3
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
Junio C Hamano June 6, 2024, 6:12 a.m. UTC | #4
"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.
diff mbox series

Patch

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
 #