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

From: Jeff King <peff@peff.net>
Date: Aug 31, 2018, 20:07 UTC
Message-ID: <20180831200748.GD5120@sigill.intra.peff.net>
In-Reply-To: <20180831063318.33373-4-sunshine@sunshineco.com>

On Fri, Aug 31, 2018 at 02:33:18AM -0400, Eric Sunshine wrote:

Show 7 quoted lines

> doc-diff creates a temporary working tree (git-worktree) and generates a
> bunch of temporary files which it does not remove since they act as a
> cache to speed up subsequent runs. Although doc-diff's working tree and
> generated files are not strictly build products of the Makefile (which,
> itself, never runs doc-diff), as a convenience, update "make clean" to
> clean up doc-diff's working tree and generated files along with other
> development detritus normally removed by "make clean".

Makes sense.

Show 9 quoted lines

> diff --git a/Documentation/Makefile b/Documentation/Makefile
> index a42dcfc745..26e268ae8d 100644
> --- a/Documentation/Makefile
> +++ b/Documentation/Makefile
> @@ -332,6 +332,7 @@ clean:
>  	$(RM) SubmittingPatches.txt
>  	$(RM) $(cmds_txt) $(mergetools_txt) *.made
>  	$(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 guess we are leaving it to people on exotic shells to run "$their_sh doc-diff" in the first place. That's probably OK, since it should work out of the box on most /bin/sh instances, and people on other platforms aren't that likely to even run it.

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. ;)

-Peff