Re: [PATCH v3 2/4] commit: replace the raw buffer with strbuf in read_graft_line

From: Jeff King <peff@peff.net>
Date: Aug 18, 2017, 06:29 UTC
Message-ID: <20170818062929.f4zitbtaeii4xiko@sigill.intra.peff.net>
In-Reply-To: <65f84c5eb94e8b6f5cbce31f56810fdb71a58bf9.1503020338.git.patryk.obara@gmail.com>

On Fri, Aug 18, 2017 at 03:59:36AM +0200, Patryk Obara wrote:

Show 15 quoted lines

> diff --git a/commit.c b/commit.c
> index 8b28415..019e733 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -134,17 +134,18 @@ int register_commit_graft(struct commit_graft *graft, int ignore_dups)
>  	return 0;
>  }
>  
> -struct commit_graft *read_graft_line(char *buf, int len)
> +struct commit_graft *read_graft_line(struct strbuf *line)
>  {
>  	/* The format is just "Commit Parent1 Parent2 ...\n" */
> -	int i;
> +	int i, len;
> +	char *buf = line->buf;

Copying a pointer to a strbuf's buffer is a dangerous habit. The strbuf is free to re-allocate the buffer under the hood during any operation it likes, potentially leaving you pointing to freed memory.

In this case it's OK because the only function you call is strbuf_rtrim(), which never reallocates. But I feel like this is setting up a maintenance trap for the next person to touch the function.

AFAICT this is only here to avoid having to s/buf/line->buf/ in the rest of the function. But I think we should just make that change (you already did in some of the spots). And IMHO we should do the same for line->len. When there are two names for the same value, it increases the chances of a bug where the two end up diverging.

Show 6 quoted lines

> -	while (len && isspace(buf[len-1]))
> -		buf[--len] = '\0';
> -	if (buf[0] == '#' || buf[0] == '\0')
> +	strbuf_rtrim(line);
> +	if (line->buf[0] == '#' || line->len == 0)
>  		return NULL;

I find it funny to look at line->buf[0] before line->len, because it means we're reading pas the end of the buffer. It's OK here because we know there's a NUL terminator, but I think short-circuiting like:

  if (!line->len || line->buf[0] == '#')

is better (I also think "!" instead of "== 0" is our usual style, but that's much less important).

-Peff