Re: [PATCH 3/3] doc/Makefile: drop doc-diff worktree and temporary files on "make clean"

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Aug 31, 2018, 21:37 UTC
Message-ID: <CAPig+cSktNnaWBew02Di5BaCQAqP6kXtMyoC3=g1A9hZcT-BEw@mail.gmail.com>
In-Reply-To: <20180831200748.GD5120@sigill.intra.peff.net>

On Fri, Aug 31, 2018 at 4:07 PM Jeff King <peff@peff.net> wrote:

Show 19 quoted lines

> On Fri, Aug 31, 2018 at 02:33:18AM -0400, Eric Sunshine wrote:
> > diff --git a/Documentation/Makefile b/Documentation/Makefile
> > @@ -332,6 +332,7 @@ clean:
> >       $(RM) manpage-base-url.xsl
> > +     '$(SHELL_PATH_SQ)' ./doc-diff --clean
>
> This spelling took me by surprise. The doc-diff script itself specifies
> /bin/sh, and we do not build it, so the #! line is never replaced. [...]
>
> I don't think the script does anything complicated that would choke a
> lesser /bin/sh. But it doesn't hurt to be defensive, since this bit of
> the Makefile will be run for everyone, whether they care about doc-diff
> or not.
>
> So that all makes sense. I initially wrote this to suggest that we call
> out this subtlety in the commit message. But I see this is based on
> existing instances from ee7ec2f9de (documentation: Makefile accounts for
> SHELL_PATH setting, 2009-03-22). So maybe I am just showing my
> ignorance. ;)

Correct. I was concerned that invoking it simply as "./doc-diff --clean" could be problematic, so, knowing that the Makefile invoked other scripts in Documentation/, I mirrored their invocation. If it didn't follow existing practice of invoking the command with $(SHELL_PATH_SQ), then that would merit mention in the commit message, but as it is, the commit message is probably fine.

Thanks for the review.