coccicheck: process every source file at once
diff mbox series

Message ID 20181002191642.21504-1-jacob.e.keller@intel.com
State New
Headers show
Series
  • coccicheck: process every source file at once
Related show

Commit Message

Jacob Keller Oct. 2, 2018, 7:16 p.m. UTC
From: Jacob Keller <jacob.keller@gmail.com>

make coccicheck is used in order to apply coccinelle semantic patches,
and see if any of the transformations found within contrib/coccinelle/
can be applied to the current code base.

Pass every file to a single invocation of spatch, instead of running
spatch once per source file.

This reduces the time required to run make coccicheck by a significant
amount of time:

Prior timing of make coccicheck
  real    6m14.090s
  user    25m2.606s
  sys     1m22.919s

New timing of make coccicheck
  real    1m36.580s
  user    7m55.933s
  sys     0m18.219s

This is nearly a 4x decrease in the time required to run make
coccicheck. This is due to the overhead of restarting spatch for every
file. By processing all files at once, we can amortize this startup cost
across the total number of files, rather than paying it once per file.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 Makefile | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

Jeff King Oct. 2, 2018, 7:55 p.m. UTC | #1
On Tue, Oct 02, 2018 at 12:16:42PM -0700, Jacob Keller wrote:

> make coccicheck is used in order to apply coccinelle semantic patches,
> and see if any of the transformations found within contrib/coccinelle/
> can be applied to the current code base.
> 
> Pass every file to a single invocation of spatch, instead of running
> spatch once per source file.
> 
> This reduces the time required to run make coccicheck by a significant
> amount of time:
> 
> Prior timing of make coccicheck
>   real    6m14.090s
>   user    25m2.606s
>   sys     1m22.919s
> 
> New timing of make coccicheck
>   real    1m36.580s
>   user    7m55.933s
>   sys     0m18.219s

Yay! This is a nice result.

It's also one of the things that Julia suggested in an earlier thread.
One thing I wasn't quite sure about after digging into the various
versions (1.0.4 on Debian stable/unstable, 1.0.6 in experimental, and
pre-1.0.7 at the bleeding edge) was whether the old versions would be
similarly helped (or work at all).

I just replicated your results with 1.0.4.deb-3+b2 from Debian stable.
It's possible there are older versions floating around, but for
something developer-only like this, I think "in Debian stable" is a
reasonable enough cutoff.

> This is nearly a 4x decrease in the time required to run make
> coccicheck. This is due to the overhead of restarting spatch for every
> file. By processing all files at once, we can amortize this startup cost
> across the total number of files, rather than paying it once per file.

One minor side effect is that we lose the opportunity to parallelize
quite as much. However, I think the reduction in total CPU makes it well
worth that (and we still have 8 cocci files and growing, which should
keep at least 8 cores busy).

I think recent versions of Coccinelle will actually parallelize
internally, too, but my 1.0.4 doesn't seem to. That's probably what I
was thinking of earlier (but this is still a win even without that).

It looks like this also fixes a problem I ran into when doing the oideq
work, which is that the patch for a header file would get shown multiple
times (once for each file that includes it). So this is faster _and_
more correct. Double-yay.

> diff --git a/Makefile b/Makefile
> index df1df9db78da..b9947f3f51ec 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2715,10 +2715,8 @@ endif
>  %.cocci.patch: %.cocci $(COCCI_SOURCES)
>  	@echo '    ' SPATCH $<; \
>  	ret=0; \
> -	for f in $(COCCI_SOURCES); do \
> -		$(SPATCH) --sp-file $< $$f $(SPATCH_FLAGS) || \
> -			{ ret=$$?; break; }; \
> -	done >$@+ 2>$@.log; \
> +	( $(SPATCH) --sp-file $< $(COCCI_SOURCES) $(SPATCH_FLAGS) || \
> +		{ ret=$$?; }; ) >$@+ 2>$@.log; \

This looks pretty straight-forward. I wondered if we could get rid of
the "ret" handling, since we don't need to pass the error back out of
the loop. But it's also used for the "show the log only on error" logic
below:

>  	if test $$ret != 0; \
>  	then \
>  		cat $@.log; \

The subshell could be a {}, though, I think, but it's not that big a
deal either way.

-Peff
Jacob Keller Oct. 2, 2018, 8 p.m. UTC | #2
On Tue, Oct 2, 2018 at 12:55 PM Jeff King <peff@peff.net> wrote:
>
> On Tue, Oct 02, 2018 at 12:16:42PM -0700, Jacob Keller wrote:
>
> > make coccicheck is used in order to apply coccinelle semantic patches,
> > and see if any of the transformations found within contrib/coccinelle/
> > can be applied to the current code base.
> >
> > Pass every file to a single invocation of spatch, instead of running
> > spatch once per source file.
> >
> > This reduces the time required to run make coccicheck by a significant
> > amount of time:
> >
> > Prior timing of make coccicheck
> >   real    6m14.090s
> >   user    25m2.606s
> >   sys     1m22.919s
> >
> > New timing of make coccicheck
> >   real    1m36.580s
> >   user    7m55.933s
> >   sys     0m18.219s
>
> Yay! This is a nice result.
>
> It's also one of the things that Julia suggested in an earlier thread.
> One thing I wasn't quite sure about after digging into the various
> versions (1.0.4 on Debian stable/unstable, 1.0.6 in experimental, and
> pre-1.0.7 at the bleeding edge) was whether the old versions would be
> similarly helped (or work at all).
>
> I just replicated your results with 1.0.4.deb-3+b2 from Debian stable.
> It's possible there are older versions floating around, but for
> something developer-only like this, I think "in Debian stable" is a
> reasonable enough cutoff.
>

Good. I hadn't checked back too far, but I know support for multiple
files has existed since quite a while.

> > This is nearly a 4x decrease in the time required to run make
> > coccicheck. This is due to the overhead of restarting spatch for every
> > file. By processing all files at once, we can amortize this startup cost
> > across the total number of files, rather than paying it once per file.
>
> One minor side effect is that we lose the opportunity to parallelize
> quite as much. However, I think the reduction in total CPU makes it well
> worth that (and we still have 8 cocci files and growing, which should
> keep at least 8 cores busy).
>

I don't think we do any less than previously, because we are doing
each file in a for-loop, so those would all be serial anyways.

> I think recent versions of Coccinelle will actually parallelize
> internally, too, but my 1.0.4 doesn't seem to. That's probably what I
> was thinking of earlier (but this is still a win even without that).
>
> It looks like this also fixes a problem I ran into when doing the oideq
> work, which is that the patch for a header file would get shown multiple
> times (once for each file that includes it). So this is faster _and_
> more correct. Double-yay.
>

Woot :D

> > diff --git a/Makefile b/Makefile
> > index df1df9db78da..b9947f3f51ec 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -2715,10 +2715,8 @@ endif
> >  %.cocci.patch: %.cocci $(COCCI_SOURCES)
> >       @echo '    ' SPATCH $<; \
> >       ret=0; \
> > -     for f in $(COCCI_SOURCES); do \
> > -             $(SPATCH) --sp-file $< $$f $(SPATCH_FLAGS) || \
> > -                     { ret=$$?; break; }; \
> > -     done >$@+ 2>$@.log; \
> > +     ( $(SPATCH) --sp-file $< $(COCCI_SOURCES) $(SPATCH_FLAGS) || \
> > +             { ret=$$?; }; ) >$@+ 2>$@.log; \
>
> This looks pretty straight-forward. I wondered if we could get rid of
> the "ret" handling, since we don't need to pass the error back out of
> the loop. But it's also used for the "show the log only on error" logic
> below:
>

We could probably get rid of it by doing the spatch without an ||, and
then just save $?.

> >       if test $$ret != 0; \
> >       then \
> >               cat $@.log; \
>
> The subshell could be a {}, though, I think, but it's not that big a
> deal either way.
>
> -Peff
Jeff King Oct. 2, 2018, 8:31 p.m. UTC | #3
On Tue, Oct 02, 2018 at 01:00:21PM -0700, Jacob Keller wrote:

> > > This is nearly a 4x decrease in the time required to run make
> > > coccicheck. This is due to the overhead of restarting spatch for every
> > > file. By processing all files at once, we can amortize this startup cost
> > > across the total number of files, rather than paying it once per file.
> >
> > One minor side effect is that we lose the opportunity to parallelize
> > quite as much. However, I think the reduction in total CPU makes it well
> > worth that (and we still have 8 cocci files and growing, which should
> > keep at least 8 cores busy).
> >
> 
> I don't think we do any less than previously, because we are doing
> each file in a for-loop, so those would all be serial anyways.

Yeah, sorry to be unclear. This is a strict improvement on what was
there before. We had floated some patches to do the full NxM
parallelization, but I think this path is better because of the
reduction in actual CPU used (and if more recent versions of spatch can
parallelize internally, we'll eventually get the best of both worlds).

> > > diff --git a/Makefile b/Makefile
> > > index df1df9db78da..b9947f3f51ec 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -2715,10 +2715,8 @@ endif
> > >  %.cocci.patch: %.cocci $(COCCI_SOURCES)
> > >       @echo '    ' SPATCH $<; \
> > >       ret=0; \
> > > -     for f in $(COCCI_SOURCES); do \
> > > -             $(SPATCH) --sp-file $< $$f $(SPATCH_FLAGS) || \
> > > -                     { ret=$$?; break; }; \
> > > -     done >$@+ 2>$@.log; \
> > > +     ( $(SPATCH) --sp-file $< $(COCCI_SOURCES) $(SPATCH_FLAGS) || \
> > > +             { ret=$$?; }; ) >$@+ 2>$@.log; \
> >
> > This looks pretty straight-forward. I wondered if we could get rid of
> > the "ret" handling, since we don't need to pass the error back out of
> > the loop. But it's also used for the "show the log only on error" logic
> > below:
> >
> 
> We could probably get rid of it by doing the spatch without an ||, and
> then just save $?.

Actually, I guess we do not need to save $? at all, since we have only a
single process to care about. So even simpler:

  spatch ... 2>$@+ 2>$@.log ||
  {
	cat $@.log
	exit 1
  }
  # if we get here, we were successful
  mv $@+ $@ ;# etc

would work. That's missing all the Makefile=required backslashes and
semicolons, of course. ;)

-Peff
Jacob Keller Oct. 2, 2018, 8:58 p.m. UTC | #4
On Tue, Oct 2, 2018 at 1:31 PM Jeff King <peff@peff.net> wrote:
> Actually, I guess we do not need to save $? at all, since we have only a
> single process to care about. So even simpler:
>
>   spatch ... 2>$@+ 2>$@.log ||
>   {
>         cat $@.log
>         exit 1
>   }
>   # if we get here, we were successful
>   mv $@+ $@ ;# etc
>
> would work. That's missing all the Makefile=required backslashes and
> semicolons, of course. ;)
>

I opted to drop to just save the return, immediately after calling.
It's a bit less code change, and I think the result is as clear as the
above would be. This way we do drop the subshell, not that it matters
much in the end...

Thanks,
Jake

> -Peff
Jeff King Oct. 2, 2018, 9:08 p.m. UTC | #5
On Tue, Oct 02, 2018 at 01:58:10PM -0700, Jacob Keller wrote:

> On Tue, Oct 2, 2018 at 1:31 PM Jeff King <peff@peff.net> wrote:
> > Actually, I guess we do not need to save $? at all, since we have only a
> > single process to care about. So even simpler:
> >
> >   spatch ... 2>$@+ 2>$@.log ||
> >   {
> >         cat $@.log
> >         exit 1
> >   }
> >   # if we get here, we were successful
> >   mv $@+ $@ ;# etc
> >
> > would work. That's missing all the Makefile=required backslashes and
> > semicolons, of course. ;)
> >
> 
> I opted to drop to just save the return, immediately after calling.
> It's a bit less code change, and I think the result is as clear as the
> above would be. This way we do drop the subshell, not that it matters
> much in the end...

Yeah. To be clear, I'm fine with any of the versions discussed in this
thread. Thanks for working on this!

-Peff
SZEDER Gábor Oct. 3, 2018, 10:16 a.m. UTC | #6
On Tue, Oct 02, 2018 at 03:55:19PM -0400, Jeff King wrote:
> On Tue, Oct 02, 2018 at 12:16:42PM -0700, Jacob Keller wrote:
> 
> > make coccicheck is used in order to apply coccinelle semantic patches,
> > and see if any of the transformations found within contrib/coccinelle/
> > can be applied to the current code base.
> > 
> > Pass every file to a single invocation of spatch, instead of running
> > spatch once per source file.
> > 
> > This reduces the time required to run make coccicheck by a significant
> > amount of time:
> > 
> > Prior timing of make coccicheck
> >   real    6m14.090s
> >   user    25m2.606s
> >   sys     1m22.919s
> > 
> > New timing of make coccicheck
> >   real    1m36.580s
> >   user    7m55.933s
> >   sys     0m18.219s
> 
> Yay! This is a nice result.
> 
> It's also one of the things that Julia suggested in an earlier thread.
> One thing I wasn't quite sure about after digging into the various
> versions (1.0.4 on Debian stable/unstable, 1.0.6 in experimental, and
> pre-1.0.7 at the bleeding edge) was whether the old versions would be
> similarly helped (or work at all).
> 
> I just replicated your results with 1.0.4.deb-3+b2 from Debian stable.
> It's possible there are older versions floating around, but for
> something developer-only like this, I think "in Debian stable" is a
> reasonable enough cutoff.

Linux build jobs on Travis CI run Ubuntu Trusty 14.04 LTS, and
therefore our static analysis build job still runs 1.0.0~rc19.deb-3.

This patch appears to work fine with that version, too, though note
that I also changed the build job to don't run two parallel jobs; for
the reason see below.

> > This is nearly a 4x decrease in the time required to run make
> > coccicheck. This is due to the overhead of restarting spatch for every
> > file. By processing all files at once, we can amortize this startup cost
> > across the total number of files, rather than paying it once per file.
> 
> One minor side effect is that we lose the opportunity to parallelize
> quite as much. However, I think the reduction in total CPU makes it well
> worth that (and we still have 8 cocci files and growing, which should
> keep at least 8 cores busy).

One major side effect, however, is the drastically increased memory
usage resulting from processing all files at once.  With your patch on
top of current master:

  $ for cocci in contrib/coccinelle/*.cocci ; do command time -f 'max RSS: %MkB' make ${cocci}.patch ; done
       SPATCH contrib/coccinelle/array.cocci
  max RSS: 1537068kB
       SPATCH contrib/coccinelle/commit.cocci
  max RSS: 1510920kB
       SPATCH contrib/coccinelle/free.cocci
  max RSS: 1393712kB
       SPATCH contrib/coccinelle/object_id.cocci
       SPATCH result: contrib/coccinelle/object_id.cocci.patch
  max RSS: 1831700kB
       SPATCH contrib/coccinelle/qsort.cocci
  max RSS: 1384960kB
       SPATCH contrib/coccinelle/strbuf.cocci
  max RSS: 1395800kB
       SPATCH contrib/coccinelle/swap.cocci
  max RSS: 1393620kB
       SPATCH contrib/coccinelle/xstrdup_or_null.cocci
  max RSS: 1371716kB

Without your patch the max RSS lingers around 87048kB - 101980kB,
meaning ~15x - 18x increase

This might cause quite the surprise to developers who are used to run
'make -jN coccicheck'; my tiny laptop came to a grinding halt with
-j4.

I think this side effect should be mentioned in the commit message.

Furthermore, the above mentioned static analysis build job on Travis
CI runs two parallel jobs.  Perhaps we should be considerate and
reduce that to a single job, even if that means that he build job will
run longer.

> I think recent versions of Coccinelle will actually parallelize
> internally, too, but my 1.0.4 doesn't seem to. That's probably what I
> was thinking of earlier (but this is still a win even without that).

I think Coccinelle parallelizes only when invoked with -j <N>, but
that option is not documented in 1.0.4.  I wrote "documented" instead
of "present", because e.g.:

  $ spatch --sp-file contrib/coccinelle/swap.cocci -j 2 a*.c

doesn't abort with "unknown option" and usage, but runs for a bit and
then outputs:

  init_defs_builtins: /usr/lib/coccinelle/standard.h
  HANDLING: abspath.c advice.c alias.c alloc.c apply.c archive.c archive-tar.c archive-zip.c argv-array.c attr.c
  Fatal error: exception Sys_error("swap: No such file or directory")

Without '-j 2' the command runs just fine.  I don't know what to make
of it.

> It looks like this also fixes a problem I ran into when doing the oideq
> work, which is that the patch for a header file would get shown multiple
> times (once for each file that includes it). So this is faster _and_
> more correct. Double-yay.

In that case I used to apply the change to the header first, and then
apply the semantic patch one more time.
SZEDER Gábor Oct. 3, 2018, 3:05 p.m. UTC | #7
On Wed, Oct 03, 2018 at 12:16:58PM +0200, SZEDER Gábor wrote:
> On Tue, Oct 02, 2018 at 03:55:19PM -0400, Jeff King wrote:
> > On Tue, Oct 02, 2018 at 12:16:42PM -0700, Jacob Keller wrote:
> > 
> > > make coccicheck is used in order to apply coccinelle semantic patches,
> > > and see if any of the transformations found within contrib/coccinelle/
> > > can be applied to the current code base.
> > > 
> > > Pass every file to a single invocation of spatch, instead of running
> > > spatch once per source file.
> > > 
> > > This reduces the time required to run make coccicheck by a significant
> > > amount of time:
> > > 
> > > Prior timing of make coccicheck
> > >   real    6m14.090s
> > >   user    25m2.606s
> > >   sys     1m22.919s
> > > 
> > > New timing of make coccicheck
> > >   real    1m36.580s
> > >   user    7m55.933s
> > >   sys     0m18.219s
> > 
> > Yay! This is a nice result.
> > 
> > It's also one of the things that Julia suggested in an earlier thread.
> > One thing I wasn't quite sure about after digging into the various
> > versions (1.0.4 on Debian stable/unstable, 1.0.6 in experimental, and
> > pre-1.0.7 at the bleeding edge) was whether the old versions would be
> > similarly helped (or work at all).
> > 
> > I just replicated your results with 1.0.4.deb-3+b2 from Debian stable.
> > It's possible there are older versions floating around, but for
> > something developer-only like this, I think "in Debian stable" is a
> > reasonable enough cutoff.
> 
> Linux build jobs on Travis CI run Ubuntu Trusty 14.04 LTS, and
> therefore our static analysis build job still runs 1.0.0~rc19.deb-3.
> 
> This patch appears to work fine with that version, too,

In fact, it works finer than ever, because running 1.0.0 with this
patch on Travis CI notices a possible memmove() -> MOVE_ARRAY()
conversion:

  https://travis-ci.org/szeder/git/jobs/436542684#L576

Surprisingly, running 1.0.0 without this patch, or running 1.0.4
locally either with or without this patch doesn't notice that
memmove() call.  Presumably that's why Jonathan could kind-of "revert"
my conversion from f919ffebed (Use MOVE_ARRAY, 2018-01-22) in his
6a1a79fd14 (object: move grafts to object parser, 2018-05-15) without
anyone noticing.
Jacob Keller Oct. 3, 2018, 3:51 p.m. UTC | #8
On Wed, Oct 3, 2018 at 3:17 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
>
> On Tue, Oct 02, 2018 at 03:55:19PM -0400, Jeff King wrote:
> > On Tue, Oct 02, 2018 at 12:16:42PM -0700, Jacob Keller wrote:
> >
> > > make coccicheck is used in order to apply coccinelle semantic patches,
> > > and see if any of the transformations found within contrib/coccinelle/
> > > can be applied to the current code base.
> > >
> > > Pass every file to a single invocation of spatch, instead of running
> > > spatch once per source file.
> > >
> > > This reduces the time required to run make coccicheck by a significant
> > > amount of time:
> > >
> > > Prior timing of make coccicheck
> > >   real    6m14.090s
> > >   user    25m2.606s
> > >   sys     1m22.919s
> > >
> > > New timing of make coccicheck
> > >   real    1m36.580s
> > >   user    7m55.933s
> > >   sys     0m18.219s
> >
> > Yay! This is a nice result.
> >
> > It's also one of the things that Julia suggested in an earlier thread.
> > One thing I wasn't quite sure about after digging into the various
> > versions (1.0.4 on Debian stable/unstable, 1.0.6 in experimental, and
> > pre-1.0.7 at the bleeding edge) was whether the old versions would be
> > similarly helped (or work at all).
> >
> > I just replicated your results with 1.0.4.deb-3+b2 from Debian stable.
> > It's possible there are older versions floating around, but for
> > something developer-only like this, I think "in Debian stable" is a
> > reasonable enough cutoff.
>
> Linux build jobs on Travis CI run Ubuntu Trusty 14.04 LTS, and
> therefore our static analysis build job still runs 1.0.0~rc19.deb-3.
>
> This patch appears to work fine with that version, too, though note
> that I also changed the build job to don't run two parallel jobs; for
> the reason see below.
>
> > > This is nearly a 4x decrease in the time required to run make
> > > coccicheck. This is due to the overhead of restarting spatch for every
> > > file. By processing all files at once, we can amortize this startup cost
> > > across the total number of files, rather than paying it once per file.
> >
> > One minor side effect is that we lose the opportunity to parallelize
> > quite as much. However, I think the reduction in total CPU makes it well
> > worth that (and we still have 8 cocci files and growing, which should
> > keep at least 8 cores busy).
>
> One major side effect, however, is the drastically increased memory
> usage resulting from processing all files at once.  With your patch on
> top of current master:
>
>   $ for cocci in contrib/coccinelle/*.cocci ; do command time -f 'max RSS: %MkB' make ${cocci}.patch ; done
>        SPATCH contrib/coccinelle/array.cocci
>   max RSS: 1537068kB
>        SPATCH contrib/coccinelle/commit.cocci
>   max RSS: 1510920kB
>        SPATCH contrib/coccinelle/free.cocci
>   max RSS: 1393712kB
>        SPATCH contrib/coccinelle/object_id.cocci
>        SPATCH result: contrib/coccinelle/object_id.cocci.patch
>   max RSS: 1831700kB
>        SPATCH contrib/coccinelle/qsort.cocci
>   max RSS: 1384960kB
>        SPATCH contrib/coccinelle/strbuf.cocci
>   max RSS: 1395800kB
>        SPATCH contrib/coccinelle/swap.cocci
>   max RSS: 1393620kB
>        SPATCH contrib/coccinelle/xstrdup_or_null.cocci
>   max RSS: 1371716kB
>
> Without your patch the max RSS lingers around 87048kB - 101980kB,
> meaning ~15x - 18x increase
>

Quite a significant increase.


> This might cause quite the surprise to developers who are used to run
> 'make -jN coccicheck'; my tiny laptop came to a grinding halt with
> -j4.
>
> I think this side effect should be mentioned in the commit message.
>

Yes I agree. I hadn't considered the memory problem.

Thanks,
Jake
Jacob Keller Oct. 3, 2018, 3:52 p.m. UTC | #9
On Wed, Oct 3, 2018 at 8:05 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> In fact, it works finer than ever, because running 1.0.0 with this
> patch on Travis CI notices a possible memmove() -> MOVE_ARRAY()
> conversion:
>
>   https://travis-ci.org/szeder/git/jobs/436542684#L576
>
> Surprisingly, running 1.0.0 without this patch, or running 1.0.4
> locally either with or without this patch doesn't notice that
> memmove() call.  Presumably that's why Jonathan could kind-of "revert"
> my conversion from f919ffebed (Use MOVE_ARRAY, 2018-01-22) in his
> 6a1a79fd14 (object: move grafts to object parser, 2018-05-15) without
> anyone noticing.
>

That seems very odd...

Thanks,
Jake
Stefan Beller Oct. 3, 2018, 5:54 p.m. UTC | #10
On Wed, Oct 3, 2018 at 8:52 AM Jacob Keller <jacob.keller@gmail.com> wrote:
>
> On Wed, Oct 3, 2018 at 8:05 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> > In fact, it works finer than ever, because running 1.0.0 with this
> > patch on Travis CI notices a possible memmove() -> MOVE_ARRAY()
> > conversion:
> >
> >   https://travis-ci.org/szeder/git/jobs/436542684#L576
> >
> > Surprisingly, running 1.0.0 without this patch, or running 1.0.4
> > locally either with or without this patch doesn't notice that
> > memmove() call.  Presumably that's why Jonathan could kind-of "revert"
> > my conversion from f919ffebed (Use MOVE_ARRAY, 2018-01-22) in his
> > 6a1a79fd14 (object: move grafts to object parser, 2018-05-15) without
> > anyone noticing.
> >
>
> That seems very odd...

That looks like a bad rebase on my side as I was carrying that patch
for a while as the development of object store series took some time.

And I agree we should re-introduce the MOVE_ARRAY there.

Patch
diff mbox series

diff --git a/Makefile b/Makefile
index df1df9db78da..b9947f3f51ec 100644
--- a/Makefile
+++ b/Makefile
@@ -2715,10 +2715,8 @@  endif
 %.cocci.patch: %.cocci $(COCCI_SOURCES)
 	@echo '    ' SPATCH $<; \
 	ret=0; \
-	for f in $(COCCI_SOURCES); do \
-		$(SPATCH) --sp-file $< $$f $(SPATCH_FLAGS) || \
-			{ ret=$$?; break; }; \
-	done >$@+ 2>$@.log; \
+	( $(SPATCH) --sp-file $< $(COCCI_SOURCES) $(SPATCH_FLAGS) || \
+		{ ret=$$?; }; ) >$@+ 2>$@.log; \
 	if test $$ret != 0; \
 	then \
 		cat $@.log; \