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

From: Jeff King <peff@peff.net>
Date: May 4, 2016, 21:44 UTC
Message-ID: <20160504214412.GA25237@sigill.intra.peff.net>
In-Reply-To: <xmqq4madfqxs.fsf@gitster.mtv.corp.google.com>

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

Show 20 quoted lines

> 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?

Show 12 quoted lines

> +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