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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]