Re: [PATCH] revision: replace "struct cmdline_pathspec" with argv_array

2017-09-20 Thread Junio C Hamano
Jonathan Nieder writes: > diff --git a/pathspec.c b/pathspec.c > index e2a23ebc96..cdefdc7cc0 100644 > --- a/pathspec.c > +++ b/pathspec.c > @@ -526,10 +526,6 @@ static void NORETURN unsupported_magic(const char > *pattern, > pattern, sb.buf); > } > > -/* > - *

Re: [PATCH] revision: replace "struct cmdline_pathspec" with argv_array

2017-09-20 Thread Jeff King
On Wed, Sep 20, 2017 at 09:41:12PM -0700, Jonathan Nieder wrote: > > Well, no...the idea is that this is a function which leaks a bunch of > > memory, and we shouldn't have to think hard about how often its leak can > > be triggered or how severe it is. We should just fix it. > > I forgot to

Re: [PATCH] revision: replace "struct cmdline_pathspec" with argv_array

2017-09-20 Thread Jeff King
On Wed, Sep 20, 2017 at 08:49:00PM -0700, Jonathan Nieder wrote: > The patch I am replying to tightens the contract for parse_pathspec(). I disagree with this bit. In my view that was already the contract for parse_pathspec(), and it is simply poorly documented. You can see it being required

Re: [PATCH] revision: replace "struct cmdline_pathspec" with argv_array

2017-09-20 Thread Jonathan Nieder
Jeff King wrote: > On Wed, Sep 20, 2017 at 03:48:26PM -0700, Jonathan Nieder wrote: >> Jeff King wrote: >>> Subject: [PATCH] revision: replace "struct cmdline_pathspec" with argv_array >>> >>> We assemble an array of strings in a custom struct

Re: [PATCH] revision: replace "struct cmdline_pathspec" with argv_array

2017-09-20 Thread Junio C Hamano
Jeff King <p...@peff.net> writes: > Subject: [PATCH] revision: replace "struct cmdline_pathspec" with argv_array > > We assemble an array of strings in a custom struct, > NULL-terminate the result, and then pass it to > parse_pathspec(). > > But then we n

Re: [PATCH] revision: replace "struct cmdline_pathspec" with argv_array

2017-09-20 Thread Martin Ågren
On 20 September 2017 at 22:36, Jeff King wrote: > On Wed, Sep 20, 2017 at 04:25:52PM -0400, Jeff King wrote: > >> Isn't this whole thing just an argv_array, and this is argv_array_pushv? >> We even NULL-terminate it manually later on! >> >> So rather than increasing the line count

Re: [PATCH] revision: replace "struct cmdline_pathspec" with argv_array

2017-09-20 Thread Jonathan Nieder
Hi, Jeff King wrote: > But mostly I am fundamentally against using UNLEAK() in a case like > this, because it does not match either of the properties which justified > adding UNLEAK() in the first place: > > 1. We are about to exit the program, so the "leak" is only caused by > the memory

Re: [PATCH] revision: replace "struct cmdline_pathspec" with argv_array

2017-09-20 Thread Jeff King
On Wed, Sep 20, 2017 at 03:48:26PM -0700, Jonathan Nieder wrote: > > Subject: [PATCH] revision: replace "struct cmdline_pathspec" with argv_array > > > > We assemble an array of strings in a custom struct, > > NULL-terminate the result, and

Re: [PATCH] revision: replace "struct cmdline_pathspec" with argv_array

2017-09-20 Thread Jonathan Nieder
Hi, Jeff King wrote: > Subject: [PATCH] revision: replace "struct cmdline_pathspec" with argv_array > > We assemble an array of strings in a custom struct, > NULL-terminate the result, and then pass it to > parse_pathspec(). > > But then we never free the array o

[PATCH] revision: replace "struct cmdline_pathspec" with argv_array

2017-09-20 Thread Jeff King
y" without actually seeing if it was possible. At which point the patch was pretty much done. -- >8 -- Subject: [PATCH] revision: replace "struct cmdline_pathspec" with argv_array We assemble an array of strings in a custom struct, NULL-terminate the result, and then pass it to parse_p