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

[Jeff King <peff@peff.net>]

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 <gitster@pobox.com>
> ---

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