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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Sep 20, 2017, 22:48 UTC
Message-ID: <20170920224826.GH27425@aiede.mtv.corp.google.com>
In-Reply-To: <20170920203659.xqy76bg5nfabvbfx@sigill.intra.peff.net>

Hi,

Jeff King wrote:

Show 10 quoted lines

> 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 or the individual strings
> (nor can we do the latter, as they are heap-allocated when
> they come from stdin but not when they come from the
> passed-in argv).

To be devil's advocate for a moment: why don't we use UNLEAK on the paths passed in from stdin?

It's true that there can be an unbounded number of them, but they all coexist in memory anyway. They are not generated dynamically on the fly. Being able to free them doesn't have any obvious advantage over using exit().

Except... is the idea that this allows the strings from stdin to be freed sooner, as soon as they have been parsed into a "struct pathspec"?

That sounds appealing. The only risk would be if "struct pathspec" holds on to a pointer to one of these strings.

Fortunately parse_pathspec() is careful to strdup any strings it borrows (though it is not documented to do so).

In other words, proposed changes:

 1. Could the commit message describe what effect this would have on
    maximum heap usage, if any?  (In qualitative terms is fine, though
    actual numbers would be even better if it's easy to get them.)
    That would make it easier to justify not using UNLEAK.

 2. Can parse_pathspec get a comment in pathspec.h saying that it
    defensively copies anything it needs from args so the caller is
    free to modify or free it?  That way, it should be more obvious
    to people in the future modifying parse_pathspec() that callers
    may rely on that.  (The current API comment describes argv as
    "command line arguments", which I fear would send the opposite
    message to implementors.)

Show 9 quoted lines

> Let's swap this out for an argv_array. It does the same
> thing with fewer lines of code, and it's safe to call
> argv_array_clear() at the end to avoid a memory leak.
>
> Reported-by: Martin Ågren <martin.agren@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  revision.c | 39 +++++++++++----------------------------
>  1 file changed, 11 insertions(+), 28 deletions(-)

The code looks good.

Thanks, Jonathan