On Mon, Jan 04, 2016 at 04:44:00AM -0500, Eric Sunshine wrote:
> > diff --git a/Documentation/git-shortlog.txt b/Documentation/git-shortlog.txt
> > @@ -47,6 +47,14 @@ OPTIONS
> > +--ident=<type>::
>
> Should this be called --group-by?
Yeah, that may be a more sensible name. For committer/author, I think
--ident is accurate. But for a trailer, we technically do not know what
is in it. I expect people would use this primarily with ident-based
trailers (like "reviewed-by" or whatever), but there's no reason you
could not collate references to a bug id, or something.
> > + By default, `shortlog` collects and collates author identities;
> > + using `--ident` will collect other types of identity. If
> > + `<type>` is:
> > ++
> > + - `author`, commits are grouped by author (this is the default)
> > + - `committer`, commits are grouped by committer
>
> There is a bit of redundancy here. I wonder if it could be described
> more succinctly. For instance, instead of all the above, perhaps:
>
> Group commits by `<type>`, which can be one of:
>
> - `author` (default)
> - `committer`
I avoided that kind of parallel structure because I knew that later in
the series I would be adding a new bullet that would need more
explanation. I'm still pondering `trailer:<key>` as the syntax, which
would involve reworking this text. I'll keep it in mind when I do.
> > + case SHORTLOG_IDENT_COMMITTER:
> > + format_commit_message(commit, "%cn <%ce>", &ident, &ctx);
> > + if (ident.len <= 3) {
> > + warning(_("Missing committer: %s"),
> > + oid_to_hex(&commit->object.oid));
> > + return;
>
> Is this leaking strbuf 'ident'?
>
> (Ditto for the "author" case as mentioned already in the patch 6/14 review.)
Yep. :) I think these can become "goto out" per my previous fixup.
-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