Re: [PATCH] coccicheck: optionally batch spatch invocations

From: Denton Liu <liu.denton@gmail.com>
To: Jeff King <peff@peff.net>
Cc: "Junio C Hamano" <gitster@pobox.com>,
	"Duy Nguyen" <pclouds@gmail.com>,
	"Jacob Keller" <jacob.keller@gmail.com>,
	"SZEDER Gábor" <szeder.dev@gmail.com>,
	"Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
	"Git Mailing List" <git@vger.kernel.org>,
	"Emily Shaffer" <emilyshaffer@google.com>,
	"Thomas Gummerer" <t.gummerer@gmail.com>
Subject: Re: [PATCH] coccicheck: optionally batch spatch invocations
Date: Wed, 8 May 2019 08:36:10 -0400	[thread overview]
Message-ID: <20190508123610.GA19502@archbookpro.localdomain> (raw)
In-Reply-To: <20190508070754.GA9195@sigill.intra.peff.net>

Hi Jeff,

On Wed, May 08, 2019 at 03:07:54AM -0400, Jeff King wrote:
> On Tue, May 07, 2019 at 01:52:32PM +0900, Junio C Hamano wrote:
> 
> > Jeff King <peff@peff.net> writes:
> > 
> > > Yes, 2^31-1 is probably a better number, but it's harder to write out. :)
> > >
> > > Here's what a patch might look like to implement "0". By still using
> > > xargs in the unlimited code path, it's not too bad. I dunno.
> > 
> > As somebody who is too used to run "diff -U999" and be happy, I
> > cannot claim that I care enough, but the result does not look
> > too bad.
> 
> OK. With two "not too bad" comments (plus my own similar feeling), let's
> just do it. Here it is as a patch on top. It can also be squashed into
> the tip of jk/cocci-batch, but then we should probably s/999/0/ in the
> commit message. :)
> 
> -- >8 --
> Subject: [PATCH] coccicheck: make batch size of 0 mean "unlimited"
> 
> If you have the memory to handle it, the ideal case is to run a single
> spatch invocation with all of the source files. But the only way to do
> so now is to pick an arbitrarily large batch size. Let's make "0" do
> this, which is a little friendlier (and doesn't otherwise have a useful
> meaning).
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  Makefile | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/Makefile b/Makefile
> index daba958b8f..9cea614523 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1176,6 +1176,7 @@ SP_EXTRA_FLAGS =
>  
>  # For the 'coccicheck' target; setting SPATCH_BATCH_SIZE higher will
>  # usually result in less CPU usage at the cost of higher peak memory.
> +# Setting it to 0 will feed all files in a single spatch invocation.
>  SPATCH_FLAGS = --all-includes --patch .
>  SPATCH_BATCH_SIZE = 1
>  
> @@ -2792,7 +2793,12 @@ endif
>  
>  %.cocci.patch: %.cocci $(COCCI_SOURCES)
>  	@echo '    ' SPATCH $<; \
> -	if ! echo $(COCCI_SOURCES) | xargs -n $(SPATCH_BATCH_SIZE) \
> +	if test $(SPATCH_BATCH_SIZE) = 0; then \
> +		limit=; \
> +	else \
> +		limit='-n $(SPATCH_BATCH_SIZE)'; \
> +	fi; \

Could we pull `limit` out of the recipe and into a make variable? You
mentioned earlier that you wanted to do this but it was too complicated
but now that it's written like this, it seem like it'd be pretty easy to
do.

> +	if ! echo $(COCCI_SOURCES) | xargs $$limit \
>  		$(SPATCH) --sp-file $< $(SPATCH_FLAGS) \
>  		>$@+ 2>$@.log; \
>  	then \
> -- 
> 2.21.0.1314.g224b191707
> 