Re: [PATCH 15/15] commit: record buffer length in cache
From: Jeff King
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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 15/15] commit: record buffer length in cache
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.
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;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 15/15] commit: record buffer length in cache
On Tue, Jun 10, 2014 at 07:12:37AM +0200, Christian Couder wrote: > From: Jeff King > > > > --- 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 15/15] commit: record buffer length in cache
From: Jeff King > > --- 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); Thanks, Christian. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
