| Message ID | 20200927084004.GD2465761@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | parsing trailers with shortlog | expand |
Jeff King <peff@peff.net> writes: > If a project uses commit trailers, this patch lets you use > shortlog to see who is performing each action. For example, > running: > > git shortlog -ns --group=trailer:reviewed-by > > in git.git shows who has reviewed. You can even use a custom > format to see things like who has helped whom: > > git shortlog --format="...helped %an (%ad)" \ > --group=trailer:helped-by That's a cute example. > +Note that commits that do not include the trailer will not be counted. Understandable. > +Likewise, commits with multiple trailers (e.g., multiple signoffs) may > +be counted more than once. Solicits a "is it desirable, or is it just too hard to dedupe?" response. > +The contents of each trailer value are taken literally and completely. > +No mailmap is applied, and the `-e` option has no effect (if the trailer > +contains a username and email, they are both always shown). OK. Some users may find that not quite satisfying, though. But I have a suspicion that the above will be refined in later steps? It would have been nicer to see that mentioned in the proposed log message (e.g. "this step gives the minimum basics and rough edges like X and Y will be refined with later patches"). Thanks.
On Sun, Sep 27, 2020 at 12:51:43PM -0700, Junio C Hamano wrote: > > +The contents of each trailer value are taken literally and completely. > > +No mailmap is applied, and the `-e` option has no effect (if the trailer > > +contains a username and email, they are both always shown). > > OK. Some users may find that not quite satisfying, though. > > But I have a suspicion that the above will be refined in later > steps? It would have been nicer to see that mentioned in the > proposed log message (e.g. "this step gives the minimum basics and > rough edges like X and Y will be refined with later patches"). I wondered if this might confuse people reading the series, and almost called attention to it in the cover letter. Now that you've presumably read through and figured it out, is it worth going back and amending the commit message? It's more of a point for reviewers, I think, but perhaps somebody reading the commits later would care. -Peff
Jeff King <peff@peff.net> writes: > I wondered if this might confuse people reading the series, and almost > called attention to it in the cover letter. Now that you've presumably > read through and figured it out, is it worth going back and amending the > commit message? It's more of a point for reviewers, I think, but perhaps > somebody reading the commits later would care. When I read history older than the immediate past, I usuall start at the point 'git blame' found and then go back---going forward is much less natural operation. If my "git blame" session happens to hit an earlier part of this series, perhaps the future-me would get a wrong impression that originally the feature was in a limited form and was extended later in a separate series? I dunno. I did wish the log message said what was going on while reading, but that is a perfectionist in me speaking, and not a complaint that log messages that did not say the future plans in the series were offense that deserves a rejection. A good summary in "What's cooking" report would become a merge commit log message, and we can teach users to go from 'git blame', find where the entire series was merged to the mainline and then read from there---and it would be sufficient if the summary of the topic described the endgame well.
diff --git a/Documentation/git-shortlog.txt b/Documentation/git-shortlog.txt index 6496d313c1..edd6cda58a 100644 --- a/Documentation/git-shortlog.txt +++ b/Documentation/git-shortlog.txt @@ -53,6 +53,19 @@ OPTIONS + - `author`, commits are grouped by author - `committer`, commits are grouped by committer (the same as `-c`) + - `trailer:<field>`, the `<field>` is interpreted as a case-insensitive + commit message trailer (see linkgit:git-interpret-trailers[1]). For + example, if your project uses `Reviewed-by` trailers, you might want + to see who has been reviewing with + `git shortlog -ns --group=trailer:reviewed-by`. ++ +Note that commits that do not include the trailer will not be counted. +Likewise, commits with multiple trailers (e.g., multiple signoffs) may +be counted more than once. ++ +The contents of each trailer value are taken literally and completely. +No mailmap is applied, and the `-e` option has no effect (if the trailer +contains a username and email, they are both always shown). -c:: --committer:: diff --git a/builtin/shortlog.c b/builtin/shortlog.c index 880ce19304..e1d9ee909f 100644 --- a/builtin/shortlog.c +++ b/builtin/shortlog.c @@ -9,6 +9,7 @@ #include "mailmap.h" #include "shortlog.h" #include "parse-options.h" +#include "trailer.h" static char const * const shortlog_usage[] = { N_("git shortlog [<options>] [<revision-range>] [[--] <path>...]"), @@ -136,6 +137,8 @@ static void read_from_stdin(struct shortlog *log) case SHORTLOG_GROUP_COMMITTER: match = committer_match; break; + case SHORTLOG_GROUP_TRAILER: + die(_("using --group=trailer with stdin is not supported")); default: BUG("unhandled shortlog group"); } @@ -163,6 +166,37 @@ static void read_from_stdin(struct shortlog *log) strbuf_release(&oneline); } +static void insert_records_from_trailers(struct shortlog *log, + struct commit *commit, + struct pretty_print_context *ctx, + const char *oneline) +{ + struct trailer_iterator iter; + const char *commit_buffer, *body; + + /* + * Using format_commit_message("%B") would be simpler here, but + * this saves us copying the message. + */ + commit_buffer = logmsg_reencode(commit, NULL, ctx->output_encoding); + body = strstr(commit_buffer, "\n\n"); + if (!body) + return; + + trailer_iterator_init(&iter, body); + while (trailer_iterator_advance(&iter)) { + const char *value = iter.val.buf; + + if (strcasecmp(iter.key.buf, log->trailer)) + continue; + + insert_one_record(log, value, oneline); + } + trailer_iterator_release(&iter); + + unuse_commit_buffer(commit, commit_buffer); +} + void shortlog_add_commit(struct shortlog *log, struct commit *commit) { struct strbuf ident = STRBUF_INIT; @@ -197,6 +231,9 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit) &ident, &ctx); insert_one_record(log, ident.buf, oneline_str); break; + case SHORTLOG_GROUP_TRAILER: + insert_records_from_trailers(log, commit, &ctx, oneline_str); + break; } strbuf_release(&ident); @@ -263,12 +300,17 @@ static int parse_wrap_args(const struct option *opt, const char *arg, int unset) static int parse_group_option(const struct option *opt, const char *arg, int unset) { struct shortlog *log = opt->value; + const char *field; if (unset || !strcasecmp(arg, "author")) log->group = SHORTLOG_GROUP_AUTHOR; else if (!strcasecmp(arg, "committer")) log->group = SHORTLOG_GROUP_COMMITTER; - else + else if (skip_prefix(arg, "trailer:", &field)) { + log->group = SHORTLOG_GROUP_TRAILER; + free(log->trailer); + log->trailer = xstrdup(field); + } else return error(_("unknown group type: %s"), arg); return 0; diff --git a/shortlog.h b/shortlog.h index 876a52158d..89c2dbc5e6 100644 --- a/shortlog.h +++ b/shortlog.h @@ -19,7 +19,9 @@ struct shortlog { enum { SHORTLOG_GROUP_AUTHOR = 0, SHORTLOG_GROUP_COMMITTER, + SHORTLOG_GROUP_TRAILER, } group; + char *trailer; char *common_repo_prefix; int email; diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh index 65e4468746..e97d891a71 100755 --- a/t/t4201-shortlog.sh +++ b/t/t4201-shortlog.sh @@ -220,4 +220,18 @@ test_expect_success '--group=committer is the same as --committer' ' test_cmp expect actual ' +test_expect_success 'shortlog --group=trailer:signed-off-by' ' + git commit --allow-empty -m foo -s && + GIT_COMMITTER_NAME="SOB One" \ + GIT_COMMITTER_EMAIL=sob@example.com \ + git commit --allow-empty -m foo -s && + git commit --allow-empty --amend --no-edit -s && + cat >expect <<-\EOF && + 2 C O Mitter <committer@example.com> + 1 SOB One <sob@example.com> + EOF + git shortlog -ns --group=trailer:signed-off-by HEAD >actual && + test_cmp expect actual +' + test_done
If a project uses commit trailers, this patch lets you use shortlog to see who is performing each action. For example, running: git shortlog -ns --group=trailer:reviewed-by in git.git shows who has reviewed. You can even use a custom format to see things like who has helped whom: git shortlog --format="...helped %an (%ad)" \ --group=trailer:helped-by Signed-off-by: Jeff King <peff@peff.net> --- Documentation/git-shortlog.txt | 13 ++++++++++ builtin/shortlog.c | 44 +++++++++++++++++++++++++++++++++- shortlog.h | 2 ++ t/t4201-shortlog.sh | 14 +++++++++++ 4 files changed, 72 insertions(+), 1 deletion(-)