Re: [PATCH 1/1] check-docs: fix for setups where executables have an extension

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 24, 2019, 12:54 UTC
Message-ID: <xmqqbm20ph45.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <nycvar.QRO.7.76.6.1903241055230.45@tvgsbejvaqbjf.bet>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 12 quoted lines

> Sharp eyes, and a *very* good point. The double space is actually required
> for this patch to work as intended. I added the following explanation to
> the commit message:
>
>     Note that `$(ALL_COMMANDS)` starts with a space, and that is rather
>     crucial for the `while read how cmd` loop: some of the input lines do
>     not actually have the form `<how> <cmd>`, but only `<cmd>`, therefore
>     `$cmd` evaluates to the empty string, and when we are looking for
>     `* $cmd *` in ` $(ALL_COMMANDS)`, we do find it because the latter
>     starts with a double space.
>     In other words, the double space helps us skip the lines that list
>     only a command.

That sounds more like a bug in the existing set-up even before your patch (in fact, these lines are probably from 2007 or so, long before you started touching our top-level Makefile heavily). If we want to ignore lines with only one token, I'd rather see it explicitly done, e.g.

	... generate data ... |
	while read how cmd
	do
		# skip a line with a single token
		case "$cmd" in "") continue ;; esac

		# is $cmd part of the known command list?
		case " $(ALL_COMMANDS) " in
		*" $cmd "*)	;; # skip
		*)	echo ... warning ... ;;
		esac
	done

instead of relying on "if $cmd is empty, then SP + $cmd + SP makes double space, so let's have double space somewhere in the list of known commands" cuteness.

Thanks.