Re: [PATCH v4 1/2] Documentation: fix linkgit references

On Wed, May 04, 2016 at 02:34:23PM -0700, Junio C Hamano wrote:

Third time's a charm, perhaps?
 
-- >8 --
Subject: [PATCH] ci: validate "gitlink:" in documentation

It is easy to add incorrect "linkgit:<page>[<section>]" references
to our documentation suite.  Catch these common classes of errors:

 * Referring to Documentation/<page>.txt that does not exist.

 * Referring to a <page> outside the Git suite.  In general, <page>
   must begin with "git".

 * Listing the manual <section> incorrectly.  The first line of the
   Documentation/<page>.txt must end with "(<section>)".

with a new script "ci/lint-gitlink", and drive it from "make check-docs".

Signed-off-by: Junio C Hamano <redacted>
---

This looks good to me. Two minor nits that may not be worth addressing:

+lint-docs::
+	$(QUIET_LINT)$(foreach txt,$(patsubst %.html,%.txt,$(DOC_HTML)), \
+	../ci/lint-gitlink $(txt))
+

This dependency feels funny. Wouldn't CI want to invoke this as:

  make -C Documentation lint-docs

IOW, Documentation owns the script (just like t/ owns its own lint
scripts like check-non-portable-shell.pl), and CI always just triggers
the make-driven checks, just as a normal developer would?

+sub lint {
+	my ($file) = @_;
+	open my $fh, "<", $file
+		or return;
+	while (<$fh>) {
+		my $where = "$file:$.";
[... actual linting of line ...]
+}
+
+for (@ARGV) {
+	lint($_);
+}

The usual perl way here would be:

  while(<>) {
	my $where = "$ARGV:$.";
	... actual linting of line ...
  }

where "<>" automagically reads from files on the command-line or stdin
if none were specified (and sets $ARGV as appropriate). But maybe you
prefer not to handle the stdin case (it is a benefit sometimes, but an
annoyance if you accidentally end up with an empty file list).

-Peff