Re: [PATCH 1/4] Improve message-id generation flow control for format-patch

On Wed, 6 Feb 2008, Pierre Habouzit wrote:

On Wed, Feb 06, 2008 at 08:31:08PM +0000, Junio C Hamano wrote:

quoted

Daniel Barkalow [off-list ref] writes:

quoted

Signed-off-by: Daniel Barkalow <redacted>
---
 builtin-log.c |   29 ++++++++++++++---------------
 1 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index dcc9f81..1f74d66 100644
--- a/builtin-log.c
+++ b/builtin-log.c

@@ -576,16 +576,19 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids, const cha
 	o2->flags = flags2;
 }
 
-static void gen_message_id(char *dest, unsigned int length, char *base)
+static void gen_message_id(struct rev_info *info, char *base)
 {
 	const char *committer = git_committer_info(IDENT_WARN_ON_NO_NAME);
 	const char *email_start = strrchr(committer, '<');
 	const char *email_end = strrchr(committer, '>');
-	if(!email_start || !email_end || email_start > email_end - 1)
+	struct strbuf buf;
+	if (!email_start || !email_end || email_start > email_end - 1)
 		die("Could not extract email from committer identity.");
-	snprintf(dest, length, "%s.%lu.git.%.*s", base,
-		 (unsigned long) time(NULL),
-		 (int)(email_end - email_start - 1), email_start + 1);
+	strbuf_init(&buf, 0);
+	strbuf_addf(&buf, "%s.%lu.git.%.*s", base,
+		    (unsigned long) time(NULL),
+		    (int)(email_end - email_start - 1), email_start + 1);
+	info->message_id = buf.buf;

I wonder how the rule established by b315c5c (strbuf change: be
sure ->buf is never ever NULL) and at the beginning of strbuf.h
applies here.  I think the current implementation of strbuf
happens to allow this, and it is very handy.  Perhaps the rule
stated there should be loosened and allow copying the buf away
when you know you have stuff in there (i.e. ->buf != slopbuf).
Pierre, what do you think?

What the patch does itself is much nicer than the original.

  Why wouldn't you just use strbuf_detach ? I mean replacing:

+	info->message_id = buf.buf;

with:

+	info->message_id = strbuf_detach(&buf, NULL);

  isn't really hard to read, and has the nice side effect to prevent
errors that could happen in the future (like reusing buf and screwing
with info->message_id without noticing it). I'd rather stand on the safe
side here, it's more forward-compatible and idiot-proof[0].

Is it actually right to have buf go out of scope right after 
strbuf_detach()? It sort of looks like it would leak memory from buf.buf. 
I'm happy to do whatever the API wants there, and I didn't see anything to 
leave the struct as if strbuf_release were called, but with the string 
extracted for the caller.

	-Daniel
*This .sig left intentionally blank*