Re: [PATCH 06/14] shortlog: replace hand-parsing of author with pretty-printer

From: Jeff King <peff@peff.net>
Date: Jan 4, 2016, 10:17 UTC
Message-ID: <20160104101712.GA32086@sigill.intra.peff.net>
In-Reply-To: <CAPig+cRxBHp9hRBbFPYtEdytnmGdVYVMfJLKzjT6qoQ8EOSxQw@mail.gmail.com>

On Mon, Jan 04, 2016 at 04:43:23AM -0500, Eric Sunshine wrote:

> > +       format_commit_message(commit, "%an <%ae>", &author, &ctx);
> > +       if (author.len <= 3) {
> 
> I suppose magic number 3 is the space, '<', and '>'...

Yeah. That's worthy of a comment, I think (see below).

I don't think we will detect a case with just a missing name _or_ email, but neither did the original.

Show 5 quoted lines

> >                 warning(_("Missing author: %s"),
> >                     oid_to_hex(&commit->object.oid));
> >                 return;
> 
> Is this leaking strbuf 'author'?

Yes. Looks like the original leaked "buf" in the same way.

Rather than repeat the strbuf_release, I think we can introduce a goto, like:

diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 10c92fa..4180a2d 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -124,10 +124,11 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)
 	ctx.output_encoding = get_log_output_encoding();
 
 	format_commit_message(commit, "%an <%ae>", &author, &ctx);
+	/* we can detect a total failure only by seeing " <>" in the output */
 	if (author.len <= 3) {
 		warning(_("Missing author: %s"),
 		    oid_to_hex(&commit->object.oid));
-		return;
+		goto out;
 	}
 
 	if (log->user_format)
@@ -136,6 +137,8 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)
 		format_commit_message(commit, "%s", &oneline, &ctx);
 
 	insert_one_record(log, author.buf, oneline.len ? oneline.buf : "<none>");
+
+out:
 	strbuf_release(&author);
 	strbuf_release(&oneline);
 }

That should make it easier to handle the cases added later in the
series, too.

-Peff