On Mon, Oct 10, 2022 at 08:34:12PM -0400, Taylor Blau wrote:
> diff --git a/Documentation/git-shortlog.txt b/Documentation/git-shortlog.txt
> index 4982ceee21..ca15643f45 100644
> --- a/Documentation/git-shortlog.txt
> +++ b/Documentation/git-shortlog.txt
> @@ -59,6 +59,8 @@ OPTIONS
> 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`.
> + - `<format>`, any string accepted by the `--format` option of 'git log'.
> + (See the "PRETTY FORMATS" section of linkgit:git-log[1].)
I have a slight preference here to call this:
--group=format:<format>
and then DWIM:
--group=<format>
as long as <format> contains a percent. This is similar to how --pretty
works, but leaves us more room for changing things later if we need to.
> +static void insert_records_from_format(struct shortlog *log,
> + struct strset *dups,
> + struct commit *commit,
> + struct pretty_print_context *ctx,
> + const char *oneline)
> +{
> + struct strbuf buf = STRBUF_INIT;
> + struct string_list_item *item;
> +
> + for_each_string_list_item(item, &log->format) {
> + strbuf_reset(&buf);
> +
> + format_commit_message(commit, item->string, &buf, ctx);
> +
> + if (strset_add(dups, buf.buf))
> + insert_one_record(log, buf.buf, oneline);
> + }
> +
> + strbuf_release(&buf);
> +}
Versus the earlier patch we discussed, this is one identity per --group,
so it matches how --group=trailer works. Good.
> @@ -243,6 +266,7 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)
> if (log->groups & SHORTLOG_GROUP_TRAILER) {
> insert_records_from_trailers(log, &dups, commit, &ctx, oneline_str);
> }
> + insert_records_from_format(log, &dups, commit, &ctx, oneline_str);
Hmm. I see why we don't need to guard this with:
if (log->groups & SHORTLOG_GROUP_FORMAT)
since our helper function is a noop if log->format is empty. But I
wonder if:
- it's more clear to match the others (although it looks like they are
going away later, so that potentially becomes a non-issue)
- it's useful to have a conditional that lets us skip any setup work.
For trailers, in particular, we call logmsg_reencode(), which is
potentially expensive. OTOH, it would be easy for the helper
function to just return early when log->format.nr is 0.
So I'll reserve judgement until seeing the end state.
> diff --git a/shortlog.h b/shortlog.h
> index dc388dd459..4850a8c30f 100644
> --- a/shortlog.h
> +++ b/shortlog.h
> @@ -22,8 +22,10 @@ struct shortlog {
> SHORTLOG_GROUP_AUTHOR = (1 << 0),
> SHORTLOG_GROUP_COMMITTER = (1 << 1),
> SHORTLOG_GROUP_TRAILER = (1 << 2),
> + SHORTLOG_GROUP_FORMAT = (1 << 3),
> } groups;
> struct string_list trailers;
> + struct string_list format;
Given that "shortlog --format" does a totally different thing, I wonder
if this needs a more descriptive name like group_format or something.
Probably not. That other format feature is all in rev_info, and it's not
like "trailers" above doesn't have a similar issue.
> diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
> index 3095b1b2ff..2417ac8896 100755
> --- a/t/t4201-shortlog.sh
> +++ b/t/t4201-shortlog.sh
> @@ -237,6 +237,15 @@ test_expect_success 'shortlog --group=trailer:signed-off-by' '
> test_cmp expect actual
> '
>
> +test_expect_success 'shortlog --group=<format>' '
> + git shortlog -s --date="format:%Y" --group="%cN (%cd)" HEAD >actual &&
> + cat >expect <<-\EOF &&
> + 4 C O Mitter (2005)
> + 1 Sin Nombre (2005)
> + EOF
> + test_cmp expect actual
> +'
Makes sense. Two other tests that might be worth including:
- "shortlog --group=bogus" generates an error (we might already have
such a test; I didn't check)
- since the multiple-option behavior is so subtle, maybe show a case
where two formats partially overlap. A plausible one is "--group=%aN
--group=%cN", but the test setup might need tweaked to cover both.
There's an existing "multiple groups" test that might come in handy.
-Peff
