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

Reply via email to