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.
> > 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
--
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