Re: [PATCH 1/1] check-docs: fix for setups where executables have an extension
From: Johannes Schindelin <hidden>
Date: 2019-03-25 21:08:59
Hi Junio, On Sun, 24 Mar 2019, Junio C Hamano wrote:
Johannes Schindelin [off-list ref] writes:quoted
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.
You are right, I should have root-caused it, it had a funny smell to it. Turns out that we should not just skip those lines with a single token, but instead fix the bug that prevents the `how` variable to be set to `documented` in those cases, as had been intended all along. I fixed the commit, and accompanied it with a bug fix for this, plus for the fall-out bugs that had been hidden by this bug. Ciao, Dscho