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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Mar 24, 2019, 10:02 UTC
Message-ID: <nycvar.QRO.7.76.6.1903241055230.45@tvgsbejvaqbjf.bet>
In-Reply-To: <20190322184347.GC12155@Taylors-MacBook-Pro.local>

Hi Taylor,

On Fri, 22 Mar 2019, Taylor Blau wrote:

Show 34 quoted lines

> On Wed, Mar 13, 2019 at 04:56:53AM -0700, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > On Windows, for example, executables (must) have the extension `.exe`.
> > Our `check-docs` target was not prepared for that.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  Makefile | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index 537493822b..df56bf0cd1 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -3074,7 +3074,7 @@ ALL_COMMANDS += git-gui git-citool
> >  .PHONY: check-docs
> >  check-docs::
> >  	$(MAKE) -C Documentation lint-docs
> > -	@(for v in $(ALL_COMMANDS); \
> > +	@(for v in $(patsubst %$X,%,$(ALL_COMMANDS)); \
> >  	do \
> >  		case "$$v" in \
> >  		git-merge-octopus | git-merge-ours | git-merge-recursive | \
> > @@ -3103,7 +3103,7 @@ check-docs::
> >  		    -e 's/\.txt//'; \
> >  	) | while read how cmd; \
> >  	do \
> > -		case " $(ALL_COMMANDS) " in \
> > +		case "  $(patsubst %$X,%,$(ALL_COMMANDS)) " in \
>
> I'm a little late to reading this thread, but I was curious why there
> are now two spaces around the patsubst as opposed to the one around
> ALL_COMMANDS?

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.

    But in this patch, we want to transform `$(ALL_COMMANDS)` via
    `$(patsubst ...)` which swallows that double space. To help that, we
    prefix the result with *two* spaces instead of just one.

Do you think that is good enough?

Oh, and can I ask you to review *all* of my patches in the future?

Thanks, Dscho