Re: [PATCH 15/15] commit: record buffer length in cache

From: Jeff King <peff@peff.net>
Date: Jun 10, 2014, 05:27 UTC
Message-ID: <20140610052713.GA2978@sigill.intra.peff.net>
In-Reply-To: <20140610.071237.852310668484562387.chriscool@tuxfamily.org>

On Tue, Jun 10, 2014 at 07:12:37AM +0200, Christian Couder wrote:

Show 17 quoted lines

> From: Jeff King <peff@peff.net>
> >
> > --- a/builtin/blame.c
> > +++ b/builtin/blame.c
> > @@ -2313,7 +2313,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,
> >  		    ident, ident, path,
> >  		    (!contents_from ? path :
> >  		     (!strcmp(contents_from, "-") ? "standard input" : contents_from)));
> > -	set_commit_buffer(commit, strbuf_detach(&msg, NULL));
> > +	set_commit_buffer(commit, msg.buf, msg.len);
> 
> I find the above strange. I would have done something like:
> 
> -	set_commit_buffer(commit, strbuf_detach(&msg, NULL));
> +	size_t size;
> +	char *buf = strbuf_detach(&msg, &size);
> +	set_commit_buffer(commit, buf, size);

It is a little strange. You can't do:

  set_commit_buffer(commit, strbuf_detach(&msg, NULL), msg.len);

because the second argument resets msg.len as a side effect. Doing it your way is longer, but perhaps is a bit more canonical. In general, one should call strbuf_detach to ensure that the buffer is allocated (and does not point to slopbuf). That's guaranteed here, because we just put contents into the buffer, but it's probably more hygienic to use the more verbose form.

-Peff