Re: [PATCH 11/67] trace: use strbuf for quote_crnl output
On Wed, Sep 16, 2015 at 06:31:14AM -0400, Jeff King wrote:
Show 17 quoted lines
> On Tue, Sep 15, 2015 at 08:55:58PM -0400, Eric Sunshine wrote:
> > On Tue, Sep 15, 2015 at 11:28 AM, Jeff King <peff@peff.net> wrote:
> > > static const char *quote_crnl(const char *path)
> > > {
> > > - static char new_path[PATH_MAX];
> > > + static struct strbuf new_path = STRBUF_INIT;
> > > const char *p2 = path;
> > > - char *p1 = new_path;
> >
> > It's a little sad that this leaves a variable named 'p2' when there is
> > no corresponding 'p1'. Would this deserve a cleanup patch which
> > renames 'p2' to 'p' or do we not care enough?
>
> Yeah, you're right. The original had symmetry in p1 and p2, in that it
> moved them forward together. Now that symmetry is gone, and I wonder if
> the simplest cleanup is to just drop "p2" altogether and advance the
> "path" source pointer? Like this:Works for me. The diff is a bit noisier, but the final result feels clean.
Show 66 quoted lines
> -- >8 --
> Subject: [PATCH] trace: use strbuf for quote_crnl output
>
> When we output GIT_TRACE_SETUP paths, we quote any
> meta-characters. But our buffer to hold the result is only
> PATH_MAX bytes, and we could double the size of the input
> path (if every character needs quoting). We could use a
> 2*PATH_MAX buffer, if we assume the input will never be more
> than PATH_MAX. But it's easier still to just switch to a
> strbuf and not worry about whether the input can exceed
> PATH_MAX or not.
>
> The original copied the "p2" pointer to "p1", advancing
> both. Since this gets rid of "p1", let's also drop "p2",
> whose name is now confusing. We can just advance the
> original "path" pointer.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> trace.c | 23 +++++++++++------------
> 1 file changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/trace.c b/trace.c
> index 7393926..4aeea60 100644
> --- a/trace.c
> +++ b/trace.c
> @@ -277,25 +277,24 @@ void trace_performance_fl(const char *file, int line, uint64_t nanos,
>
> static const char *quote_crnl(const char *path)
> {
> - static char new_path[PATH_MAX];
> - const char *p2 = path;
> - char *p1 = new_path;
> + static struct strbuf new_path = STRBUF_INIT;
>
> if (!path)
> return NULL;
>
> - while (*p2) {
> - switch (*p2) {
> - case '\\': *p1++ = '\\'; *p1++ = '\\'; break;
> - case '\n': *p1++ = '\\'; *p1++ = 'n'; break;
> - case '\r': *p1++ = '\\'; *p1++ = 'r'; break;
> + strbuf_reset(&new_path);
> +
> + while (*path) {
> + switch (*path) {
> + case '\\': strbuf_addstr(&new_path, "\\\\"); break;
> + case '\n': strbuf_addstr(&new_path, "\\n"); break;
> + case '\r': strbuf_addstr(&new_path, "\\r"); break;
> default:
> - *p1++ = *p2;
> + strbuf_addch(&new_path, *path);
> }
> - p2++;
> + path++;
> }
> - *p1 = '\0';
> - return new_path;
> + return new_path.buf;
> }
>
> /* FIXME: move prefix to startup_info struct and get rid of this arg */
> --
> 2.6.0.rc2.408.ga2926b9
> Eric Sunshine <sunshine@sunshineco.com>