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

[Christian Couder <chriscool@tuxfamily.org>]

From: Jeff King <peff@peff.net>
Subject: Re: [PATCH 15/15] commit: record buffer length in cache
Date: Tue, 10 Jun 2014 16:33:49 -0400

> On Tue, Jun 10, 2014 at 01:27:13AM -0400, Jeff King wrote:
> 
>> > 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.
> 
> I was trying to avoid cluttering up the function with the extra variable
> definitions. I ended up with the change below, which I think is clear,
> correct, and pushes the complexity outside of the function.

Yeah, great!
 
> diff --git a/builtin/blame.c b/builtin/blame.c
> index cde19eb..53f43ab 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -2266,6 +2266,18 @@ static void append_merge_parents(struct commit_list **tail)
>  }
>  
>  /*
> + * This isn't as simple as passing sb->buf and sb->len, because we
> + * want to transfer ownership of the buffer to the commit (so we
> + * must use detach).
> + */
> +static void set_commit_buffer_from_strbuf(struct commit *c, struct strbuf *sb)
> +{
> +	size_t len;
> +	void *buf = strbuf_detach(sb, &len);
> +	set_commit_buffer(c, buf, len);
> +}
> +
> +/*
>   * Prepare a dummy commit that represents the work tree (or staged) item.
>   * Note that annotating work tree item never works in the reverse.
>   */
> @@ -2313,7 +2325,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_from_strbuf(commit, &msg);
>  
>  	if (!contents_from || strcmp("-", contents_from)) {
>  		struct stat st;

Thanks,
Christian.