{"thread":{"id":"58597","subject":"[PATCH 0/7] shortlog: introduce `--group=`","startedAt":"2022-10-11T00:34:05Z","lastAt":"2022-10-25T00:57:28Z","messageCount":72,"participants":["Taylor Blau","Jeff King","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"464558","messageId":"cover.1665448437.git.me@ttaylorr.com","threadId":"58597","inReplyTo":null,"subject":"[PATCH 0/7] shortlog: introduce `--group=`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-11T00:33:59Z","receivedAt":"2022-10-11T00:34:05Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This series is based on a suggestion[1] from Junio in response to\nJacob Stopak's series to add committer month/year options as shortlog\ngroups.\n\nThis series pursues a more flexible approach, where arbitrary pretty\nformats can be used to generate group names in shortlog. For example, to\ngroup by committer month/year, you can do the following:\n\n $ git.compile shortlog -s --group='%cd' --date='format:%Y-%m' v2.37.0..\n 117 2022-06\n 274 2022-07\n 325 2022-08\n 271 2022-09\n 17 2022-10\n\n, which is cute. It can also be used to reimplement existing\nfunctionality in shortlog, which is what the final three patches in this\nseries do. Author and committer options are implemented under the hood\nas if they were shorthands for `--group='%aN <%aE>'` (or `--group='%cN\n<%cE>'`, respectively).\n\nThe `--group=trailer:` option is also reimplemented, but it is\nslightly trickier than the rest. See the final patch for more details\nthere.\n\nThanks in advance for your review.\n\n[1]: https://lore.kernel.org/git/xmqqillevzeh.fsf@gitster.g/\n\nJeff King (1):\n shortlog: accept `--date`-related options\n\nTaylor Blau (6):\n Documentation: extract date-options.txt\n shortlog: extract `--group` fragment for translation\n shortlog: support arbitrary commit format `--group`s\n shortlog: implement `--group=author` in terms of `--group=`\n shortlog: implement `--group=committer` in terms of `--group=`\n shortlog: implement `--group=trailer` in terms of `--group=`\n\n Documentation/date-options.txt | 66 +++++++++++++\n Documentation/git-shortlog.txt | 4 +\n Documentation/rev-list-options.txt | 67 +------------\n builtin/log.c | 1 +\n builtin/shortlog.c | 145 +++++++++++++++--------------\n shortlog.h | 6 +-\n t/t4201-shortlog.sh | 9 ++\n 7 files changed, 163 insertions(+), 135 deletions(-)\n create mode 100644 Documentation/date-options.txt\n\n-- \n2.37.0.1.g1379af2e9d\n"},{"id":"464559","messageId":"eaec59daa178c205a59b779c63e411316d8180ed.1665448437.git.me@ttaylorr.com","threadId":"58597","inReplyTo":"cover.1665448437.git.me@ttaylorr.com","subject":"[PATCH 1/7] Documentation: extract date-options.txt","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-11T00:34:02Z","receivedAt":"2022-10-11T00:34:11Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"A future commit will want to include `--date`-related options in the\ndocumentation for `git-shortlog(1)`, but without some of the additional\nbaggage in the usual rev-list-options.txt.\n\nExtract those options to a separate file in Documentation and include it\nfrom its original source in rev-list-options.txt.\n\nThis patch does not modify the contents of the `--date`-options section\nof Documentation/rev-list-options.txt.\n\nSigned-off-by: Taylor Blau \n---\n Documentation/date-options.txt | 66 +++++++++++++++++++++++++++++\n Documentation/rev-list-options.txt | 67 +-----------------------------\n 2 files changed, 67 insertions(+), 66 deletions(-)\n create mode 100644 Documentation/date-options.txt\n\ndiff --git a/Documentation/date-options.txt b/Documentation/date-options.txt\nnew file mode 100644\nindex 0000000000..65896e4b95\n--- /dev/null\n+++ b/Documentation/date-options.txt\n@@ -0,0 +1,66 @@\n+--relative-date::\n+\tSynonym for `--date=relative`.\n+\n+--date=::\n+\tOnly takes effect for dates shown in human-readable format, such\n+\tas when using `--pretty`. `log.date` config variable sets a default\n+\tvalue for the log command's `--date` option. By default, dates\n+\tare shown in the original time zone (either committer's or\n+\tauthor's). If `-local` is appended to the format (e.g.,\n+\t`iso-local`), the user's local time zone is used instead.\n++\n+--\n+`--date=relative` shows dates relative to the current time,\n+e.g. ``2 hours ago''. The `-local` option has no effect for\n+`--date=relative`.\n+\n+`--date=local` is an alias for `--date=default-local`.\n+\n+`--date=iso` (or `--date=iso8601`) shows timestamps in a ISO 8601-like format.\n+The differences to the strict ISO 8601 format are:\n+\n+\t- a space instead of the `T` date/time delimiter\n+\t- a space between time and time zone\n+\t- no colon between hours and minutes of the time zone\n+\n+`--date=iso-strict` (or `--date=iso8601-strict`) shows timestamps in strict\n+ISO 8601 format.\n+\n+`--date=rfc` (or `--date=rfc2822`) shows timestamps in RFC 2822\n+format, often found in email messages.\n+\n+`--date=short` shows only the date, but not the time, in `YYYY-MM-DD` format.\n+\n+`--date=raw` shows the date as seconds since the epoch (1970-01-01\n+00:00:00 UTC), followed by a space, and then the timezone as an offset\n+from UTC (a `+` or `-` with four digits; the first two are hours, and\n+the second two are minutes). I.e., as if the timestamp were formatted\n+with `strftime(\"%s %z\")`).\n+Note that the `-local` option does not affect the seconds-since-epoch\n+value (which is always measured in UTC), but does switch the accompanying\n+timezone value.\n+\n+`--date=human` shows the timezone if the timezone does not match the\n+current time-zone, and doesn't print the whole date if that matches\n+(ie skip printing year for dates that are \"this year\", but also skip\n+the whole date itself if it's in the last few days and we can just say\n+what weekday it was). For older dates the hour and minute is also\n+omitted.\n+\n+`--date=unix` shows the date as a Unix epoch timestamp (seconds since\n+1970). As with `--raw`, this is always in UTC and therefore `-local`\n+has no effect.\n+\n+`--date=format:...` feeds the format `...` to your system `strftime`,\n+except for %s, %z, and %Z, which are handled internally.\n+Use `--date=format:%c` to show the date in your system locale's\n+preferred format. See the `strftime` manual for a complete list of\n+format placeholders. When using `-local`, the correct syntax is\n+`--date=format-local:...`.\n+\n+`--date=default` is the default format, and is similar to\n+`--date=rfc2822`, with a few exceptions:\n+--\n+\t- there is no comma after the day-of-week\n+\n+\t- the time zone is omitted when the local time zone is used\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 1837509566..1cb91dfb9c 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -1033,72 +1033,7 @@ endif::git-rev-list[]\n \n include::pretty-options.txt[]\n \n---relative-date::\n-\tSynonym for `--date=relative`.\n-\n---date=::\n-\tOnly takes effect for dates shown in human-readable format, such\n-\tas when using `--pretty`. `log.date` config variable sets a default\n-\tvalue for the log command's `--date` option. By default, dates\n-\tare shown in the original time zone (either committer's or\n-\tauthor's). If `-local` is appended to the format (e.g.,\n-\t`iso-local`), the user's local time zone is used instead.\n-+\n---\n-`--date=relative` shows dates relative to the current time,\n-e.g. ``2 hours ago''. The `-local` option has no effect for\n-`--date=relative`.\n-\n-`--date=local` is an alias for `--date=default-local`.\n-\n-`--date=iso` (or `--date=iso8601`) shows timestamps in a ISO 8601-like format.\n-The differences to the strict ISO 8601 format are:\n-\n-\t- a space instead of the `T` date/time delimiter\n-\t- a space between time and time zone\n-\t- no colon between hours and minutes of the time zone\n-\n-`--date=iso-strict` (or `--date=iso8601-strict`) shows timestamps in strict\n-ISO 8601 format.\n-\n-`--date=rfc` (or `--date=rfc2822`) shows timestamps in RFC 2822\n-format, often found in email messages.\n-\n-`--date=short` shows only the date, but not the time, in `YYYY-MM-DD` format.\n-\n-`--date=raw` shows the date as seconds since the epoch (1970-01-01\n-00:00:00 UTC), followed by a space, and then the timezone as an offset\n-from UTC (a `+` or `-` with four digits; the first two are hours, and\n-the second two are minutes). I.e., as if the timestamp were formatted\n-with `strftime(\"%s %z\")`).\n-Note that the `-local` option does not affect the seconds-since-epoch\n-value (which is always measured in UTC), but does switch the accompanying\n-timezone value.\n-\n-`--date=human` shows the timezone if the timezone does not match the\n-current time-zone, and doesn't print the whole date if that matches\n-(ie skip printing year for dates that are \"this year\", but also skip\n-the whole date itself if it's in the last few days and we can just say\n-what weekday it was). For older dates the hour and minute is also\n-omitted.\n-\n-`--date=unix` shows the date as a Unix epoch timestamp (seconds since\n-1970). As with `--raw`, this is always in UTC and therefore `-local`\n-has no effect.\n-\n-`--date=format:...` feeds the format `...` to your system `strftime`,\n-except for %s, %z, and %Z, which are handled internally.\n-Use `--date=format:%c` to show the date in your system locale's\n-preferred format. See the `strftime` manual for a complete list of\n-format placeholders. When using `-local`, the correct syntax is\n-`--date=format-local:...`.\n-\n-`--date=default` is the default format, and is similar to\n-`--date=rfc2822`, with a few exceptions:\n---\n-\t- there is no comma after the day-of-week\n-\n-\t- the time zone is omitted when the local time zone is used\n+include::date-options.txt[]\n \n ifdef::git-rev-list[]\n --header::\n-- \n2.37.0.1.g1379af2e9d\n\n"},{"id":"464560","messageId":"b587b8ea4ab593806b9fb6d1db8751591991455b.1665448437.git.me@ttaylorr.com","threadId":"58597","inReplyTo":"cover.1665448437.git.me@ttaylorr.com","subject":"[PATCH 2/7] shortlog: accept `--date`-related options","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-11T00:34:05Z","receivedAt":"2022-10-11T00:34:24Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"From: Jeff King \n\nPrepare for the future patch which will introduce arbitrary pretty\nformats via the `--group` argument.\n\nTo allow additional customizability (for example, to support something\nlike `git shortlog -s --group='%aD' --date='format:%Y-%m' ...` (which\ngroups commits by the datestring 'YYYY-mm' according to author date), we\nmust store off the `--date` parsed from calling `parse_revision_opt()`.\n\nSigned-off-by: Jeff King \nSigned-off-by: Taylor Blau \n---\n Documentation/git-shortlog.txt | 2 ++\n builtin/shortlog.c | 3 ++-\n shortlog.h | 2 ++\n 3 files changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-shortlog.txt b/Documentation/git-shortlog.txt\nindex f64e77047b..4982ceee21 100644\n--- a/Documentation/git-shortlog.txt\n+++ b/Documentation/git-shortlog.txt\n@@ -108,6 +108,8 @@ options or the revision range, when confusion arises.\n :git-shortlog: 1\n include::rev-list-options.txt[]\n \n+include::date-options.txt[]\n+\n MAPPING AUTHORS\n ---------------\n \ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 7a1e1fe7c0..53c379a51d 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -211,7 +211,7 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n \tctx.fmt = CMIT_FMT_USERFORMAT;\n \tctx.abbrev = log->abbrev;\n \tctx.print_email_subject = 1;\n-\tctx.date_mode.type = DATE_NORMAL;\n+\tctx.date_mode = log->date_mode;\n \tctx.output_encoding = get_log_output_encoding();\n \n \tif (!log->summary) {\n@@ -407,6 +407,7 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)\n \tlog.user_format = rev.commit_format == CMIT_FMT_USERFORMAT;\n \tlog.abbrev = rev.abbrev;\n \tlog.file = rev.diffopt.file;\n+\tlog.date_mode = rev.date_mode;\n \n \tif (!log.groups)\n \t\tlog.groups = SHORTLOG_GROUP_AUTHOR;\ndiff --git a/shortlog.h b/shortlog.h\nindex 3f7e9aabca..dc388dd459 100644\n--- a/shortlog.h\n+++ b/shortlog.h\n@@ -2,6 +2,7 @@\n #define SHORTLOG_H\n \n #include \"string-list.h\"\n+#include \"date.h\"\n \n struct commit;\n \n@@ -15,6 +16,7 @@ struct shortlog {\n \tint in2;\n \tint user_format;\n \tint abbrev;\n+\tstruct date_mode date_mode;\n \n \tenum {\n \t\tSHORTLOG_GROUP_AUTHOR = (1 << 0),\n-- \n2.37.0.1.g1379af2e9d\n\n"},{"id":"464561","messageId":"c3f50465cba4430b2014a76c5e473b906db50059.1665448437.git.me@ttaylorr.com","threadId":"58597","inReplyTo":"cover.1665448437.git.me@ttaylorr.com","subject":"[PATCH 3/7] shortlog: extract `--group` fragment for translation","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-11T00:34:08Z","receivedAt":"2022-10-11T00:34:27Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The subsequent commit will add another unhandled case in\n`read_from_stdin()` which will want to use the same message as with\n`--group=trailer`.\n\nExtract the \"--group=trailer\" part from this message so the same\ntranslation key can be used for both cases.\n\nSigned-off-by: Taylor Blau \n---\n builtin/shortlog.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 53c379a51d..051c97bd5a 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -132,7 +132,7 @@ static void read_from_stdin(struct shortlog *log)\n \t\tmatch = committer_match;\n \t\tbreak;\n \tcase SHORTLOG_GROUP_TRAILER:\n-\t\tdie(_(\"using --group=trailer with stdin is not supported\"));\n+\t\tdie(_(\"using %s with stdin is not supported\"), \"--group=trailer\");\n \tdefault:\n \t\tBUG(\"unhandled shortlog group\");\n \t}\n-- \n2.37.0.1.g1379af2e9d\n\n"},{"id":"464562","messageId":"6f38990cc2ea8460ce37437e0770784d9b712dab.1665448437.git.me@ttaylorr.com","threadId":"58597","inReplyTo":"cover.1665448437.git.me@ttaylorr.com","subject":"[PATCH 4/7] shortlog: support arbitrary commit format `--group`s","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-11T00:34:12Z","receivedAt":"2022-10-11T00:34:45Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In addition to generating a shortlog based on committer, author, or the\nidentity in one or more specified trailers, it can be useful to generate\na shortlog based on an arbitrary commit format.\n\nThis can be used, for example, to generate a distribution of commit\nactivity over time, like so:\n\n $ git shortlog --group='%cd' --date='format:%Y-%m' -s v2.37.0..\n 117 2022-06\n 274 2022-07\n 324 2022-08\n 263 2022-09\n 7 2022-10\n\nArbitrary commit formats can be used. In fact, `git shortlog`'s default\nbehavior (to count by commit authors) can be emulated as follows:\n\n $ git shortlog --group='%aN <%aE>' ...\n\nand future patches will make the default behavior (as well as\n`--committer`, and `--group=trailer:`) special cases of the\nmore flexible `--group` option.\n\nNote also that the SHORTLOG_GROUP_FORMAT enum value is used only to\ndesignate that `--group:` is in use when in stdin mode to\ndeclare that the combination is invalid.\n\nSigned-off-by: Taylor Blau \n---\n Documentation/git-shortlog.txt | 2 ++\n builtin/shortlog.c | 33 ++++++++++++++++++++++++++++++++-\n shortlog.h | 2 ++\n t/t4201-shortlog.sh | 9 +++++++++\n 4 files changed, 45 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-shortlog.txt b/Documentation/git-shortlog.txt\nindex 4982ceee21..ca15643f45 100644\n--- a/Documentation/git-shortlog.txt\n+++ b/Documentation/git-shortlog.txt\n@@ -59,6 +59,8 @@ OPTIONS\n example, if your project uses `Reviewed-by` trailers, you might want\n to see who has been reviewing with\n `git shortlog -ns --group=trailer:reviewed-by`.\n+ - ``, any string accepted by the `--format` option of 'git log'.\n+ (See the \"PRETTY FORMATS\" section of linkgit:git-log[1].)\n +\n Note that commits that do not include the trailer will not be counted.\n Likewise, commits with multiple trailers (e.g., multiple signoffs) may\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 051c97bd5a..f708d96558 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -133,6 +133,8 @@ static void read_from_stdin(struct shortlog *log)\n \t\tbreak;\n \tcase SHORTLOG_GROUP_TRAILER:\n \t\tdie(_(\"using %s with stdin is not supported\"), \"--group=trailer\");\n+\tcase SHORTLOG_GROUP_FORMAT:\n+\t\tdie(_(\"using %s with stdin is not supported\"), \"--group=format\");\n \tdefault:\n \t\tBUG(\"unhandled shortlog group\");\n \t}\n@@ -200,6 +202,27 @@ static void insert_records_from_trailers(struct shortlog *log,\n \tunuse_commit_buffer(commit, commit_buffer);\n }\n \n+static void insert_records_from_format(struct shortlog *log,\n+\t\t\t\t struct strset *dups,\n+\t\t\t\t struct commit *commit,\n+\t\t\t\t struct pretty_print_context *ctx,\n+\t\t\t\t const char *oneline)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct string_list_item *item;\n+\n+\tfor_each_string_list_item(item, &log->format) {\n+\t\tstrbuf_reset(&buf);\n+\n+\t\tformat_commit_message(commit, item->string, &buf, ctx);\n+\n+\t\tif (strset_add(dups, buf.buf))\n+\t\t\tinsert_one_record(log, buf.buf, oneline);\n+\t}\n+\n+\tstrbuf_release(&buf);\n+}\n+\n void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n {\n \tstruct strbuf ident = STRBUF_INIT;\n@@ -243,6 +266,7 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n \tif (log->groups & SHORTLOG_GROUP_TRAILER) {\n \t\tinsert_records_from_trailers(log, &dups, commit, &ctx, oneline_str);\n \t}\n+\tinsert_records_from_format(log, &dups, commit, &ctx, oneline_str);\n \n \tstrset_clear(&dups);\n \tstrbuf_release(&ident);\n@@ -314,6 +338,7 @@ static int parse_group_option(const struct option *opt, const char *arg, int uns\n \tif (unset) {\n \t\tlog->groups = 0;\n \t\tstring_list_clear(&log->trailers, 0);\n+\t\tstring_list_clear(&log->format, 0);\n \t} else if (!strcasecmp(arg, \"author\"))\n \t\tlog->groups |= SHORTLOG_GROUP_AUTHOR;\n \telse if (!strcasecmp(arg, \"committer\"))\n@@ -321,8 +346,12 @@ static int parse_group_option(const struct option *opt, const char *arg, int uns\n \telse if (skip_prefix(arg, \"trailer:\", &field)) {\n \t\tlog->groups |= SHORTLOG_GROUP_TRAILER;\n \t\tstring_list_append(&log->trailers, field);\n-\t} else\n+\t} else if (strchrnul(arg, '%')) {\n+\t\tlog->groups |= SHORTLOG_GROUP_FORMAT;\n+\t\tstring_list_append(&log->format, arg);\n+\t} else {\n \t\treturn error(_(\"unknown group type: %s\"), arg);\n+\t}\n \n \treturn 0;\n }\n@@ -340,6 +369,7 @@ void shortlog_init(struct shortlog *log)\n \tlog->in2 = DEFAULT_INDENT2;\n \tlog->trailers.strdup_strings = 1;\n \tlog->trailers.cmp = strcasecmp;\n+\tlog->format.strdup_strings = 1;\n }\n \n int cmd_shortlog(int argc, const char **argv, const char *prefix)\n@@ -480,4 +510,5 @@ void shortlog_output(struct shortlog *log)\n \tlog->list.strdup_strings = 1;\n \tstring_list_clear(&log->list, 1);\n \tclear_mailmap(&log->mailmap);\n+\tstring_list_clear(&log->format, 0);\n }\ndiff --git a/shortlog.h b/shortlog.h\nindex dc388dd459..4850a8c30f 100644\n--- a/shortlog.h\n+++ b/shortlog.h\n@@ -22,8 +22,10 @@ struct shortlog {\n \t\tSHORTLOG_GROUP_AUTHOR = (1 << 0),\n \t\tSHORTLOG_GROUP_COMMITTER = (1 << 1),\n \t\tSHORTLOG_GROUP_TRAILER = (1 << 2),\n+\t\tSHORTLOG_GROUP_FORMAT = (1 << 3),\n \t} groups;\n \tstruct string_list trailers;\n+\tstruct string_list format;\n \n \tint email;\n \tstruct string_list mailmap;\ndiff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh\nindex 3095b1b2ff..2417ac8896 100755\n--- a/t/t4201-shortlog.sh\n+++ b/t/t4201-shortlog.sh\n@@ -237,6 +237,15 @@ test_expect_success 'shortlog --group=trailer:signed-off-by' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'shortlog --group=' '\n+\tgit shortlog -s --date=\"format:%Y\" --group=\"%cN (%cd)\" HEAD >actual &&\n+\tcat >expect <<-\\EOF &&\n+\t 4\tC O Mitter (2005)\n+\t 1\tSin Nombre (2005)\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'trailer idents are split' '\n \tcat >expect <<-\\EOF &&\n \t 2\tC O Mitter\n-- \n2.37.0.1.g1379af2e9d\n\n"},{"id":"464563","messageId":"55a6ef7bc0082818fa51a0915c43002ede5c449f.1665448437.git.me@ttaylorr.com","threadId":"58597","inReplyTo":"cover.1665448437.git.me@ttaylorr.com","subject":"[PATCH 5/7] shortlog: implement `--group=author` in terms of `--group=`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-11T00:34:15Z","receivedAt":"2022-10-11T00:34:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Instead of handling SHORTLOG_GROUP_AUTHOR separately, reimplement it as\na special case of the new `--group=` mode, where the author mode\nis a shorthand for `--group='%aN <%aE>'.\n\nNote that we still need to keep the SHORTLOG_GROUP_AUTHOR enum since it\nhas a different meaning in `read_from_stdin()`, where it is still used\nfor a different purpose.\n\nSigned-off-by: Taylor Blau \n---\n builtin/log.c | 1 +\n builtin/shortlog.c | 23 ++++++++++++-----------\n shortlog.h | 1 +\n 3 files changed, 14 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex ee19dc5d45..6b77e520b5 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1334,6 +1334,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,\n \tlog.in2 = 4;\n \tlog.file = rev->diffopt.file;\n \tlog.groups = SHORTLOG_GROUP_AUTHOR;\n+\tshortlog_init_group(&log);\n \tfor (i = 0; i < nr; i++)\n \t\tshortlog_add_commit(&log, list[i]);\n \ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex f708d96558..aac8c7afa4 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -245,15 +245,6 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n \t}\n \toneline_str = oneline.len ? oneline.buf : \"\";\n \n-\tif (log->groups & SHORTLOG_GROUP_AUTHOR) {\n-\t\tstrbuf_reset(&ident);\n-\t\tformat_commit_message(commit,\n-\t\t\t\t log->email ? \"%aN <%aE>\" : \"%aN\",\n-\t\t\t\t &ident, &ctx);\n-\t\tif (!HAS_MULTI_BITS(log->groups) ||\n-\t\t strset_add(&dups, ident.buf))\n-\t\t\tinsert_one_record(log, ident.buf, oneline_str);\n-\t}\n \tif (log->groups & SHORTLOG_GROUP_COMMITTER) {\n \t\tstrbuf_reset(&ident);\n \t\tformat_commit_message(commit,\n@@ -372,6 +363,16 @@ void shortlog_init(struct shortlog *log)\n \tlog->format.strdup_strings = 1;\n }\n \n+void shortlog_init_group(struct shortlog *log)\n+{\n+\tif (!log->groups)\n+\t\tlog->groups = SHORTLOG_GROUP_AUTHOR;\n+\n+\tif (log->groups & SHORTLOG_GROUP_AUTHOR)\n+\t\tstring_list_append(&log->format,\n+\t\t\t\t log->email ? \"%aN <%aE>\" : \"%aN\");\n+}\n+\n int cmd_shortlog(int argc, const char **argv, const char *prefix)\n {\n \tstruct shortlog log = { STRING_LIST_INIT_NODUP };\n@@ -439,8 +440,8 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)\n \tlog.file = rev.diffopt.file;\n \tlog.date_mode = rev.date_mode;\n \n-\tif (!log.groups)\n-\t\tlog.groups = SHORTLOG_GROUP_AUTHOR;\n+\tshortlog_init_group(&log);\n+\n \tstring_list_sort(&log.trailers);\n \n \t/* assume HEAD if from a tty */\ndiff --git a/shortlog.h b/shortlog.h\nindex 4850a8c30f..e52f001fb7 100644\n--- a/shortlog.h\n+++ b/shortlog.h\n@@ -33,6 +33,7 @@ struct shortlog {\n };\n \n void shortlog_init(struct shortlog *log);\n+void shortlog_init_group(struct shortlog *log);\n \n void shortlog_add_commit(struct shortlog *log, struct commit *commit);\n \n-- \n2.37.0.1.g1379af2e9d\n\n"},{"id":"464564","messageId":"02adc297e7661cbc25302b9f5659d2356b8b5008.1665448437.git.me@ttaylorr.com","threadId":"58597","inReplyTo":"cover.1665448437.git.me@ttaylorr.com","subject":"[PATCH 7/7] shortlog: implement `--group=trailer` in terms of `--group=`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-11T00:34:21Z","receivedAt":"2022-10-11T00:35:04Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In the same spirit as the previous commit, reimplement\n`--group=trailer:` as a special case of `--group=`, too.\n\nUnsurprisingly, this reimplementation is a little bit more complicated\nthan the previous two. The complexity stems from having to enumerate\neach of the trailers one-by-one, as well as delegating control to\n`parse_ident()` to handle whether or not to show an individual's email.\n\nTo enumerate each trailer in a commit, we set the separator used in the\npretty format to be a NUL byte. This hack allows us to treat the strbuf\nfrom `format_commit_message()` as an array of strings.\n\nSince this handling is unique to the `--group:trailer` mode, use `util`\nbit in the `string_list_entry` to signal which ``s came from\n`--group:trailer` arguments (and thus need special treatment).\n\nSigned-off-by: Taylor Blau \n---\n builtin/shortlog.c | 66 ++++++++++++++++++----------------------------\n shortlog.h | 1 -\n 2 files changed, 25 insertions(+), 42 deletions(-)\n\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex b46df179fe..e8cf727342 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -162,44 +162,30 @@ static void read_from_stdin(struct shortlog *log)\n \tstrbuf_release(&oneline);\n }\n \n-static void insert_records_from_trailers(struct shortlog *log,\n-\t\t\t\t\t struct strset *dups,\n-\t\t\t\t\t struct commit *commit,\n-\t\t\t\t\t struct pretty_print_context *ctx,\n-\t\t\t\t\t const char *oneline)\n+static void insert_records_from_trailer(struct shortlog *log,\n+\t\t\t\t\tstruct strset *dups,\n+\t\t\t\t\tstruct strbuf *buf,\n+\t\t\t\t\tstruct pretty_print_context *ctx,\n+\t\t\t\t\tconst char *oneline)\n {\n-\tstruct trailer_iterator iter;\n-\tconst char *commit_buffer, *body;\n-\tstruct strbuf ident = STRBUF_INIT;\n+\tstruct strbuf ibuf = STRBUF_INIT;\n+\tchar *bol = buf->buf;\n+\tchar *value;\n \n-\t/*\n-\t * Using format_commit_message(\"%B\") would be simpler here, but\n-\t * this saves us copying the message.\n-\t */\n-\tcommit_buffer = logmsg_reencode(commit, NULL, ctx->output_encoding);\n-\tbody = strstr(commit_buffer, \"\\n\\n\");\n-\tif (!body)\n-\t\treturn;\n+\twhile (bol < buf->buf + buf->len) {\n+\t\tstrbuf_reset(&ibuf);\n \n-\ttrailer_iterator_init(&iter, body);\n-\twhile (trailer_iterator_advance(&iter)) {\n-\t\tconst char *value = iter.val.buf;\n+\t\tvalue = bol;\n+\t\tif (!parse_ident(log, &ibuf, bol))\n+\t\t\tvalue = ibuf.buf;\n \n-\t\tif (!string_list_has_string(&log->trailers, iter.key.buf))\n-\t\t\tcontinue;\n+\t\tif (strset_add(dups, value))\n+\t\t\tinsert_one_record(log, value, oneline);\n \n-\t\tstrbuf_reset(&ident);\n-\t\tif (!parse_ident(log, &ident, value))\n-\t\t\tvalue = ident.buf;\n-\n-\t\tif (!strset_add(dups, value))\n-\t\t\tcontinue;\n-\t\tinsert_one_record(log, value, oneline);\n+\t\tbol += strlen(bol) + 1;\n \t}\n-\ttrailer_iterator_release(&iter);\n \n-\tstrbuf_release(&ident);\n-\tunuse_commit_buffer(commit, commit_buffer);\n+\tstrbuf_release(&ibuf);\n }\n \n static void insert_records_from_format(struct shortlog *log,\n@@ -216,7 +202,10 @@ static void insert_records_from_format(struct shortlog *log,\n \n \t\tformat_commit_message(commit, item->string, &buf, ctx);\n \n-\t\tif (strset_add(dups, buf.buf))\n+\t\tif (item->util)\n+\t\t\tinsert_records_from_trailer(log, dups, &buf, ctx,\n+\t\t\t\t\t\t oneline);\n+\t\telse if (strset_add(dups, buf.buf))\n \t\t\tinsert_one_record(log, buf.buf, oneline);\n \t}\n \n@@ -244,9 +233,6 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n \t}\n \toneline_str = oneline.len ? oneline.buf : \"\";\n \n-\tif (log->groups & SHORTLOG_GROUP_TRAILER) {\n-\t\tinsert_records_from_trailers(log, &dups, commit, &ctx, oneline_str);\n-\t}\n \tinsert_records_from_format(log, &dups, commit, &ctx, oneline_str);\n \n \tstrset_clear(&dups);\n@@ -317,15 +303,17 @@ static int parse_group_option(const struct option *opt, const char *arg, int uns\n \n \tif (unset) {\n \t\tlog->groups = 0;\n-\t\tstring_list_clear(&log->trailers, 0);\n \t\tstring_list_clear(&log->format, 0);\n \t} else if (!strcasecmp(arg, \"author\"))\n \t\tlog->groups |= SHORTLOG_GROUP_AUTHOR;\n \telse if (!strcasecmp(arg, \"committer\"))\n \t\tlog->groups |= SHORTLOG_GROUP_COMMITTER;\n \telse if (skip_prefix(arg, \"trailer:\", &field)) {\n+\t\tstruct strbuf buf = STRBUF_INIT;\n \t\tlog->groups |= SHORTLOG_GROUP_TRAILER;\n-\t\tstring_list_append(&log->trailers, field);\n+\t\tstrbuf_addf(&buf, \"%%(trailers:key=%s,valueonly=true,separator=%%x00)\", field);\n+\t\tstring_list_append(&log->format, buf.buf)->util = (void*)1;\n+\t\tstrbuf_release(&buf);\n \t} else if (strchrnul(arg, '%')) {\n \t\tlog->groups |= SHORTLOG_GROUP_FORMAT;\n \t\tstring_list_append(&log->format, arg);\n@@ -347,8 +335,6 @@ void shortlog_init(struct shortlog *log)\n \tlog->wrap = DEFAULT_WRAPLEN;\n \tlog->in1 = DEFAULT_INDENT1;\n \tlog->in2 = DEFAULT_INDENT2;\n-\tlog->trailers.strdup_strings = 1;\n-\tlog->trailers.cmp = strcasecmp;\n \tlog->format.strdup_strings = 1;\n }\n \n@@ -434,8 +420,6 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)\n \n \tshortlog_init_group(&log);\n \n-\tstring_list_sort(&log.trailers);\n-\n \t/* assume HEAD if from a tty */\n \tif (!nongit && !rev.pending.nr && isatty(0))\n \t\tadd_head_to_pending(&rev);\ndiff --git a/shortlog.h b/shortlog.h\nindex e52f001fb7..ac844394a8 100644\n--- a/shortlog.h\n+++ b/shortlog.h\n@@ -24,7 +24,6 @@ struct shortlog {\n \t\tSHORTLOG_GROUP_TRAILER = (1 << 2),\n \t\tSHORTLOG_GROUP_FORMAT = (1 << 3),\n \t} groups;\n-\tstruct string_list trailers;\n \tstruct string_list format;\n \n \tint email;\n-- \n2.37.0.1.g1379af2e9d\n"},{"id":"464565","messageId":"814ee4b8d8055269d9206847b0faf839816abf59.1665448437.git.me@ttaylorr.com","threadId":"58597","inReplyTo":"cover.1665448437.git.me@ttaylorr.com","subject":"[PATCH 6/7] shortlog: implement `--group=committer` in terms of `--group=`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-11T00:34:18Z","receivedAt":"2022-10-11T00:35:21Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In the same spirit as the previous commit, reimplement\n`--group=committer` as a special case of `--group=`, too.\n\nSigned-off-by: Taylor Blau \n---\n builtin/shortlog.c | 14 +++-----------\n 1 file changed, 3 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex aac8c7afa4..b46df179fe 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -225,7 +225,6 @@ static void insert_records_from_format(struct shortlog *log,\n \n void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n {\n-\tstruct strbuf ident = STRBUF_INIT;\n \tstruct strbuf oneline = STRBUF_INIT;\n \tstruct strset dups = STRSET_INIT;\n \tstruct pretty_print_context ctx = {0};\n@@ -245,22 +244,12 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n \t}\n \toneline_str = oneline.len ? oneline.buf : \"\";\n \n-\tif (log->groups & SHORTLOG_GROUP_COMMITTER) {\n-\t\tstrbuf_reset(&ident);\n-\t\tformat_commit_message(commit,\n-\t\t\t\t log->email ? \"%cN <%cE>\" : \"%cN\",\n-\t\t\t\t &ident, &ctx);\n-\t\tif (!HAS_MULTI_BITS(log->groups) ||\n-\t\t strset_add(&dups, ident.buf))\n-\t\t\tinsert_one_record(log, ident.buf, oneline_str);\n-\t}\n \tif (log->groups & SHORTLOG_GROUP_TRAILER) {\n \t\tinsert_records_from_trailers(log, &dups, commit, &ctx, oneline_str);\n \t}\n \tinsert_records_from_format(log, &dups, commit, &ctx, oneline_str);\n \n \tstrset_clear(&dups);\n-\tstrbuf_release(&ident);\n \tstrbuf_release(&oneline);\n }\n \n@@ -371,6 +360,9 @@ void shortlog_init_group(struct shortlog *log)\n \tif (log->groups & SHORTLOG_GROUP_AUTHOR)\n \t\tstring_list_append(&log->format,\n \t\t\t\t log->email ? \"%aN <%aE>\" : \"%aN\");\n+\tif (log->groups & SHORTLOG_GROUP_COMMITTER)\n+\t\tstring_list_append(&log->format,\n+\t\t\t\t log->email ? \"%cN <%cE>\" : \"%cN\");\n }\n \n int cmd_shortlog(int argc, const char **argv, const char *prefix)\n-- \n2.37.0.1.g1379af2e9d\n\n"},{"id":"464568","messageId":"Y0S9cZBiLL3GuWm0@coredump.intra.peff.net","threadId":"58597","inReplyTo":"b587b8ea4ab593806b9fb6d1db8751591991455b.1665448437.git.me@ttaylorr.com","subject":"Re: [PATCH 2/7] shortlog: accept `--date`-related options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-11T00:48:49Z","receivedAt":"2022-10-11T00:48:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 10, 2022 at 08:34:05PM -0400, Taylor Blau wrote:\n\n> From: Jeff King \n> \n> Prepare for the future patch which will introduce arbitrary pretty\n> formats via the `--group` argument.\n> \n> To allow additional customizability (for example, to support something\n> like `git shortlog -s --group='%aD' --date='format:%Y-%m' ...` (which\n> groups commits by the datestring 'YYYY-mm' according to author date), we\n> must store off the `--date` parsed from calling `parse_revision_opt()`.\n\nSorry, I haven't had a chance yet to look carefully at the rest of the\nshortlog discussion, but I wanted to note here: this patch also affects\ncustom output formats. So:\n\n - we probably want to note that in the commit message as a limitation\n of this, versus something like %(authordate:format=...). I think\n that's fine, as the same limitation already applies to multiple\n dates in a single format string.\n\n - it's arguably a bug-fix, and can be tested in isolation like:\n\n git shortlog --format='%ad %s' --date=short\n\n which currently ignores the --date option entirely.\n\n-Peff\n"},{"id":"464569","messageId":"Y0S+s04LrQWSeIC+@coredump.intra.peff.net","threadId":"58597","inReplyTo":"eaec59daa178c205a59b779c63e411316d8180ed.1665448437.git.me@ttaylorr.com","subject":"Re: [PATCH 1/7] Documentation: extract date-options.txt","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-11T00:54:11Z","receivedAt":"2022-10-11T00:54:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 10, 2022 at 08:34:02PM -0400, Taylor Blau wrote:\n\n> A future commit will want to include `--date`-related options in the\n> documentation for `git-shortlog(1)`, but without some of the additional\n> baggage in the usual rev-list-options.txt.\n> \n> Extract those options to a separate file in Documentation and include it\n> from its original source in rev-list-options.txt.\n> \n> This patch does not modify the contents of the `--date`-options section\n> of Documentation/rev-list-options.txt.\n\nI think git-shortlog.txt already includes rev-list-options, but the date\nparts are omitted because there's an ifndef::git-shortlog[] around the\nwhole \"commit formatting\" section.\n\nShould we just be loosening the ifndef here?\n\nI think we _could_ include more of the \"formatting\" section overall, but\nit looks like we have a custom mention of \"--format\" in\ngit-shortlog.txt, which I think is a better solution anyway. Maybe we\nshould just do the same with `--date`?\n\n-Peff\n"},{"id":"464572","messageId":"Y0TArbwMho1zHNPj@nand.local","threadId":"58597","inReplyTo":"Y0S+s04LrQWSeIC+@coredump.intra.peff.net","subject":"Re: [PATCH 1/7] Documentation: extract date-options.txt","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-11T01:02:37Z","receivedAt":"2022-10-11T01:02:44Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 10, 2022 at 08:54:11PM -0400, Jeff King wrote:\n> On Mon, Oct 10, 2022 at 08:34:02PM -0400, Taylor Blau wrote:\n>\n> > A future commit will want to include `--date`-related options in the\n> > documentation for `git-shortlog(1)`, but without some of the additional\n> > baggage in the usual rev-list-options.txt.\n> >\n> > Extract those options to a separate file in Documentation and include it\n> > from its original source in rev-list-options.txt.\n> >\n> > This patch does not modify the contents of the `--date`-options section\n> > of Documentation/rev-list-options.txt.\n>\n> I think git-shortlog.txt already includes rev-list-options, but the date\n> parts are omitted because there's an ifndef::git-shortlog[] around the\n> whole \"commit formatting\" section.\n>\n> Should we just be loosening the ifndef here?\n\nThat would do the trick. When I originally wrote it, I thought that\nchopping the section up would be trickier than it actually was, which is\nreally just limited to the following:\n\n--- 8< ---\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 1cb91dfb9c..d5fbde6c80 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -1033,8 +1033,12 @@ endif::git-rev-list[]\n\n include::pretty-options.txt[]\n\n+endif::git-shortlog[]\n+\n include::date-options.txt[]\n\n+ifndef::git-shortlog[]\n+\n ifdef::git-rev-list[]\n --header::\n \tPrint the contents of the commit in raw-format; each record is\n\n--- >8 ---\n\n(Obviously reverting this patch to replace the include of\ndate-options.txt with the old contents that was there before this\npatch).\n\n> I think we _could_ include more of the \"formatting\" section overall, but\n> it looks like we have a custom mention of \"--format\" in\n> git-shortlog.txt, which I think is a better solution anyway. Maybe we\n> should just do the same with `--date`?\n\nThat works for me, too. Let's do that ;-).\n\nThanks,\nTaylor\n"},{"id":"464573","messageId":"Y0TDDvzeCxIMFbG5@coredump.intra.peff.net","threadId":"58597","inReplyTo":"6f38990cc2ea8460ce37437e0770784d9b712dab.1665448437.git.me@ttaylorr.com","subject":"Re: [PATCH 4/7] shortlog: support arbitrary commit format `--group`s","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-11T01:12:46Z","receivedAt":"2022-10-11T01:12:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 10, 2022 at 08:34:12PM -0400, Taylor Blau wrote:\n\n> diff --git a/Documentation/git-shortlog.txt b/Documentation/git-shortlog.txt\n> index 4982ceee21..ca15643f45 100644\n> --- a/Documentation/git-shortlog.txt\n> +++ b/Documentation/git-shortlog.txt\n> @@ -59,6 +59,8 @@ OPTIONS\n> example, if your project uses `Reviewed-by` trailers, you might want\n> to see who has been reviewing with\n> `git shortlog -ns --group=trailer:reviewed-by`.\n> + - ``, any string accepted by the `--format` option of 'git log'.\n> + (See the \"PRETTY FORMATS\" section of linkgit:git-log[1].)\n\nI have a slight preference here to call this:\n\n --group=format:\n\nand then DWIM:\n\n --group=\n\nas long as contains a percent. This is similar to how --pretty\nworks, but leaves us more room for changing things later if we need to.\n\n> +static void insert_records_from_format(struct shortlog *log,\n> +\t\t\t\t struct strset *dups,\n> +\t\t\t\t struct commit *commit,\n> +\t\t\t\t struct pretty_print_context *ctx,\n> +\t\t\t\t const char *oneline)\n> +{\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tstruct string_list_item *item;\n> +\n> +\tfor_each_string_list_item(item, &log->format) {\n> +\t\tstrbuf_reset(&buf);\n> +\n> +\t\tformat_commit_message(commit, item->string, &buf, ctx);\n> +\n> +\t\tif (strset_add(dups, buf.buf))\n> +\t\t\tinsert_one_record(log, buf.buf, oneline);\n> +\t}\n> +\n> +\tstrbuf_release(&buf);\n> +}\n\nVersus the earlier patch we discussed, this is one identity per --group,\nso it matches how --group=trailer works. Good.\n\n> @@ -243,6 +266,7 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n> \tif (log->groups & SHORTLOG_GROUP_TRAILER) {\n> \t\tinsert_records_from_trailers(log, &dups, commit, &ctx, oneline_str);\n> \t}\n> +\tinsert_records_from_format(log, &dups, commit, &ctx, oneline_str);\n\nHmm. I see why we don't need to guard this with:\n\n if (log->groups & SHORTLOG_GROUP_FORMAT)\n\nsince our helper function is a noop if log->format is empty. But I\nwonder if:\n\n - it's more clear to match the others (although it looks like they are\n going away later, so that potentially becomes a non-issue)\n\n - it's useful to have a conditional that lets us skip any setup work.\n For trailers, in particular, we call logmsg_reencode(), which is\n potentially expensive. OTOH, it would be easy for the helper\n function to just return early when log->format.nr is 0.\n\nSo I'll reserve judgement until seeing the end state.\n\n> diff --git a/shortlog.h b/shortlog.h\n> index dc388dd459..4850a8c30f 100644\n> --- a/shortlog.h\n> +++ b/shortlog.h\n> @@ -22,8 +22,10 @@ struct shortlog {\n> \t\tSHORTLOG_GROUP_AUTHOR = (1 << 0),\n> \t\tSHORTLOG_GROUP_COMMITTER = (1 << 1),\n> \t\tSHORTLOG_GROUP_TRAILER = (1 << 2),\n> +\t\tSHORTLOG_GROUP_FORMAT = (1 << 3),\n> \t} groups;\n> \tstruct string_list trailers;\n> +\tstruct string_list format;\n\nGiven that \"shortlog --format\" does a totally different thing, I wonder\nif this needs a more descriptive name like group_format or something.\nProbably not. That other format feature is all in rev_info, and it's not\nlike \"trailers\" above doesn't have a similar issue.\n\n> diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh\n> index 3095b1b2ff..2417ac8896 100755\n> --- a/t/t4201-shortlog.sh\n> +++ b/t/t4201-shortlog.sh\n> @@ -237,6 +237,15 @@ test_expect_success 'shortlog --group=trailer:signed-off-by' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'shortlog --group=' '\n> +\tgit shortlog -s --date=\"format:%Y\" --group=\"%cN (%cd)\" HEAD >actual &&\n> +\tcat >expect <<-\\EOF &&\n> +\t 4\tC O Mitter (2005)\n> +\t 1\tSin Nombre (2005)\n> +\tEOF\n> +\ttest_cmp expect actual\n> +'\n\nMakes sense. Two other tests that might be worth including:\n\n - \"shortlog --group=bogus\" generates an error (we might already have\n such a test; I didn't check)\n\n - since the multiple-option behavior is so subtle, maybe show a case\n where two formats partially overlap. A plausible one is \"--group=%aN\n --group=%cN\", but the test setup might need tweaked to cover both.\n There's an existing \"multiple groups\" test that might come in handy.\n\n-Peff\n"},{"id":"464574","messageId":"Y0TDhP8O3xZsC/vv@nand.local","threadId":"58597","inReplyTo":"Y0S9cZBiLL3GuWm0@coredump.intra.peff.net","subject":"Re: [PATCH 2/7] shortlog: accept `--date`-related options","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-11T01:14:44Z","receivedAt":"2022-10-11T01:14:53Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 10, 2022 at 08:48:49PM -0400, Jeff King wrote:\n> - it's arguably a bug-fix, and can be tested in isolation like:\n>\n> git shortlog --format='%ad %s' --date=short\n>\n> which currently ignores the --date option entirely.\n\nGood call-out, and I agree with this reasoning. I updated the patch\naccordingly, thanks!\n\nThanks,\nTaylor\n"},{"id":"464575","messageId":"Y0TF0M6UzLS9r6iM@nand.local","threadId":"58597","inReplyTo":"Y0TDDvzeCxIMFbG5@coredump.intra.peff.net","subject":"Re: [PATCH 4/7] shortlog: support arbitrary commit format `--group`s","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-11T01:24:32Z","receivedAt":"2022-10-11T01:25:34Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 10, 2022 at 09:12:46PM -0400, Jeff King wrote:\n> On Mon, Oct 10, 2022 at 08:34:12PM -0400, Taylor Blau wrote:\n>\n> > diff --git a/Documentation/git-shortlog.txt b/Documentation/git-shortlog.txt\n> > index 4982ceee21..ca15643f45 100644\n> > --- a/Documentation/git-shortlog.txt\n> > +++ b/Documentation/git-shortlog.txt\n> > @@ -59,6 +59,8 @@ OPTIONS\n> > example, if your project uses `Reviewed-by` trailers, you might want\n> > to see who has been reviewing with\n> > `git shortlog -ns --group=trailer:reviewed-by`.\n> > + - ``, any string accepted by the `--format` option of 'git log'.\n> > + (See the \"PRETTY FORMATS\" section of linkgit:git-log[1].)\n>\n> I have a slight preference here to call this:\n>\n> --group=format:\n\nThat seems very reasonable, thanks!\n\n> > @@ -243,6 +266,7 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n> > \tif (log->groups & SHORTLOG_GROUP_TRAILER) {\n> > \t\tinsert_records_from_trailers(log, &dups, commit, &ctx, oneline_str);\n> > \t}\n> > +\tinsert_records_from_format(log, &dups, commit, &ctx, oneline_str);\n>\n> Hmm. I see why we don't need to guard this with:\n>\n> if (log->groups & SHORTLOG_GROUP_FORMAT)\n>\n> since our helper function is a noop if log->format is empty. But I\n> wonder if:\n>\n> - it's more clear to match the others (although it looks like they are\n> going away later, so that potentially becomes a non-issue)\n>\n> - it's useful to have a conditional that lets us skip any setup work.\n> For trailers, in particular, we call logmsg_reencode(), which is\n> potentially expensive. OTOH, it would be easy for the helper\n> function to just return early when log->format.nr is 0.\n\nIn this case, `insert_records_from_format()` is cheap when\nlog->format.nr is 0. It is limited to setting up a strbuf to\nSTRBUF_INIT, and then calling strbuf_release() on it before returning.\n\nAnd, indeed, the remaining conditionals go away by the final patch, so\nyou may want to decide then if it looks good to you or not.\n\n> > diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh\n> > index 3095b1b2ff..2417ac8896 100755\n> > --- a/t/t4201-shortlog.sh\n> > +++ b/t/t4201-shortlog.sh\n> > @@ -237,6 +237,15 @@ test_expect_success 'shortlog --group=trailer:signed-off-by' '\n> > \ttest_cmp expect actual\n> > '\n> >\n> > +test_expect_success 'shortlog --group=' '\n> > +\tgit shortlog -s --date=\"format:%Y\" --group=\"%cN (%cd)\" HEAD >actual &&\n> > +\tcat >expect <<-\\EOF &&\n> > +\t 4\tC O Mitter (2005)\n> > +\t 1\tSin Nombre (2005)\n> > +\tEOF\n> > +\ttest_cmp expect actual\n> > +'\n>\n> Makes sense. Two other tests that might be worth including:\n>\n> - \"shortlog --group=bogus\" generates an error (we might already have\n> such a test; I didn't check)\n\nWe didn't have such a test before, so adding one is a good call, thanks!\n\n> - since the multiple-option behavior is so subtle, maybe show a case\n> where two formats partially overlap. A plausible one is \"--group=%aN\n> --group=%cN\", but the test setup might need tweaked to cover both.\n> There's an existing \"multiple groups\" test that might come in handy.\n\nInteresting. I was starting to write that test up, but then realized\nthat this will be covered by the end of the series, since the\n`--group=trailer` machinery is reimplemented in terms of the new format\ngroup.\n\nSo if the existing \"shortlog can match multiple groups\" test keeps\npassing, we did our job correctly ;-).\n\nThanks,\nTaylor\n"},{"id":"464576","messageId":"Y0TGuFDhoG1Nu6ND@nand.local","threadId":"58597","inReplyTo":"Y0TF0M6UzLS9r6iM@nand.local","subject":"Re: [PATCH 4/7] shortlog: support arbitrary commit format `--group`s","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-11T01:28:24Z","receivedAt":"2022-10-11T01:28:29Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 10, 2022 at 09:24:32PM -0400, Taylor Blau wrote:\n> > Makes sense. Two other tests that might be worth including:\n> >\n> > - \"shortlog --group=bogus\" generates an error (we might already have\n> > such a test; I didn't check)\n>\n> We didn't have such a test before, so adding one is a good call, thanks!\n\nJust writing back to say that this was a really good suggestion, since\nit caught my mistake in writing:\n\n } else if (strchrnul(arg, '%')) {\n\nsince we'll always get back a non-NULL answer from calling `strchrnul`\ninstead of `strchr`.\n\nThanks ;-).\n\nThanks,\nTaylor\n"},{"id":"464577","messageId":"Y0TIMlrrifYKuBnR@coredump.intra.peff.net","threadId":"58597","inReplyTo":"55a6ef7bc0082818fa51a0915c43002ede5c449f.1665448437.git.me@ttaylorr.com","subject":"Re: [PATCH 5/7] shortlog: implement `--group=author` in terms of `--group=`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-11T01:34:42Z","receivedAt":"2022-10-11T01:35:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 10, 2022 at 08:34:15PM -0400, Taylor Blau wrote:\n\n> Instead of handling SHORTLOG_GROUP_AUTHOR separately, reimplement it as\n> a special case of the new `--group=` mode, where the author mode\n> is a shorthand for `--group='%aN <%aE>'.\n\nOK, this should be a nice cleanup.\n\n> diff --git a/builtin/log.c b/builtin/log.c\n> index ee19dc5d45..6b77e520b5 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -1334,6 +1334,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,\n> \tlog.in2 = 4;\n> \tlog.file = rev->diffopt.file;\n> \tlog.groups = SHORTLOG_GROUP_AUTHOR;\n> +\tshortlog_init_group(&log);\n> \tfor (i = 0; i < nr; i++)\n> \t\tshortlog_add_commit(&log, list[i]);\n\nIn another caller you drop the assignment of log.groups, since\nshortlog_init_group() already does so if log.groups is 0 (which it will\nbe, since shortlog_init() will zero-initialize).\n\nShould we do the same here? Or maybe leaving it is more obvious. It\nwould be more obvious still if we made the helper take the type, like:\n\n shortlog_init_group(&log, SHORTLOG_GROUP_AUTHOR);\n\nBut I guess that is not accurate, as we'd eventually use this in\nshortlog.c to turn _any_ bits we've accumulated in log.group into their\ncorrect formats.\n\nI think the name of the helper function puzzled me a bit. It is really\n\"finish up any setup for the shortlog struct\". We could lazily do it in\nshortlog_add_commit(), but that's pretty subtle. But could we maybe call\nit:\n\n shortlog_finish_setup();\n\nor something? I dunno. I might be nit-picking, but I actually had to\nscratch my head quite a bit to understand what was going on here.\n\n> diff --git a/builtin/shortlog.c b/builtin/shortlog.c\n> index f708d96558..aac8c7afa4 100644\n> --- a/builtin/shortlog.c\n> +++ b/builtin/shortlog.c\n> @@ -245,15 +245,6 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n> \t}\n> \toneline_str = oneline.len ? oneline.buf : \"\";\n> \n> -\tif (log->groups & SHORTLOG_GROUP_AUTHOR) {\n> -\t\tstrbuf_reset(&ident);\n> -\t\tformat_commit_message(commit,\n> -\t\t\t\t log->email ? \"%aN <%aE>\" : \"%aN\",\n> -\t\t\t\t &ident, &ctx);\n> -\t\tif (!HAS_MULTI_BITS(log->groups) ||\n> -\t\t strset_add(&dups, ident.buf))\n> -\t\t\tinsert_one_record(log, ident.buf, oneline_str);\n> -\t}\n\nThis loses the HAS_MULTI_BITS() optimization. The idea there is that if\nyou have a single group-by that can't produce multiple outputs, then\nthere's no need to do duplicate detection.\n\nThe equivalent in an all-formats world is something like:\n\n log.format.nr > 1 && !log.trailers.nr\n\n(because trailers are special in that one trailer key can produce\nmultiple idents for a single commit).\n\n> +void shortlog_init_group(struct shortlog *log)\n> +{\n> +\tif (!log->groups)\n> +\t\tlog->groups = SHORTLOG_GROUP_AUTHOR;\n> +\n> +\tif (log->groups & SHORTLOG_GROUP_AUTHOR)\n> +\t\tstring_list_append(&log->format,\n> +\t\t\t\t log->email ? \"%aN <%aE>\" : \"%aN\");\n> +}\n\nRegardless of the naming suggestion I made, I think things would be more\nobvious if this top conditional remained in cmd_shortlog(). And then the\nexplicit assignment of \"log.group\" in make_cover_letter() would remain.\n\n> @@ -439,8 +440,8 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)\n> \tlog.file = rev.diffopt.file;\n> \tlog.date_mode = rev.date_mode;\n> \n> -\tif (!log.groups)\n> -\t\tlog.groups = SHORTLOG_GROUP_AUTHOR;\n> +\tshortlog_init_group(&log);\n> +\n> \tstring_list_sort(&log.trailers);\n\nNow that we have a \"finish the shortlog init\" function, probably this\ntrailer sort should go into it, too. The current caller doesn't need it,\nbut it removes on more gotcha from using the shortlog API.\n\n-Peff\n"},{"id":"464578","messageId":"Y0TIWhr9PCcZQfK4@coredump.intra.peff.net","threadId":"58597","inReplyTo":"814ee4b8d8055269d9206847b0faf839816abf59.1665448437.git.me@ttaylorr.com","subject":"Re: [PATCH 6/7] shortlog: implement `--group=committer` in terms of `--group=`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-11T01:35:22Z","receivedAt":"2022-10-11T01:36:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 10, 2022 at 08:34:18PM -0400, Taylor Blau wrote:\n\n> In the same spirit as the previous commit, reimplement\n> `--group=committer` as a special case of `--group=`, too.\n\nMakes sense. This all looks as expected, but the same HAS_MULTI_BITS()\ncomment applies.\n\n-Peff\n"},{"id":"464579","messageId":"Y0TLf/J22ioQ5UCt@nand.local","threadId":"58597","inReplyTo":"Y0TIMlrrifYKuBnR@coredump.intra.peff.net","subject":"Re: [PATCH 5/7] shortlog: implement `--group=author` in terms of `--group=`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-11T01:48:47Z","receivedAt":"2022-10-11T01:48:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 10, 2022 at 09:34:42PM -0400, Jeff King wrote:\n> On Mon, Oct 10, 2022 at 08:34:15PM -0400, Taylor Blau wrote:\n>\n> I think the name of the helper function puzzled me a bit. It is really\n> \"finish up any setup for the shortlog struct\". We could lazily do it in\n> shortlog_add_commit(), but that's pretty subtle. But could we maybe call\n> it:\n>\n> shortlog_finish_setup();\n>\n> or something? I dunno. I might be nit-picking, but I actually had to\n> scratch my head quite a bit to understand what was going on here.\n\nI think that shortlog_finish_setup() is probably the most reasonable\nchoice (and I changed it to that locally). I spent a day or so thinking\non and off about this while writing the series, and didn't come up with\nany satisfying names.\n\nI think it points to something deeper about the API that doesn't quite\nsit right with me. But shortlog_finish_setup() is the least-unsatisfying\nname so far, so let's go with that ;-).\n\n> > diff --git a/builtin/shortlog.c b/builtin/shortlog.c\n> > index f708d96558..aac8c7afa4 100644\n> > --- a/builtin/shortlog.c\n> > +++ b/builtin/shortlog.c\n> > @@ -245,15 +245,6 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n> > \t}\n> > \toneline_str = oneline.len ? oneline.buf : \"\";\n> >\n> > -\tif (log->groups & SHORTLOG_GROUP_AUTHOR) {\n> > -\t\tstrbuf_reset(&ident);\n> > -\t\tformat_commit_message(commit,\n> > -\t\t\t\t log->email ? \"%aN <%aE>\" : \"%aN\",\n> > -\t\t\t\t &ident, &ctx);\n> > -\t\tif (!HAS_MULTI_BITS(log->groups) ||\n> > -\t\t strset_add(&dups, ident.buf))\n> > -\t\t\tinsert_one_record(log, ident.buf, oneline_str);\n> > -\t}\n>\n> This loses the HAS_MULTI_BITS() optimization. The idea there is that if\n> you have a single group-by that can't produce multiple outputs, then\n> there's no need to do duplicate detection.\n>\n> The equivalent in an all-formats world is something like:\n>\n> log.format.nr > 1 && !log.trailers.nr\n>\n> (because trailers are special in that one trailer key can produce\n> multiple idents for a single commit).\n\nHmm. I suspect that this is going to become more complicated by the time\nthat you read the final patch ;-). Let's wait until then and see what\nyou think.\n\n> > +void shortlog_init_group(struct shortlog *log)\n> > +{\n> > +\tif (!log->groups)\n> > +\t\tlog->groups = SHORTLOG_GROUP_AUTHOR;\n> > +\n> > +\tif (log->groups & SHORTLOG_GROUP_AUTHOR)\n> > +\t\tstring_list_append(&log->format,\n> > +\t\t\t\t log->email ? \"%aN <%aE>\" : \"%aN\");\n> > +}\n>\n> Regardless of the naming suggestion I made, I think things would be more\n> obvious if this top conditional remained in cmd_shortlog(). And then the\n> explicit assignment of \"log.group\" in make_cover_letter() would remain.\n\nYep, works for me.\n\n> > @@ -439,8 +440,8 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)\n> > \tlog.file = rev.diffopt.file;\n> > \tlog.date_mode = rev.date_mode;\n> >\n> > -\tif (!log.groups)\n> > -\t\tlog.groups = SHORTLOG_GROUP_AUTHOR;\n> > +\tshortlog_init_group(&log);\n> > +\n> > \tstring_list_sort(&log.trailers);\n>\n> Now that we have a \"finish the shortlog init\" function, probably this\n> trailer sort should go into it, too. The current caller doesn't need it,\n> but it removes on more gotcha from using the shortlog API.\n\nWe'll drop this list by the end of the series, too, so it probably isn't\nworth moving it into shortlog_finish_setup() in the interim.\n\n> -Peff\n\nThanks,\nTaylor\n"},{"id":"464580","messageId":"Y0TL8unKev9TPytJ@coredump.intra.peff.net","threadId":"58597","inReplyTo":"02adc297e7661cbc25302b9f5659d2356b8b5008.1665448437.git.me@ttaylorr.com","subject":"Re: [PATCH 7/7] shortlog: implement `--group=trailer` in terms of `--group=`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-11T01:50:42Z","receivedAt":"2022-10-11T01:50:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 10, 2022 at 08:34:21PM -0400, Taylor Blau wrote:\n\n> In the same spirit as the previous commit, reimplement\n> `--group=trailer:` as a special case of `--group=`, too.\n> \n> Unsurprisingly, this reimplementation is a little bit more complicated\n> than the previous two. The complexity stems from having to enumerate\n> each of the trailers one-by-one, as well as delegating control to\n> `parse_ident()` to handle whether or not to show an individual's email.\n> \n> To enumerate each trailer in a commit, we set the separator used in the\n> pretty format to be a NUL byte. This hack allows us to treat the strbuf\n> from `format_commit_message()` as an array of strings.\n\nHmph. So that's _probably_ OK, but I have to wonder if this is going too\nfar. The resulting code is not that much shorter, and IMHO is actually a\nlittle more complicated, because of this hack and the extra util bit.\n\nI also would be curious if there is any speed difference between the\ntwo.\n\nI spent a fair bit of time optimizing the regular author/committer paths\n(and I think if we restore the skip-the-dedup logic I mentioned earlier\nfor those, they should be equivalent, as they already use\nformat_commit_message()). But the internals of the trailer code are\nalready so slow I doubt it makes much of a difference either way here.\n\nThe one thing that could hurt is that multiple trailers now require\nmultiple format calls, which may load the commit object for each one. I\nthink _probably_ not because we'd hopefully have it cached in the commit\nslab via the save_commit_buffer mechanism. Though I guess if we used the\ncommit graph in the first place, we would need to load it. We'd also\nlikely reencode, but that's a noop on utf8 commits, so it's probably not\nworth worrying about.\n\n-Peff\n\nPS That last paragraph gives me the suspicion that there's some\n low-hanging optimization fruit here: we want save_commit_buffer on in\n general, but after we visit each commit, we probably should call\n free_commit_buffer() on it to get rid of it, like we do for git-log.\n Probably shortlog is keeping all of the commit messages in memory at\n once (though of course not if the commit-graph is in use).\n\n If the commit-graph _is_ in use, there's probably the opposite\n optimization we could be doing: we should make sure to load it once,\n then do all of the formats, and then unuse it.\n\n Those don't have to be part of your series, but might be worth\n looking into while you're in the area.\n"},{"id":"464581","messageId":"Y0TNgxYFDMvZW/VS@coredump.intra.peff.net","threadId":"58597","inReplyTo":"Y0TL8unKev9TPytJ@coredump.intra.peff.net","subject":"Re: [PATCH 7/7] shortlog: implement `--group=trailer` in terms of `--group=`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-11T01:57:23Z","receivedAt":"2022-10-11T01:57:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 10, 2022 at 09:50:42PM -0400, Jeff King wrote:\n\n> I spent a fair bit of time optimizing the regular author/committer paths\n> (and I think if we restore the skip-the-dedup logic I mentioned earlier\n> for those, they should be equivalent, as they already use\n> format_commit_message()). But the internals of the trailer code are\n> already so slow I doubt it makes much of a difference either way here.\n> \n> The one thing that could hurt is that multiple trailers now require\n> multiple format calls, which may load the commit object for each one. I\n> think _probably_ not because we'd hopefully have it cached in the commit\n> slab via the save_commit_buffer mechanism. Though I guess if we used the\n> commit graph in the first place, we would need to load it. We'd also\n> likely reencode, but that's a noop on utf8 commits, so it's probably not\n> worth worrying about.\n\nLooking at it more carefully, your new code also does an individual\ntrailer parse for each trailer. And that code is also really slow, and\nbetter if we call it only once.\n\n-Peff\n"},{"id":"464582","messageId":"Y0TOpVF+Y70YJHzx@coredump.intra.peff.net","threadId":"58597","inReplyTo":"Y0TF0M6UzLS9r6iM@nand.local","subject":"Re: [PATCH 4/7] shortlog: support arbitrary commit format `--group`s","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-11T02:02:13Z","receivedAt":"2022-10-11T02:02:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 10, 2022 at 09:24:32PM -0400, Taylor Blau wrote:\n\n> > Hmm. I see why we don't need to guard this with:\n> >\n> > if (log->groups & SHORTLOG_GROUP_FORMAT)\n> >\n> > since our helper function is a noop if log->format is empty. But I\n> > wonder if:\n> >\n> > - it's more clear to match the others (although it looks like they are\n> > going away later, so that potentially becomes a non-issue)\n> >\n> > - it's useful to have a conditional that lets us skip any setup work.\n> > For trailers, in particular, we call logmsg_reencode(), which is\n> > potentially expensive. OTOH, it would be easy for the helper\n> > function to just return early when log->format.nr is 0.\n> \n> In this case, `insert_records_from_format()` is cheap when\n> log->format.nr is 0. It is limited to setting up a strbuf to\n> STRBUF_INIT, and then calling strbuf_release() on it before returning.\n> \n> And, indeed, the remaining conditionals go away by the final patch, so\n> you may want to decide then if it looks good to you or not.\n\nRight. Having read to the end, a few new thoughts:\n\n - I'm skeptical on the conversion of --group=trailer to use the\n formats. It seems more complicated. The others seem fine.\n\n As a result, I do think it's awkward to check bits for log.format\n (since now there are many), and we should just enter the helper\n function unconditionally. I do think it's a little weird to check\n the TRAILER bit just before it though (assuming we'd leave that in\n place). But it would be natural for us to just return early and skip\n any setup work in insert_records_from_trailers(). I.e., something\n like this:\n\n diff --git a/builtin/shortlog.c b/builtin/shortlog.c\n index 086dfee45a..19d953c26a 100644\n --- a/builtin/shortlog.c\n +++ b/builtin/shortlog.c\n @@ -170,6 +170,9 @@ static void insert_records_from_trailers(struct shortlog *log,\n \tconst char *commit_buffer, *body;\n \tstruct strbuf ident = STRBUF_INIT;\n \n +\tif (!log->trailers.nr)\n +\t\treturn;\n +\n \t/*\n \t * Using format_commit_message(\"%B\") would be simpler here, but\n \t * this saves us copying the message.\n @@ -240,9 +243,7 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n \t\t strset_add(&dups, ident.buf))\n \t\t\tinsert_one_record(log, ident.buf, oneline_str);\n \t}\n -\tif (log->groups & SHORTLOG_GROUP_TRAILER) {\n -\t\tinsert_records_from_trailers(log, &dups, commit, &ctx, oneline_str);\n -\t}\n +\tinsert_records_from_trailers(log, &dups, commit, &ctx, oneline_str);\n \n \tstrset_clear(&dups);\n \tstrbuf_release(&ident);\n\n - I mentioned possible setup work. And indeed, I think it may be\n useful for insert_records_from_format() to load and cache the commit\n message once via get_commit_buffer(), since that result could then\n be used by all formats. But that is not really specific to\n --group=format! The existing code would benefit for any multi-group\n invocation. So any such setup should probably be at the top of\n shortlog_add_commit() anyway.\n\n> > - since the multiple-option behavior is so subtle, maybe show a case\n> > where two formats partially overlap. A plausible one is \"--group=%aN\n> > --group=%cN\", but the test setup might need tweaked to cover both.\n> > There's an existing \"multiple groups\" test that might come in handy.\n> \n> Interesting. I was starting to write that test up, but then realized\n> that this will be covered by the end of the series, since the\n> `--group=trailer` machinery is reimplemented in terms of the new format\n> group.\n\nTrue, if we follow through on that. ;)\n\n-Peff\n"},{"id":"464583","messageId":"Y0TO2I3PmUjJ5926@coredump.intra.peff.net","threadId":"58597","inReplyTo":"Y0TGuFDhoG1Nu6ND@nand.local","subject":"Re: [PATCH 4/7] shortlog: support arbitrary commit format `--group`s","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-11T02:03:04Z","receivedAt":"2022-10-11T02:03:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 10, 2022 at 09:28:24PM -0400, Taylor Blau wrote:\n\n> On Mon, Oct 10, 2022 at 09:24:32PM -0400, Taylor Blau wrote:\n> > > Makes sense. Two other tests that might be worth including:\n> > >\n> > > - \"shortlog --group=bogus\" generates an error (we might already have\n> > > such a test; I didn't check)\n> >\n> > We didn't have such a test before, so adding one is a good call, thanks!\n> \n> Just writing back to say that this was a really good suggestion, since\n> it caught my mistake in writing:\n> \n> } else if (strchrnul(arg, '%')) {\n> \n> since we'll always get back a non-NULL answer from calling `strchrnul`\n> instead of `strchr`.\n\nI'm going to pretend that I noticed that bug and was gently leading you\nto it, rather than that I was oblivious and lucky.\n\n-Peff\n"},{"id":"464584","messageId":"Y0TRxDRFRgFonhtH@coredump.intra.peff.net","threadId":"58597","inReplyTo":"Y0TLf/J22ioQ5UCt@nand.local","subject":"Re: [PATCH 5/7] shortlog: implement `--group=author` in terms of `--group=`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-11T02:15:32Z","receivedAt":"2022-10-11T02:15:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 10, 2022 at 09:48:47PM -0400, Taylor Blau wrote:\n\n> I think that shortlog_finish_setup() is probably the most reasonable\n> choice (and I changed it to that locally). I spent a day or so thinking\n> on and off about this while writing the series, and didn't come up with\n> any satisfying names.\n> \n> I think it points to something deeper about the API that doesn't quite\n> sit right with me. But shortlog_finish_setup() is the least-unsatisfying\n> name so far, so let's go with that ;-).\n\nYeah, touching that block in make_cover_letter() definitely tickled my\nmemory that there was some awkwardness there. That's when I added\nshortlog_init() at all (which is good, because otherwise you'd have had\nan uninitialized log.format string-list).\n\nI think the general pattern of:\n\n foo_init();\n foo.option = whatever;\n foo_finish_setup();\n\n foo_do_the_thing();\n\nis OK. It's a little complicated, but it gives callers the freedom to\ntweak options with a lot of flexibility (e.g., based on command line\nconfig, command line options, etc). We have a similar pattern with\ndiff_setup_done().\n\nThe other option is for any option-tweaking to go through methods which\nmaintain invariants (like if GROUP_AUTHOR is set, then the \"%an\" format\nhas been added). That's usually the \"right\" object-oriented way to do\nit. But I think even in this simple case it gets awkward, because the\ncorrect format can't be known until you see whether log->email is set.\nSo it really has to be a \"finalize\" step after both log->email and\nlog->group are set.\n\n> > This loses the HAS_MULTI_BITS() optimization. The idea there is that if\n> > you have a single group-by that can't produce multiple outputs, then\n> > there's no need to do duplicate detection.\n> >\n> > The equivalent in an all-formats world is something like:\n> >\n> > log.format.nr > 1 && !log.trailers.nr\n> >\n> > (because trailers are special in that one trailer key can produce\n> > multiple idents for a single commit).\n> \n> Hmm. I suspect that this is going to become more complicated by the time\n> that you read the final patch ;-). Let's wait until then and see what\n> you think.\n\nYes, it's pretty gross with the trailer util thing. You'd probably want\nto do something like:\n\n static int needs_dedup(const struct string_list *formats)\n {\n\tstruct string_list_item *item;\n\tif (formats->nr > 1)\n\t\treturn 1;\n\tfor_each_string_list_item(item, formats) {\n\t\tif (item->util)\n\t\t\treturn 1;\n\t}\n\treturn 0;\n }\n\nand perhaps call it only once and cache the result, rather than\niterating for each commit/format.\n\nBut if we leave trailers as is, then the logic is much easier. And\nimplementing the optimization for --group=format should fall out pretty\nnaturally (and that both preserves it for --group=author, and extends it\nto --group=format, which I should have noticed when reviewing patch 4).\n\n> > > @@ -439,8 +440,8 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)\n> > > \tlog.file = rev.diffopt.file;\n> > > \tlog.date_mode = rev.date_mode;\n> > >\n> > > -\tif (!log.groups)\n> > > -\t\tlog.groups = SHORTLOG_GROUP_AUTHOR;\n> > > +\tshortlog_init_group(&log);\n> > > +\n> > > \tstring_list_sort(&log.trailers);\n> >\n> > Now that we have a \"finish the shortlog init\" function, probably this\n> > trailer sort should go into it, too. The current caller doesn't need it,\n> > but it removes on more gotcha from using the shortlog API.\n> \n> We'll drop this list by the end of the series, too, so it probably isn't\n> worth moving it into shortlog_finish_setup() in the interim.\n\nAh, right. Well, see my other comments. :)\n\n-Peff\n"},{"id":"464598","messageId":"221011.867d16pr0n.gmgdl@evledraar.gmail.com","threadId":"58597","inReplyTo":"c3f50465cba4430b2014a76c5e473b906db50059.1665448437.git.me@ttaylorr.com","subject":"Re: [PATCH 3/7] shortlog: extract `--group` fragment for translation","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-11T10:55:10Z","receivedAt":"2022-10-11T10:57:03Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Oct 10 2022, Taylor Blau wrote:\n\n> The subsequent commit will add another unhandled case in\n> `read_from_stdin()` which will want to use the same message as with\n> `--group=trailer`.\n>\n> Extract the \"--group=trailer\" part from this message so the same\n> translation key can be used for both cases.\n>\n> Signed-off-by: Taylor Blau \n> ---\n> builtin/shortlog.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/shortlog.c b/builtin/shortlog.c\n> index 53c379a51d..051c97bd5a 100644\n> --- a/builtin/shortlog.c\n> +++ b/builtin/shortlog.c\n> @@ -132,7 +132,7 @@ static void read_from_stdin(struct shortlog *log)\n> \t\tmatch = committer_match;\n> \t\tbreak;\n> \tcase SHORTLOG_GROUP_TRAILER:\n> -\t\tdie(_(\"using --group=trailer with stdin is not supported\"));\n> +\t\tdie(_(\"using %s with stdin is not supported\"), \"--group=trailer\");\n> \tdefault:\n> \t\tBUG(\"unhandled shortlog group\");\n> \t}\n\nRather than add another translation that you can use in 2x places here\n(with 4/7) instead do:\n\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 7a1e1fe7c0e..59aef24f637 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -132,7 +132,8 @@ static void read_from_stdin(struct shortlog *log)\n \t\tmatch = committer_match;\n \t\tbreak;\n \tcase SHORTLOG_GROUP_TRAILER:\n-\t\tdie(_(\"using --group=trailer with stdin is not supported\"));\n+\t\tdie(_(\"options '%s' and '%s' cannot be used together\"),\n+\t\t \"--group=\", \"--stdin\");\n \tdefault:\n \t\tBUG(\"unhandled shortlog group\");\n \t}\n\nWhich as you can see with \"git grep 'cannot be used together'\" we\nalready use in a lot of places.\n\nWe should probably extract this to a:\n\n\tdie_opt_2_incompatible(\"--group=\", \"--stdin\");\n\nBut that can all be done in some hypothetical future, but for now the\nstatus quo is that we know of these common strings, and copy/paste them\nin various files.\n"},{"id":"464599","messageId":"221011.8635bupqx1.gmgdl@evledraar.gmail.com","threadId":"58597","inReplyTo":"55a6ef7bc0082818fa51a0915c43002ede5c449f.1665448437.git.me@ttaylorr.com","subject":"Re: [PATCH 5/7] shortlog: implement `--group=author` in terms of `--group=`","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-11T10:57:41Z","receivedAt":"2022-10-11T10:59:13Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Oct 10 2022, Taylor Blau wrote:\n\n> Instead of handling SHORTLOG_GROUP_AUTHOR separately, reimplement it as\n> a special case of the new `--group=` mode, where the author mode\n> is a shorthand for `--group='%aN <%aE>'.\n>\n> Note that we still need to keep the SHORTLOG_GROUP_AUTHOR enum since it\n> has a different meaning in `read_from_stdin()`, where it is still used\n> for a different purpose.\n>\n> Signed-off-by: Taylor Blau \n> ---\n> builtin/log.c | 1 +\n> builtin/shortlog.c | 23 ++++++++++++-----------\n> shortlog.h | 1 +\n> 3 files changed, 14 insertions(+), 11 deletions(-)\n>\n> diff --git a/builtin/log.c b/builtin/log.c\n> index ee19dc5d45..6b77e520b5 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -1334,6 +1334,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,\n> \tlog.in2 = 4;\n> \tlog.file = rev->diffopt.file;\n> \tlog.groups = SHORTLOG_GROUP_AUTHOR;\n> +\tshortlog_init_group(&log);\n> \tfor (i = 0; i < nr; i++)\n> \t\tshortlog_add_commit(&log, list[i]);\n> \n> diff --git a/builtin/shortlog.c b/builtin/shortlog.c\n> index f708d96558..aac8c7afa4 100644\n> --- a/builtin/shortlog.c\n> +++ b/builtin/shortlog.c\n> @@ -245,15 +245,6 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n> \t}\n> \toneline_str = oneline.len ? oneline.buf : \"\";\n> \n> -\tif (log->groups & SHORTLOG_GROUP_AUTHOR) {\n> -\t\tstrbuf_reset(&ident);\n> -\t\tformat_commit_message(commit,\n> -\t\t\t\t log->email ? \"%aN <%aE>\" : \"%aN\",\n> -\t\t\t\t &ident, &ctx);\n> -\t\tif (!HAS_MULTI_BITS(log->groups) ||\n> -\t\t strset_add(&dups, ident.buf))\n> -\t\t\tinsert_one_record(log, ident.buf, oneline_str);\n> -\t}\n> \tif (log->groups & SHORTLOG_GROUP_COMMITTER) {\n> \t\tstrbuf_reset(&ident);\n> \t\tformat_commit_message(commit,\n> @@ -372,6 +363,16 @@ void shortlog_init(struct shortlog *log)\n> \tlog->format.strdup_strings = 1;\n> }\n> \n> +void shortlog_init_group(struct shortlog *log)\n> +{\n> +\tif (!log->groups)\n> +\t\tlog->groups = SHORTLOG_GROUP_AUTHOR;\n> +\n> +\tif (log->groups & SHORTLOG_GROUP_AUTHOR)\n\nNit (easier reading):\n\n\tif (!x)\n\t...\n\telse if (x & FLAG)\n\t...\n"},{"id":"464600","messageId":"221011.86y1tmobxn.gmgdl@evledraar.gmail.com","threadId":"58597","inReplyTo":"221011.8635bupqx1.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 5/7] shortlog: implement `--group=author` in terms of `--group=`","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-11T11:07:37Z","receivedAt":"2022-10-11T11:08:10Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Oct 11 2022, Ævar Arnfjörð Bjarmason wrote:\n\n> On Mon, Oct 10 2022, Taylor Blau wrote:\n\n>> +void shortlog_init_group(struct shortlog *log)\n>> +{\n>> +\tif (!log->groups)\n>> +\t\tlog->groups = SHORTLOG_GROUP_AUTHOR;\n>> +\n>> +\tif (log->groups & SHORTLOG_GROUP_AUTHOR)\n>\n> Nit (easier reading):\n>\n> \tif (!x)\n> \t...\n> \telse if (x & FLAG)\n> \t...\n\nUrgh, sorry about the noise. That's an obvious logic error, I don't know\nwhat I was thinking...\n"},{"id":"464609","messageId":"Y0Vuf2quUoypZyws@coredump.intra.peff.net","threadId":"58597","inReplyTo":"221011.867d16pr0n.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 3/7] shortlog: extract `--group` fragment for translation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-11T13:24:15Z","receivedAt":"2022-10-11T13:24:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 11, 2022 at 12:55:10PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > \tcase SHORTLOG_GROUP_TRAILER:\n> > -\t\tdie(_(\"using --group=trailer with stdin is not supported\"));\n> > +\t\tdie(_(\"using %s with stdin is not supported\"), \"--group=trailer\");\n> > \tdefault:\n> > \t\tBUG(\"unhandled shortlog group\");\n> > \t}\n> \n> Rather than add another translation that you can use in 2x places here\n> (with 4/7) instead do:\n> \n> diff --git a/builtin/shortlog.c b/builtin/shortlog.c\n> index 7a1e1fe7c0e..59aef24f637 100644\n> --- a/builtin/shortlog.c\n> +++ b/builtin/shortlog.c\n> @@ -132,7 +132,8 @@ static void read_from_stdin(struct shortlog *log)\n> \t\tmatch = committer_match;\n> \t\tbreak;\n> \tcase SHORTLOG_GROUP_TRAILER:\n> -\t\tdie(_(\"using --group=trailer with stdin is not supported\"));\n> +\t\tdie(_(\"options '%s' and '%s' cannot be used together\"),\n> +\t\t \"--group=\", \"--stdin\");\n> \tdefault:\n> \t\tBUG(\"unhandled shortlog group\");\n> \t}\n\nIt's not usually --stdin, though. Generally you'd just not provide any\nrevisions to traverse, and it defaults to stdin. So \"with stdin\" is IMHO\na more accurate message, as it covers all of the cases.\n\n-Peff\n"},{"id":"465394","messageId":"Y1H4MeYXv06mjKg0@nand.local","threadId":"58597","inReplyTo":"Y0TL8unKev9TPytJ@coredump.intra.peff.net","subject":"Re: [PATCH 7/7] shortlog: implement `--group=trailer` in terms of `--group=`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-21T01:38:57Z","receivedAt":"2022-10-21T01:39:05Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 10, 2022 at 09:50:42PM -0400, Jeff King wrote:\n> Hmph. So that's _probably_ OK, but I have to wonder if this is going too\n> far. The resulting code is not that much shorter, and IMHO is actually a\n> little more complicated, because of this hack and the extra util bit.\n\nThanks for a careful analysis, as always. \"Additional complexity\" and\n\"slower\" are both usually not worth it in isolation, but it's definitely\ndifficult to advocate for doing both at the same time ;-).\n\nLet's drop this patch and let the `--group=trailer` bits continue to\nbenefit from your optimizations.\n\nThanks,\nTaylor\n"},{"id":"465397","messageId":"Y1H9xdDJS+xKW8mS@nand.local","threadId":"58597","inReplyTo":"Y0TLf/J22ioQ5UCt@nand.local","subject":"Re: [PATCH 5/7] shortlog: implement `--group=author` in terms of `--group=`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-21T02:02:45Z","receivedAt":"2022-10-21T02:02:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 10, 2022 at 09:48:47PM -0400, Taylor Blau wrote:\n> > > diff --git a/builtin/shortlog.c b/builtin/shortlog.c\n> > > index f708d96558..aac8c7afa4 100644\n> > > --- a/builtin/shortlog.c\n> > > +++ b/builtin/shortlog.c\n> > > @@ -245,15 +245,6 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n> > > \t}\n> > > \toneline_str = oneline.len ? oneline.buf : \"\";\n> > >\n> > > -\tif (log->groups & SHORTLOG_GROUP_AUTHOR) {\n> > > -\t\tstrbuf_reset(&ident);\n> > > -\t\tformat_commit_message(commit,\n> > > -\t\t\t\t log->email ? \"%aN <%aE>\" : \"%aN\",\n> > > -\t\t\t\t &ident, &ctx);\n> > > -\t\tif (!HAS_MULTI_BITS(log->groups) ||\n> > > -\t\t strset_add(&dups, ident.buf))\n> > > -\t\t\tinsert_one_record(log, ident.buf, oneline_str);\n> > > -\t}\n> >\n> > This loses the HAS_MULTI_BITS() optimization. The idea there is that if\n> > you have a single group-by that can't produce multiple outputs, then\n> > there's no need to do duplicate detection.\n> >\n> > The equivalent in an all-formats world is something like:\n> >\n> > log.format.nr > 1 && !log.trailers.nr\n> >\n> > (because trailers are special in that one trailer key can produce\n> > multiple idents for a single commit).\n\nHmm. Shouldn't that \"&& !log.trailers.nr\" be an \"|| log.trailers.nr\"? It\ndoesn't seem to make sense to say \"there are things that could produce\nmultiple outputs\" if there's more than one format _and_ no trailers.\n\nThe logic should read \"there are things that could produce multiple\noutputs if there is more than one format *or* at least one trailer\".\n\nSo I think the right change would be:\n\n--- >8 ---\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 95ceab7649..7e1b56e2aa 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -216,7 +216,8 @@ static void insert_records_from_format(struct shortlog *log,\n\n \t\tformat_commit_message(commit, item->string, &buf, ctx);\n\n-\t\tif (strset_add(dups, buf.buf))\n+\t\tif (!(log->format.nr > 1 || log->trailers.nr) ||\n+\t\t strset_add(dups, buf.buf))\n \t\t\tinsert_one_record(log, buf.buf, oneline);\n \t}\n--- 8< ---\n\nYeah?\n\nThanks,\nTaylor\n"},{"id":"465398","messageId":"Y1H+DyZknfcQ+YAV@nand.local","threadId":"58597","inReplyTo":"Y0TRxDRFRgFonhtH@coredump.intra.peff.net","subject":"Re: [PATCH 5/7] shortlog: implement `--group=author` in terms of `--group=`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-21T02:03:59Z","receivedAt":"2022-10-21T02:04:07Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 10, 2022 at 10:15:32PM -0400, Jeff King wrote:\n> > > > @@ -439,8 +440,8 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)\n> > > > \tlog.file = rev.diffopt.file;\n> > > > \tlog.date_mode = rev.date_mode;\n> > > >\n> > > > -\tif (!log.groups)\n> > > > -\t\tlog.groups = SHORTLOG_GROUP_AUTHOR;\n> > > > +\tshortlog_init_group(&log);\n> > > > +\n> > > > \tstring_list_sort(&log.trailers);\n> > >\n> > > Now that we have a \"finish the shortlog init\" function, probably this\n> > > trailer sort should go into it, too. The current caller doesn't need it,\n> > > but it removes on more gotcha from using the shortlog API.\n> >\n> > We'll drop this list by the end of the series, too, so it probably isn't\n> > worth moving it into shortlog_finish_setup() in the interim.\n>\n> Ah, right. Well, see my other comments. :)\n\nSeen ;-). Now that this series no longer touches the log.trailers bits,\nwe should move this around. Done locally, will send a reroll shortly or\ntomorrow.\n\nThanks,\nTaylor\n"},{"id":"465399","messageId":"Y1IGeuudJj18sDPz@nand.local","threadId":"58597","inReplyTo":"Y0TOpVF+Y70YJHzx@coredump.intra.peff.net","subject":"Re: [PATCH 4/7] shortlog: support arbitrary commit format `--group`s","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-21T02:39:54Z","receivedAt":"2022-10-21T02:40:02Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 10, 2022 at 10:02:13PM -0400, Jeff King wrote:\n> > > - since the multiple-option behavior is so subtle, maybe show a case\n> > > where two formats partially overlap. A plausible one is \"--group=%aN\n> > > --group=%cN\", but the test setup might need tweaked to cover both.\n> > > There's an existing \"multiple groups\" test that might come in handy.\n> >\n> > Interesting. I was starting to write that test up, but then realized\n> > that this will be covered by the end of the series, since the\n> > `--group=trailer` machinery is reimplemented in terms of the new format\n> > group.\n>\n> True, if we follow through on that. ;)\n\nSo, obviously we ended up not following through on that ;-).\n\nI tried to leverage the existing test as much as possible, to the point\nthat the new test is pretty hacky. But I think that the result is cute,\nand it does save us from having to modify the downstream tests (or\ncreate unreachable commits, initialize another repository, etc).\n\nIt looks something like this:\n\n--- 8< ---\ndiff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh\nindex 0abe5354fc..566d581e1b 100755\n--- a/t/t4201-shortlog.sh\n+++ b/t/t4201-shortlog.sh\n@@ -356,6 +356,19 @@ test_expect_success 'shortlog can match multiple groups' '\n \ttest_cmp expect actual\n '\n\n+test_expect_success 'shortlog can match multiple format groups' '\n+\tcat >expect <<-\\EOF &&\n+\t 2\tUser B \n+\t 1\tA U Thor \n+\t 1\tUser A \n+\tEOF\n+\tgit shortlog -ns \\\n+\t\t--group=\"%(trailers:valueonly,separator=%0x00,key=some-trailer)\" \\\n+\t\t--group=\"%(trailers:valueonly,separator=%0x00,key=another-trailer)\" \\\n+\t\t-2 HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'set up option selection tests' '\n \tgit commit --allow-empty -F - <<-\\EOF\n \tsubject\n--- >8 ---\n\nThe gross bit there really is the 'separator=%0x00' thing, since the\n\"trailers\" pretty format gives us a NL character already. I suppose that\nyou could do something like this on top instead:\n\n--- >8 ---\ndiff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh\nindex 566d581e1b..13ac0bac64 100755\n--- a/t/t4201-shortlog.sh\n+++ b/t/t4201-shortlog.sh\n@@ -363,9 +363,10 @@ test_expect_success 'shortlog can match multiple format groups' '\n \t 1\tUser A \n \tEOF\n \tgit shortlog -ns \\\n-\t\t--group=\"%(trailers:valueonly,separator=%0x00,key=some-trailer)\" \\\n-\t\t--group=\"%(trailers:valueonly,separator=%0x00,key=another-trailer)\" \\\n-\t\t-2 HEAD >actual &&\n+\t\t--group=\"%(trailers:valueonly,key=some-trailer)\" \\\n+\t\t--group=\"%(trailers:valueonly,key=another-trailer)\" \\\n+\t\t-2 HEAD >actual.raw &&\n+\tgrep -v \"^$\" actual.raw >actual &&\n \ttest_cmp expect actual\n '\n--- 8< ---\n\nwhich I think I prefer slightly.\n\nIf this is all too hacky for your (or anybody's!) taste, let me know.\nBut I think ultimately this results in a smaller, more easily digestible\ndiff overall.\n\nThanks,\nTaylor\n"},{"id":"465401","messageId":"cover.1666320509.git.me@ttaylorr.com","threadId":"58597","inReplyTo":"cover.1665448437.git.me@ttaylorr.com","subject":"[PATCH v2 0/6] shortlog: introduce `--group=`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-21T03:11:25Z","receivedAt":"2022-10-21T03:11:51Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Here is a reroll of my series to implement arbitrary pretty formats as\nshortlog `--group`'s, based on a suggestion from Jacob Stopak.\n\nThe changes are somewhat minimal, including a rebase onto the current\ntip of master. Less minimal, however, is dropping the reimplementation\nof `--group=trailer:` in terms of the format group, since this\nended up being more trouble than it was worth.\n\nThere are also a handful of small tweaks throughout based on feedback\nfrom Peff.\n\nThanks in advance for your review.\n\nJeff King (1):\n shortlog: accept `--date`-related options\n\nTaylor Blau (5):\n shortlog: make trailer insertion a noop when appropriate\n shortlog: extract `--group` fragment for translation\n shortlog: support arbitrary commit format `--group`s\n shortlog: implement `--group=author` in terms of `--group=`\n shortlog: implement `--group=committer` in terms of `--group=`\n\n Documentation/git-shortlog.txt | 8 ++++\n builtin/log.c | 1 +\n builtin/shortlog.c | 83 +++++++++++++++++++++++-----------\n shortlog.h | 5 ++\n t/t4201-shortlog.sh | 51 +++++++++++++++++++++\n 5 files changed, 121 insertions(+), 27 deletions(-)\n\nRange-diff against v1:\n1: eaec59daa1 < -: ---------- Documentation: extract date-options.txt\n2: b587b8ea4a ! 1: 58baccbaa8 shortlog: accept `--date`-related options\n @@ Metadata\n ## Commit message ##\n shortlog: accept `--date`-related options\n \n - Prepare for the future patch which will introduce arbitrary pretty\n - formats via the `--group` argument.\n + Prepare for a future patch which will introduce arbitrary pretty formats\n + via the `--group` argument.\n \n To allow additional customizability (for example, to support something\n like `git shortlog -s --group='%aD' --date='format:%Y-%m' ...` (which\n groups commits by the datestring 'YYYY-mm' according to author date), we\n must store off the `--date` parsed from calling `parse_revision_opt()`.\n \n + Note that this also affects custom output `--format` strings in `git\n + shortlog`. Though this is a behavior change, this is arguably fixing a\n + long-standing bug (ie., that `--format` strings are not affected by\n + `--date` specifiers as they should be).\n +\n Signed-off-by: Jeff King \n Signed-off-by: Taylor Blau \n \n ## Documentation/git-shortlog.txt ##\n -@@ Documentation/git-shortlog.txt: options or the revision range, when confusion arises.\n - :git-shortlog: 1\n - include::rev-list-options.txt[]\n +@@ Documentation/git-shortlog.txt: OPTIONS\n \n -+include::date-options.txt[]\n + \tEach pretty-printed commit will be rewrapped before it is shown.\n + \n ++--date=::\n ++\tWith a `--group=format:`, show dates formatted\n ++\taccording to the given date string. (See the `--date` options\n ++\tin the \"COMMIT FORMATTING\" section of linkgit:git-log[1].)\n +\n - MAPPING AUTHORS\n - ---------------\n - \n + --group=::\n + \tGroup commits based on ``. If no `--group` option is\n + \tspecified, the default is `author`. `` is one of:\n \n ## builtin/shortlog.c ##\n @@ builtin/shortlog.c: void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n @@ shortlog.h: struct shortlog {\n \n \tenum {\n \t\tSHORTLOG_GROUP_AUTHOR = (1 << 0),\n +\n + ## t/t4201-shortlog.sh ##\n +@@ t/t4201-shortlog.sh: test_expect_success 'pretty format' '\n + \ttest_cmp expect log.predictable\n + '\n + \n ++test_expect_success 'pretty format (with --date)' '\n ++\tsed \"s/SUBJECT/2005-04-07 OBJECT_NAME/\" expect.template >expect &&\n ++\tgit shortlog --format=\"%ad %H\" --date=short HEAD >log &&\n ++\tfuzz log >log.predictable &&\n ++\ttest_cmp expect log.predictable\n ++'\n ++\n + test_expect_success '--abbrev' '\n + \tsed s/SUBJECT/OBJID/ expect.template >expect &&\n + \tgit shortlog --format=\"%h\" --abbrev=35 HEAD >log &&\n-: ---------- > 2: 7decccad7c shortlog: make trailer insertion a noop when appropriate\n3: c3f50465cb = 3: 3665488fc9 shortlog: extract `--group` fragment for translation\n4: 6f38990cc2 ! 4: 4a36c0ca4e shortlog: support arbitrary commit format `--group`s\n @@ Documentation/git-shortlog.txt: OPTIONS\n example, if your project uses `Reviewed-by` trailers, you might want\n to see who has been reviewing with\n `git shortlog -ns --group=trailer:reviewed-by`.\n -+ - ``, any string accepted by the `--format` option of 'git log'.\n -+ (See the \"PRETTY FORMATS\" section of linkgit:git-log[1].)\n ++ - `format:`, any string accepted by the `--format` option of\n ++ 'git log'. (See the \"PRETTY FORMATS\" section of\n ++ linkgit:git-log[1].)\n +\n Note that commits that do not include the trailer will not be counted.\n Likewise, commits with multiple trailers (e.g., multiple signoffs) may\n @@ builtin/shortlog.c: static void insert_records_from_trailers(struct shortlog *lo\n +\n +\t\tformat_commit_message(commit, item->string, &buf, ctx);\n +\n -+\t\tif (strset_add(dups, buf.buf))\n ++\t\tif (!(log->format.nr > 1 || log->trailers.nr) ||\n ++\t\t strset_add(dups, buf.buf))\n +\t\t\tinsert_one_record(log, buf.buf, oneline);\n +\t}\n +\n @@ builtin/shortlog.c: static void insert_records_from_trailers(struct shortlog *lo\n {\n \tstruct strbuf ident = STRBUF_INIT;\n @@ builtin/shortlog.c: void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n - \tif (log->groups & SHORTLOG_GROUP_TRAILER) {\n - \t\tinsert_records_from_trailers(log, &dups, commit, &ctx, oneline_str);\n + \t\t\tinsert_one_record(log, ident.buf, oneline_str);\n \t}\n + \tinsert_records_from_trailers(log, &dups, commit, &ctx, oneline_str);\n +\tinsert_records_from_format(log, &dups, commit, &ctx, oneline_str);\n \n \tstrset_clear(&dups);\n @@ builtin/shortlog.c: static int parse_group_option(const struct option *opt, cons\n \t\tlog->groups |= SHORTLOG_GROUP_TRAILER;\n \t\tstring_list_append(&log->trailers, field);\n -\t} else\n -+\t} else if (strchrnul(arg, '%')) {\n ++\t} else if (skip_prefix(arg, \"format:\", &field)) {\n ++\t\tlog->groups |= SHORTLOG_GROUP_FORMAT;\n ++\t\tstring_list_append(&log->format, field);\n ++\t} else if (strchr(arg, '%')) {\n +\t\tlog->groups |= SHORTLOG_GROUP_FORMAT;\n +\t\tstring_list_append(&log->format, arg);\n +\t} else {\n @@ t/t4201-shortlog.sh: test_expect_success 'shortlog --group=trailer:signed-off-by\n \ttest_cmp expect actual\n '\n \n -+test_expect_success 'shortlog --group=' '\n ++test_expect_success 'shortlog --group=format' '\n ++\tgit shortlog -s --date=\"format:%Y\" --group=\"format:%cN (%cd)\" \\\n ++\t\tHEAD >actual &&\n ++\tcat >expect <<-\\EOF &&\n ++\t 4\tC O Mitter (2005)\n ++\t 1\tSin Nombre (2005)\n ++\tEOF\n ++\ttest_cmp expect actual\n ++'\n ++\n ++test_expect_success 'shortlog --group= DWIM' '\n +\tgit shortlog -s --date=\"format:%Y\" --group=\"%cN (%cd)\" HEAD >actual &&\n ++\ttest_cmp expect actual\n ++'\n ++\n ++test_expect_success 'shortlog multiple --group=format' '\n ++\tgit shortlog -s --date=\"format:%Y\" --group=\"format:%cN (%cd)\" \\\n ++\t\tHEAD >actual &&\n +\tcat >expect <<-\\EOF &&\n +\t 4\tC O Mitter (2005)\n +\t 1\tSin Nombre (2005)\n +\tEOF\n +\ttest_cmp expect actual\n +'\n ++\n ++test_expect_success 'shortlog bogus --group' '\n ++\ttest_must_fail git shortlog --group=bogus HEAD 2>err &&\n ++\tgrep \"unknown group type\" err\n ++'\n +\n test_expect_success 'trailer idents are split' '\n \tcat >expect <<-\\EOF &&\n \t 2\tC O Mitter\n +@@ t/t4201-shortlog.sh: test_expect_success 'shortlog can match multiple groups' '\n + \ttest_cmp expect actual\n + '\n + \n ++test_expect_success 'shortlog can match multiple format groups' '\n ++\tcat >expect <<-\\EOF &&\n ++\t 2\tUser B \n ++\t 1\tA U Thor \n ++\t 1\tUser A \n ++\tEOF\n ++\tgit shortlog -ns \\\n ++\t\t--group=\"%(trailers:valueonly,key=some-trailer)\" \\\n ++\t\t--group=\"%(trailers:valueonly,key=another-trailer)\" \\\n ++\t\t-2 HEAD >actual.raw &&\n ++\tgrep -v \"^$\" actual.raw >actual &&\n ++\ttest_cmp expect actual\n ++'\n ++\n + test_expect_success 'set up option selection tests' '\n + \tgit commit --allow-empty -F - <<-\\EOF\n + \tsubject\n5: 55a6ef7bc0 ! 5: f0044682be shortlog: implement `--group=author` in terms of `--group=`\n @@ builtin/log.c: static void make_cover_letter(struct rev_info *rev, int use_separ\n \tlog.in2 = 4;\n \tlog.file = rev->diffopt.file;\n \tlog.groups = SHORTLOG_GROUP_AUTHOR;\n -+\tshortlog_init_group(&log);\n ++\tshortlog_finish_setup(&log);\n \tfor (i = 0; i < nr; i++)\n \t\tshortlog_add_commit(&log, list[i]);\n \n @@ builtin/shortlog.c: void shortlog_init(struct shortlog *log)\n \tlog->format.strdup_strings = 1;\n }\n \n -+void shortlog_init_group(struct shortlog *log)\n ++void shortlog_finish_setup(struct shortlog *log)\n +{\n -+\tif (!log->groups)\n -+\t\tlog->groups = SHORTLOG_GROUP_AUTHOR;\n -+\n +\tif (log->groups & SHORTLOG_GROUP_AUTHOR)\n +\t\tstring_list_append(&log->format,\n +\t\t\t\t log->email ? \"%aN <%aE>\" : \"%aN\");\n ++\n ++\tstring_list_sort(&log->trailers);\n +}\n +\n int cmd_shortlog(int argc, const char **argv, const char *prefix)\n {\n \tstruct shortlog log = { STRING_LIST_INIT_NODUP };\n @@ builtin/shortlog.c: int cmd_shortlog(int argc, const char **argv, const char *prefix)\n - \tlog.file = rev.diffopt.file;\n - \tlog.date_mode = rev.date_mode;\n \n --\tif (!log.groups)\n --\t\tlog.groups = SHORTLOG_GROUP_AUTHOR;\n -+\tshortlog_init_group(&log);\n -+\n - \tstring_list_sort(&log.trailers);\n + \tif (!log.groups)\n + \t\tlog.groups = SHORTLOG_GROUP_AUTHOR;\n +-\tstring_list_sort(&log.trailers);\n ++\tshortlog_finish_setup(&log);\n \n \t/* assume HEAD if from a tty */\n + \tif (!nongit && !rev.pending.nr && isatty(0))\n \n ## shortlog.h ##\n @@ shortlog.h: struct shortlog {\n };\n \n void shortlog_init(struct shortlog *log);\n -+void shortlog_init_group(struct shortlog *log);\n ++void shortlog_finish_setup(struct shortlog *log);\n \n void shortlog_add_commit(struct shortlog *log, struct commit *commit);\n \n6: 814ee4b8d8 ! 6: 6a9e204177 shortlog: implement `--group=committer` in terms of `--group=`\n @@ builtin/shortlog.c: void shortlog_add_commit(struct shortlog *log, struct commit\n -\t\t strset_add(&dups, ident.buf))\n -\t\t\tinsert_one_record(log, ident.buf, oneline_str);\n -\t}\n - \tif (log->groups & SHORTLOG_GROUP_TRAILER) {\n - \t\tinsert_records_from_trailers(log, &dups, commit, &ctx, oneline_str);\n - \t}\n + \tinsert_records_from_trailers(log, &dups, commit, &ctx, oneline_str);\n \tinsert_records_from_format(log, &dups, commit, &ctx, oneline_str);\n \n \tstrset_clear(&dups);\n @@ builtin/shortlog.c: void shortlog_add_commit(struct shortlog *log, struct commit\n \tstrbuf_release(&oneline);\n }\n \n -@@ builtin/shortlog.c: void shortlog_init_group(struct shortlog *log)\n +@@ builtin/shortlog.c: void shortlog_finish_setup(struct shortlog *log)\n \tif (log->groups & SHORTLOG_GROUP_AUTHOR)\n \t\tstring_list_append(&log->format,\n \t\t\t\t log->email ? \"%aN <%aE>\" : \"%aN\");\n +\tif (log->groups & SHORTLOG_GROUP_COMMITTER)\n +\t\tstring_list_append(&log->format,\n +\t\t\t\t log->email ? \"%cN <%cE>\" : \"%cN\");\n - }\n \n - int cmd_shortlog(int argc, const char **argv, const char *prefix)\n + \tstring_list_sort(&log->trailers);\n + }\n7: 02adc297e7 < -: ---------- shortlog: implement `--group=trailer` in terms of `--group=`\n-- \n2.38.0.16.g393fd4c6db\n"},{"id":"465402","messageId":"58baccbaa8612adae55f776ce10806809861270c.1666320509.git.me@ttaylorr.com","threadId":"58597","inReplyTo":"cover.1666320509.git.me@ttaylorr.com","subject":"[PATCH v2 1/6] shortlog: accept `--date`-related options","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-21T03:11:29Z","receivedAt":"2022-10-21T03:11:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"From: Jeff King \n\nPrepare for a future patch which will introduce arbitrary pretty formats\nvia the `--group` argument.\n\nTo allow additional customizability (for example, to support something\nlike `git shortlog -s --group='%aD' --date='format:%Y-%m' ...` (which\ngroups commits by the datestring 'YYYY-mm' according to author date), we\nmust store off the `--date` parsed from calling `parse_revision_opt()`.\n\nNote that this also affects custom output `--format` strings in `git\nshortlog`. Though this is a behavior change, this is arguably fixing a\nlong-standing bug (ie., that `--format` strings are not affected by\n`--date` specifiers as they should be).\n\nSigned-off-by: Jeff King \nSigned-off-by: Taylor Blau \n---\n Documentation/git-shortlog.txt | 5 +++++\n builtin/shortlog.c | 3 ++-\n shortlog.h | 2 ++\n t/t4201-shortlog.sh | 7 +++++++\n 4 files changed, 16 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-shortlog.txt b/Documentation/git-shortlog.txt\nindex f64e77047b..311c041c06 100644\n--- a/Documentation/git-shortlog.txt\n+++ b/Documentation/git-shortlog.txt\n@@ -47,6 +47,11 @@ OPTIONS\n \n \tEach pretty-printed commit will be rewrapped before it is shown.\n \n+--date=::\n+\tWith a `--group=format:`, show dates formatted\n+\taccording to the given date string. (See the `--date` options\n+\tin the \"COMMIT FORMATTING\" section of linkgit:git-log[1].)\n+\n --group=::\n \tGroup commits based on ``. If no `--group` option is\n \tspecified, the default is `author`. `` is one of:\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 7a1e1fe7c0..53c379a51d 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -211,7 +211,7 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n \tctx.fmt = CMIT_FMT_USERFORMAT;\n \tctx.abbrev = log->abbrev;\n \tctx.print_email_subject = 1;\n-\tctx.date_mode.type = DATE_NORMAL;\n+\tctx.date_mode = log->date_mode;\n \tctx.output_encoding = get_log_output_encoding();\n \n \tif (!log->summary) {\n@@ -407,6 +407,7 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)\n \tlog.user_format = rev.commit_format == CMIT_FMT_USERFORMAT;\n \tlog.abbrev = rev.abbrev;\n \tlog.file = rev.diffopt.file;\n+\tlog.date_mode = rev.date_mode;\n \n \tif (!log.groups)\n \t\tlog.groups = SHORTLOG_GROUP_AUTHOR;\ndiff --git a/shortlog.h b/shortlog.h\nindex 3f7e9aabca..dc388dd459 100644\n--- a/shortlog.h\n+++ b/shortlog.h\n@@ -2,6 +2,7 @@\n #define SHORTLOG_H\n \n #include \"string-list.h\"\n+#include \"date.h\"\n \n struct commit;\n \n@@ -15,6 +16,7 @@ struct shortlog {\n \tint in2;\n \tint user_format;\n \tint abbrev;\n+\tstruct date_mode date_mode;\n \n \tenum {\n \t\tSHORTLOG_GROUP_AUTHOR = (1 << 0),\ndiff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh\nindex 3095b1b2ff..7547da539d 100755\n--- a/t/t4201-shortlog.sh\n+++ b/t/t4201-shortlog.sh\n@@ -83,6 +83,13 @@ test_expect_success 'pretty format' '\n \ttest_cmp expect log.predictable\n '\n \n+test_expect_success 'pretty format (with --date)' '\n+\tsed \"s/SUBJECT/2005-04-07 OBJECT_NAME/\" expect.template >expect &&\n+\tgit shortlog --format=\"%ad %H\" --date=short HEAD >log &&\n+\tfuzz log >log.predictable &&\n+\ttest_cmp expect log.predictable\n+'\n+\n test_expect_success '--abbrev' '\n \tsed s/SUBJECT/OBJID/ expect.template >expect &&\n \tgit shortlog --format=\"%h\" --abbrev=35 HEAD >log &&\n-- \n2.38.0.16.g393fd4c6db\n\n"},{"id":"465403","messageId":"7decccad7ccb39eed67bab87ed665beccf5d392f.1666320509.git.me@ttaylorr.com","threadId":"58597","inReplyTo":"cover.1666320509.git.me@ttaylorr.com","subject":"[PATCH v2 2/6] shortlog: make trailer insertion a noop when appropriate","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-21T03:11:32Z","receivedAt":"2022-10-21T03:11:56Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When there are no trailers to insert, it is natural that\ninsert_records_from_trailers() should return without having done any\nwork.\n\nBut instead we guard this call unnecessarily by first checking whether\n`log->groups` has the `SHORTLOG_GROUP_TRAILER` bit set.\n\nPrepare to match a similar pattern in the future where a function which\ninserts records of a certain type does no work when no specifiers\nmatching that type are given.\n\nSigned-off-by: Taylor Blau \n---\n builtin/shortlog.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 53c379a51d..18f0800c82 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -170,6 +170,9 @@ static void insert_records_from_trailers(struct shortlog *log,\n \tconst char *commit_buffer, *body;\n \tstruct strbuf ident = STRBUF_INIT;\n \n+\tif (!log->trailers.nr)\n+\t\treturn;\n+\n \t/*\n \t * Using format_commit_message(\"%B\") would be simpler here, but\n \t * this saves us copying the message.\n@@ -240,9 +243,7 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n \t\t strset_add(&dups, ident.buf))\n \t\t\tinsert_one_record(log, ident.buf, oneline_str);\n \t}\n-\tif (log->groups & SHORTLOG_GROUP_TRAILER) {\n-\t\tinsert_records_from_trailers(log, &dups, commit, &ctx, oneline_str);\n-\t}\n+\tinsert_records_from_trailers(log, &dups, commit, &ctx, oneline_str);\n \n \tstrset_clear(&dups);\n \tstrbuf_release(&ident);\n-- \n2.38.0.16.g393fd4c6db\n\n"},{"id":"465404","messageId":"3665488fc95ee3b097f2e0e1e64d23d523c073fa.1666320509.git.me@ttaylorr.com","threadId":"58597","inReplyTo":"cover.1666320509.git.me@ttaylorr.com","subject":"[PATCH v2 3/6] shortlog: extract `--group` fragment for translation","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-21T03:11:35Z","receivedAt":"2022-10-21T03:11:58Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The subsequent commit will add another unhandled case in\n`read_from_stdin()` which will want to use the same message as with\n`--group=trailer`.\n\nExtract the \"--group=trailer\" part from this message so the same\ntranslation key can be used for both cases.\n\nSigned-off-by: Taylor Blau \n---\n builtin/shortlog.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 18f0800c82..d0645769d7 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -132,7 +132,7 @@ static void read_from_stdin(struct shortlog *log)\n \t\tmatch = committer_match;\n \t\tbreak;\n \tcase SHORTLOG_GROUP_TRAILER:\n-\t\tdie(_(\"using --group=trailer with stdin is not supported\"));\n+\t\tdie(_(\"using %s with stdin is not supported\"), \"--group=trailer\");\n \tdefault:\n \t\tBUG(\"unhandled shortlog group\");\n \t}\n-- \n2.38.0.16.g393fd4c6db\n\n"},{"id":"465405","messageId":"4a36c0ca4e840d53e2fd257e2d97498ced6fb28c.1666320509.git.me@ttaylorr.com","threadId":"58597","inReplyTo":"cover.1666320509.git.me@ttaylorr.com","subject":"[PATCH v2 4/6] shortlog: support arbitrary commit format `--group`s","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-21T03:11:38Z","receivedAt":"2022-10-21T03:11:59Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In addition to generating a shortlog based on committer, author, or the\nidentity in one or more specified trailers, it can be useful to generate\na shortlog based on an arbitrary commit format.\n\nThis can be used, for example, to generate a distribution of commit\nactivity over time, like so:\n\n $ git shortlog --group='%cd' --date='format:%Y-%m' -s v2.37.0..\n 117 2022-06\n 274 2022-07\n 324 2022-08\n 263 2022-09\n 7 2022-10\n\nArbitrary commit formats can be used. In fact, `git shortlog`'s default\nbehavior (to count by commit authors) can be emulated as follows:\n\n $ git shortlog --group='%aN <%aE>' ...\n\nand future patches will make the default behavior (as well as\n`--committer`, and `--group=trailer:`) special cases of the\nmore flexible `--group` option.\n\nNote also that the SHORTLOG_GROUP_FORMAT enum value is used only to\ndesignate that `--group:` is in use when in stdin mode to\ndeclare that the combination is invalid.\n\nSigned-off-by: Taylor Blau \n---\n Documentation/git-shortlog.txt | 3 +++\n builtin/shortlog.c | 37 +++++++++++++++++++++++++++-\n shortlog.h | 2 ++\n t/t4201-shortlog.sh | 44 ++++++++++++++++++++++++++++++++++\n 4 files changed, 85 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-shortlog.txt b/Documentation/git-shortlog.txt\nindex 311c041c06..8cf5a2e3c7 100644\n--- a/Documentation/git-shortlog.txt\n+++ b/Documentation/git-shortlog.txt\n@@ -64,6 +64,9 @@ OPTIONS\n example, if your project uses `Reviewed-by` trailers, you might want\n to see who has been reviewing with\n `git shortlog -ns --group=trailer:reviewed-by`.\n+ - `format:`, any string accepted by the `--format` option of\n+ 'git log'. (See the \"PRETTY FORMATS\" section of\n+ linkgit:git-log[1].)\n +\n Note that commits that do not include the trailer will not be counted.\n Likewise, commits with multiple trailers (e.g., multiple signoffs) may\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex d0645769d7..0358397a7b 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -133,6 +133,8 @@ static void read_from_stdin(struct shortlog *log)\n \t\tbreak;\n \tcase SHORTLOG_GROUP_TRAILER:\n \t\tdie(_(\"using %s with stdin is not supported\"), \"--group=trailer\");\n+\tcase SHORTLOG_GROUP_FORMAT:\n+\t\tdie(_(\"using %s with stdin is not supported\"), \"--group=format\");\n \tdefault:\n \t\tBUG(\"unhandled shortlog group\");\n \t}\n@@ -203,6 +205,28 @@ static void insert_records_from_trailers(struct shortlog *log,\n \tunuse_commit_buffer(commit, commit_buffer);\n }\n \n+static void insert_records_from_format(struct shortlog *log,\n+\t\t\t\t struct strset *dups,\n+\t\t\t\t struct commit *commit,\n+\t\t\t\t struct pretty_print_context *ctx,\n+\t\t\t\t const char *oneline)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct string_list_item *item;\n+\n+\tfor_each_string_list_item(item, &log->format) {\n+\t\tstrbuf_reset(&buf);\n+\n+\t\tformat_commit_message(commit, item->string, &buf, ctx);\n+\n+\t\tif (!(log->format.nr > 1 || log->trailers.nr) ||\n+\t\t strset_add(dups, buf.buf))\n+\t\t\tinsert_one_record(log, buf.buf, oneline);\n+\t}\n+\n+\tstrbuf_release(&buf);\n+}\n+\n void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n {\n \tstruct strbuf ident = STRBUF_INIT;\n@@ -244,6 +268,7 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n \t\t\tinsert_one_record(log, ident.buf, oneline_str);\n \t}\n \tinsert_records_from_trailers(log, &dups, commit, &ctx, oneline_str);\n+\tinsert_records_from_format(log, &dups, commit, &ctx, oneline_str);\n \n \tstrset_clear(&dups);\n \tstrbuf_release(&ident);\n@@ -315,6 +340,7 @@ static int parse_group_option(const struct option *opt, const char *arg, int uns\n \tif (unset) {\n \t\tlog->groups = 0;\n \t\tstring_list_clear(&log->trailers, 0);\n+\t\tstring_list_clear(&log->format, 0);\n \t} else if (!strcasecmp(arg, \"author\"))\n \t\tlog->groups |= SHORTLOG_GROUP_AUTHOR;\n \telse if (!strcasecmp(arg, \"committer\"))\n@@ -322,8 +348,15 @@ static int parse_group_option(const struct option *opt, const char *arg, int uns\n \telse if (skip_prefix(arg, \"trailer:\", &field)) {\n \t\tlog->groups |= SHORTLOG_GROUP_TRAILER;\n \t\tstring_list_append(&log->trailers, field);\n-\t} else\n+\t} else if (skip_prefix(arg, \"format:\", &field)) {\n+\t\tlog->groups |= SHORTLOG_GROUP_FORMAT;\n+\t\tstring_list_append(&log->format, field);\n+\t} else if (strchr(arg, '%')) {\n+\t\tlog->groups |= SHORTLOG_GROUP_FORMAT;\n+\t\tstring_list_append(&log->format, arg);\n+\t} else {\n \t\treturn error(_(\"unknown group type: %s\"), arg);\n+\t}\n \n \treturn 0;\n }\n@@ -341,6 +374,7 @@ void shortlog_init(struct shortlog *log)\n \tlog->in2 = DEFAULT_INDENT2;\n \tlog->trailers.strdup_strings = 1;\n \tlog->trailers.cmp = strcasecmp;\n+\tlog->format.strdup_strings = 1;\n }\n \n int cmd_shortlog(int argc, const char **argv, const char *prefix)\n@@ -481,4 +515,5 @@ void shortlog_output(struct shortlog *log)\n \tlog->list.strdup_strings = 1;\n \tstring_list_clear(&log->list, 1);\n \tclear_mailmap(&log->mailmap);\n+\tstring_list_clear(&log->format, 0);\n }\ndiff --git a/shortlog.h b/shortlog.h\nindex dc388dd459..4850a8c30f 100644\n--- a/shortlog.h\n+++ b/shortlog.h\n@@ -22,8 +22,10 @@ struct shortlog {\n \t\tSHORTLOG_GROUP_AUTHOR = (1 << 0),\n \t\tSHORTLOG_GROUP_COMMITTER = (1 << 1),\n \t\tSHORTLOG_GROUP_TRAILER = (1 << 2),\n+\t\tSHORTLOG_GROUP_FORMAT = (1 << 3),\n \t} groups;\n \tstruct string_list trailers;\n+\tstruct string_list format;\n \n \tint email;\n \tstruct string_list mailmap;\ndiff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh\nindex 7547da539d..13ac0bac64 100755\n--- a/t/t4201-shortlog.sh\n+++ b/t/t4201-shortlog.sh\n@@ -244,6 +244,36 @@ test_expect_success 'shortlog --group=trailer:signed-off-by' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'shortlog --group=format' '\n+\tgit shortlog -s --date=\"format:%Y\" --group=\"format:%cN (%cd)\" \\\n+\t\tHEAD >actual &&\n+\tcat >expect <<-\\EOF &&\n+\t 4\tC O Mitter (2005)\n+\t 1\tSin Nombre (2005)\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'shortlog --group= DWIM' '\n+\tgit shortlog -s --date=\"format:%Y\" --group=\"%cN (%cd)\" HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'shortlog multiple --group=format' '\n+\tgit shortlog -s --date=\"format:%Y\" --group=\"format:%cN (%cd)\" \\\n+\t\tHEAD >actual &&\n+\tcat >expect <<-\\EOF &&\n+\t 4\tC O Mitter (2005)\n+\t 1\tSin Nombre (2005)\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'shortlog bogus --group' '\n+\ttest_must_fail git shortlog --group=bogus HEAD 2>err &&\n+\tgrep \"unknown group type\" err\n+'\n+\n test_expect_success 'trailer idents are split' '\n \tcat >expect <<-\\EOF &&\n \t 2\tC O Mitter\n@@ -326,6 +356,20 @@ test_expect_success 'shortlog can match multiple groups' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'shortlog can match multiple format groups' '\n+\tcat >expect <<-\\EOF &&\n+\t 2\tUser B \n+\t 1\tA U Thor \n+\t 1\tUser A \n+\tEOF\n+\tgit shortlog -ns \\\n+\t\t--group=\"%(trailers:valueonly,key=some-trailer)\" \\\n+\t\t--group=\"%(trailers:valueonly,key=another-trailer)\" \\\n+\t\t-2 HEAD >actual.raw &&\n+\tgrep -v \"^$\" actual.raw >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'set up option selection tests' '\n \tgit commit --allow-empty -F - <<-\\EOF\n \tsubject\n-- \n2.38.0.16.g393fd4c6db\n\n"},{"id":"465406","messageId":"f0044682be70f876f6f92455f1fa46dd2e8aa09f.1666320509.git.me@ttaylorr.com","threadId":"58597","inReplyTo":"cover.1666320509.git.me@ttaylorr.com","subject":"[PATCH v2 5/6] shortlog: implement `--group=author` in terms of `--group=`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-21T03:11:41Z","receivedAt":"2022-10-21T03:12:03Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Instead of handling SHORTLOG_GROUP_AUTHOR separately, reimplement it as\na special case of the new `--group=` mode, where the author mode\nis a shorthand for `--group='%aN <%aE>'.\n\nNote that we still need to keep the SHORTLOG_GROUP_AUTHOR enum since it\nhas a different meaning in `read_from_stdin()`, where it is still used\nfor a different purpose.\n\nSigned-off-by: Taylor Blau \n---\n builtin/log.c | 1 +\n builtin/shortlog.c | 20 ++++++++++----------\n shortlog.h | 1 +\n 3 files changed, 12 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex ee19dc5d45..b4d5420217 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1334,6 +1334,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,\n \tlog.in2 = 4;\n \tlog.file = rev->diffopt.file;\n \tlog.groups = SHORTLOG_GROUP_AUTHOR;\n+\tshortlog_finish_setup(&log);\n \tfor (i = 0; i < nr; i++)\n \t\tshortlog_add_commit(&log, list[i]);\n \ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 0358397a7b..fceb84f12f 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -249,15 +249,6 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n \t}\n \toneline_str = oneline.len ? oneline.buf : \"\";\n \n-\tif (log->groups & SHORTLOG_GROUP_AUTHOR) {\n-\t\tstrbuf_reset(&ident);\n-\t\tformat_commit_message(commit,\n-\t\t\t\t log->email ? \"%aN <%aE>\" : \"%aN\",\n-\t\t\t\t &ident, &ctx);\n-\t\tif (!HAS_MULTI_BITS(log->groups) ||\n-\t\t strset_add(&dups, ident.buf))\n-\t\t\tinsert_one_record(log, ident.buf, oneline_str);\n-\t}\n \tif (log->groups & SHORTLOG_GROUP_COMMITTER) {\n \t\tstrbuf_reset(&ident);\n \t\tformat_commit_message(commit,\n@@ -377,6 +368,15 @@ void shortlog_init(struct shortlog *log)\n \tlog->format.strdup_strings = 1;\n }\n \n+void shortlog_finish_setup(struct shortlog *log)\n+{\n+\tif (log->groups & SHORTLOG_GROUP_AUTHOR)\n+\t\tstring_list_append(&log->format,\n+\t\t\t\t log->email ? \"%aN <%aE>\" : \"%aN\");\n+\n+\tstring_list_sort(&log->trailers);\n+}\n+\n int cmd_shortlog(int argc, const char **argv, const char *prefix)\n {\n \tstruct shortlog log = { STRING_LIST_INIT_NODUP };\n@@ -446,7 +446,7 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)\n \n \tif (!log.groups)\n \t\tlog.groups = SHORTLOG_GROUP_AUTHOR;\n-\tstring_list_sort(&log.trailers);\n+\tshortlog_finish_setup(&log);\n \n \t/* assume HEAD if from a tty */\n \tif (!nongit && !rev.pending.nr && isatty(0))\ndiff --git a/shortlog.h b/shortlog.h\nindex 4850a8c30f..28d04f951a 100644\n--- a/shortlog.h\n+++ b/shortlog.h\n@@ -33,6 +33,7 @@ struct shortlog {\n };\n \n void shortlog_init(struct shortlog *log);\n+void shortlog_finish_setup(struct shortlog *log);\n \n void shortlog_add_commit(struct shortlog *log, struct commit *commit);\n \n-- \n2.38.0.16.g393fd4c6db\n\n"},{"id":"465407","messageId":"6a9e204177d486e786574ec682e6d73907a3ce7e.1666320509.git.me@ttaylorr.com","threadId":"58597","inReplyTo":"cover.1666320509.git.me@ttaylorr.com","subject":"[PATCH v2 6/6] shortlog: implement `--group=committer` in terms of `--group=`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-21T03:11:44Z","receivedAt":"2022-10-21T03:12:04Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In the same spirit as the previous commit, reimplement\n`--group=committer` as a special case of `--group=`, too.\n\nSigned-off-by: Taylor Blau \n---\n builtin/shortlog.c | 14 +++-----------\n 1 file changed, 3 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex fceb84f12f..b9f5af22d3 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -229,7 +229,6 @@ static void insert_records_from_format(struct shortlog *log,\n \n void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n {\n-\tstruct strbuf ident = STRBUF_INIT;\n \tstruct strbuf oneline = STRBUF_INIT;\n \tstruct strset dups = STRSET_INIT;\n \tstruct pretty_print_context ctx = {0};\n@@ -249,20 +248,10 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n \t}\n \toneline_str = oneline.len ? oneline.buf : \"\";\n \n-\tif (log->groups & SHORTLOG_GROUP_COMMITTER) {\n-\t\tstrbuf_reset(&ident);\n-\t\tformat_commit_message(commit,\n-\t\t\t\t log->email ? \"%cN <%cE>\" : \"%cN\",\n-\t\t\t\t &ident, &ctx);\n-\t\tif (!HAS_MULTI_BITS(log->groups) ||\n-\t\t strset_add(&dups, ident.buf))\n-\t\t\tinsert_one_record(log, ident.buf, oneline_str);\n-\t}\n \tinsert_records_from_trailers(log, &dups, commit, &ctx, oneline_str);\n \tinsert_records_from_format(log, &dups, commit, &ctx, oneline_str);\n \n \tstrset_clear(&dups);\n-\tstrbuf_release(&ident);\n \tstrbuf_release(&oneline);\n }\n \n@@ -373,6 +362,9 @@ void shortlog_finish_setup(struct shortlog *log)\n \tif (log->groups & SHORTLOG_GROUP_AUTHOR)\n \t\tstring_list_append(&log->format,\n \t\t\t\t log->email ? \"%aN <%aE>\" : \"%aN\");\n+\tif (log->groups & SHORTLOG_GROUP_COMMITTER)\n+\t\tstring_list_append(&log->format,\n+\t\t\t\t log->email ? \"%cN <%cE>\" : \"%cN\");\n \n \tstring_list_sort(&log->trailers);\n }\n-- \n2.38.0.16.g393fd4c6db\n"},{"id":"465412","messageId":"Y1IoLBvVfo+pIC+6@coredump.intra.peff.net","threadId":"58597","inReplyTo":"Y1H9xdDJS+xKW8mS@nand.local","subject":"Re: [PATCH 5/7] shortlog: implement `--group=author` in terms of `--group=`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-21T05:03:40Z","receivedAt":"2022-10-21T05:03:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 20, 2022 at 10:02:45PM -0400, Taylor Blau wrote:\n\n> > > > -\tif (log->groups & SHORTLOG_GROUP_AUTHOR) {\n> > > > -\t\tstrbuf_reset(&ident);\n> > > > -\t\tformat_commit_message(commit,\n> > > > -\t\t\t\t log->email ? \"%aN <%aE>\" : \"%aN\",\n> > > > -\t\t\t\t &ident, &ctx);\n> > > > -\t\tif (!HAS_MULTI_BITS(log->groups) ||\n> > > > -\t\t strset_add(&dups, ident.buf))\n> > > > -\t\t\tinsert_one_record(log, ident.buf, oneline_str);\n> > > > -\t}\n> > >\n> > > This loses the HAS_MULTI_BITS() optimization. The idea there is that if\n> > > you have a single group-by that can't produce multiple outputs, then\n> > > there's no need to do duplicate detection.\n> > >\n> > > The equivalent in an all-formats world is something like:\n> > >\n> > > log.format.nr > 1 && !log.trailers.nr\n> > >\n> > > (because trailers are special in that one trailer key can produce\n> > > multiple idents for a single commit).\n> \n> Hmm. Shouldn't that \"&& !log.trailers.nr\" be an \"|| log.trailers.nr\"? It\n> doesn't seem to make sense to say \"there are things that could produce\n> multiple outputs\" if there's more than one format _and_ no trailers.\n\nYeah. I was thinking of it as \"is it OK to not de-dup\", but of course it\nis the other way around because of the \"!\". And regardless of which way\nyou write the conditional, the two sides must agree. ;)\n\n> The logic should read \"there are things that could produce multiple\n> outputs if there is more than one format *or* at least one trailer\".\n> \n> So I think the right change would be:\n> \n> --- >8 ---\n> diff --git a/builtin/shortlog.c b/builtin/shortlog.c\n> index 95ceab7649..7e1b56e2aa 100644\n> --- a/builtin/shortlog.c\n> +++ b/builtin/shortlog.c\n> @@ -216,7 +216,8 @@ static void insert_records_from_format(struct shortlog *log,\n> \n> \t\tformat_commit_message(commit, item->string, &buf, ctx);\n> \n> -\t\tif (strset_add(dups, buf.buf))\n> +\t\tif (!(log->format.nr > 1 || log->trailers.nr) ||\n> +\t\t strset_add(dups, buf.buf))\n> \t\t\tinsert_one_record(log, buf.buf, oneline);\n> \t}\n> --- 8< ---\n> \n> Yeah?\n\nRight. I wondered if it might be a little clearer to drop the outer \"!\",\nwhich yields:\n\n if ((log->format.nr <= 1 && !log->trailers.nr) ||\n strset_add(dups, buf.buf))\n\nbut it is not really any less confusing. If we gave that first part of\nthe conditional a name, like:\n\n if (!needs_dedup(log) || strset_add(dups, buf.buf))\n\nmaybe that is better. I dunno.\n\nRegardless, what you wrote above is correct. Thanks for catching it.\n\n-Peff\n"},{"id":"465414","messageId":"Y1IsWt0ZrW+0hy1v@coredump.intra.peff.net","threadId":"58597","inReplyTo":"Y1IGeuudJj18sDPz@nand.local","subject":"Re: [PATCH 4/7] shortlog: support arbitrary commit format `--group`s","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-21T05:21:30Z","receivedAt":"2022-10-21T05:21:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 20, 2022 at 10:39:54PM -0400, Taylor Blau wrote:\n\n> --- 8< ---\n> diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh\n> index 0abe5354fc..566d581e1b 100755\n> --- a/t/t4201-shortlog.sh\n> +++ b/t/t4201-shortlog.sh\n> @@ -356,6 +356,19 @@ test_expect_success 'shortlog can match multiple groups' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'shortlog can match multiple format groups' '\n> +\tcat >expect <<-\\EOF &&\n> +\t 2\tUser B \n> +\t 1\tA U Thor \n> +\t 1\tUser A \n> +\tEOF\n> +\tgit shortlog -ns \\\n> +\t\t--group=\"%(trailers:valueonly,separator=%0x00,key=some-trailer)\" \\\n> +\t\t--group=\"%(trailers:valueonly,separator=%0x00,key=another-trailer)\" \\\n> +\t\t-2 HEAD >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_expect_success 'set up option selection tests' '\n> \tgit commit --allow-empty -F - <<-\\EOF\n> \tsubject\n> --- >8 ---\n> \n> The gross bit there really is the 'separator=%0x00' thing, since the\n> \"trailers\" pretty format gives us a NL character already. I suppose that\n> you could do something like this on top instead:\n\nIMHO the new test should avoid using trailers entirely, to avoid the\nnotion that they are necessary to create the duplicate situation. In a\nnormal repository, the most obvious one is just asking about both\nauthors and committers:\n\n git shortlog --group=format:%cn --group=format:%an\n\nMost commits will have the same value for both, but we want to credit\nthe commit to them only once.\n\nOf course in our fake test-lib world, authors and committers are\ndifferent by default. :-/\n\nBut it's easy enough to make more normal-looking commits, perhaps like:\n\ndiff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh\nindex 0abe5354fc..f0ff8a1714 100755\n--- a/t/t4201-shortlog.sh\n+++ b/t/t4201-shortlog.sh\n@@ -356,6 +356,20 @@ test_expect_success 'shortlog can match multiple groups' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'shortlog can match multiple format groups' '\n+\tGIT_COMMITTER_NAME=$GIT_AUTHOR_NAME \\\n+\t\tgit commit --allow-empty -m \"identical names\" &&\n+\tcat >expect <<-\\EOF &&\n+\t 2\tA U Thor\n+\t 1\tC O Mitter\n+\tEOF\n+\tgit shortlog -ns \\\n+\t\t--group=%cn \\\n+\t\t--group=%an \\\n+\t\t-2 HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'set up option selection tests' '\n \tgit commit --allow-empty -F - <<-\\EOF\n \tsubject\n\nYou could also get fancier by dropping \"-s\" and making sure that the\ncommits were attributed as expected, but I think the count covers\nthings.\n\n-Peff\n"},{"id":"465415","messageId":"Y1Iu1x6dR6GwiA2Y@coredump.intra.peff.net","threadId":"58597","inReplyTo":"58baccbaa8612adae55f776ce10806809861270c.1666320509.git.me@ttaylorr.com","subject":"Re: [PATCH v2 1/6] shortlog: accept `--date`-related options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-21T05:32:07Z","receivedAt":"2022-10-21T05:32:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 20, 2022 at 11:11:29PM -0400, Taylor Blau wrote:\n\n> diff --git a/Documentation/git-shortlog.txt b/Documentation/git-shortlog.txt\n> index f64e77047b..311c041c06 100644\n> --- a/Documentation/git-shortlog.txt\n> +++ b/Documentation/git-shortlog.txt\n> @@ -47,6 +47,11 @@ OPTIONS\n> \n> \tEach pretty-printed commit will be rewrapped before it is shown.\n> \n> +--date=::\n> +\tWith a `--group=format:`, show dates formatted\n> +\taccording to the given date string. (See the `--date` options\n> +\tin the \"COMMIT FORMATTING\" section of linkgit:git-log[1].)\n\nI like this much better than including the whole date-formats section.\nTwo nits:\n\n - s/options/option/ ? I guess you could say there are \"options\" for\n formatting the date, but with \"--date\" it seems like you'd mean the\n singular option.\n\n - Because \"Commit Formatting\" is a subsection in git-log(1), the\n manual will format it with title caps, not all-caps.\n\n> diff --git a/builtin/shortlog.c b/builtin/shortlog.c\n> index 7a1e1fe7c0..53c379a51d 100644\n> --- a/builtin/shortlog.c\n> +++ b/builtin/shortlog.c\n> @@ -211,7 +211,7 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n> \tctx.fmt = CMIT_FMT_USERFORMAT;\n> \tctx.abbrev = log->abbrev;\n> \tctx.print_email_subject = 1;\n> -\tctx.date_mode.type = DATE_NORMAL;\n> +\tctx.date_mode = log->date_mode;\n> \tctx.output_encoding = get_log_output_encoding();\n> \n> \tif (!log->summary) {\n\nAfter the discussion in the last round of whether that funky caller in\nmake_cover_letter() properly initialized the \"struct shortlog\", I\nwondered briefly if this would do the right thing for that caller. And\nit does, because DATE_NORMAL is intentionally set to \"0\" to allow for\nzero-initializing (which is what shortlog_init does).\n\nNot that it matters in practice, because that caller will not group by\nnor format using a date, but I was curious if I had inadvertently\nintroduced a gotcha for future callers. But it is good.\n\n> diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh\n> index 3095b1b2ff..7547da539d 100755\n> --- a/t/t4201-shortlog.sh\n> +++ b/t/t4201-shortlog.sh\n> @@ -83,6 +83,13 @@ test_expect_success 'pretty format' '\n> \ttest_cmp expect log.predictable\n> '\n> \n> +test_expect_success 'pretty format (with --date)' '\n> +\tsed \"s/SUBJECT/2005-04-07 OBJECT_NAME/\" expect.template >expect &&\n> +\tgit shortlog --format=\"%ad %H\" --date=short HEAD >log &&\n> +\tfuzz log >log.predictable &&\n> +\ttest_cmp expect log.predictable\n> +'\n\nAnd this looks sensible. If you dropped \"%H\" then you wouldn't need to\nfuzz, and are still testing the interesting part. But just following the\nwhole template/fuzz thing of the surrounding tests is reasonable.\n\n-Peff\n"},{"id":"465416","messageId":"Y1IwVIoVsl1FVklI@coredump.intra.peff.net","threadId":"58597","inReplyTo":"7decccad7ccb39eed67bab87ed665beccf5d392f.1666320509.git.me@ttaylorr.com","subject":"Re: [PATCH v2 2/6] shortlog: make trailer insertion a noop when appropriate","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-21T05:38:28Z","receivedAt":"2022-10-21T05:38:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 20, 2022 at 11:11:32PM -0400, Taylor Blau wrote:\n\n> When there are no trailers to insert, it is natural that\n> insert_records_from_trailers() should return without having done any\n> work.\n> \n> But instead we guard this call unnecessarily by first checking whether\n> `log->groups` has the `SHORTLOG_GROUP_TRAILER` bit set.\n> \n> Prepare to match a similar pattern in the future where a function which\n> inserts records of a certain type does no work when no specifiers\n> matching that type are given.\n\nThe patch looks good. And knowing what the rest of the series looks\nlike, this is clearly the right thing to do. But I wonder if the\nrationale would be easier if it came at the end. Then rather than\n\"prepare to match a similar pattern in the future\", you can just say\n\"it's weird that we check the bit for SHORTLOG_GROUP_TRAILER but nothing\nelse; let's make them consistent\".\n\nObviously this is a minor nit, and I don't care much either way, as the\nend result is the same. I just think in general that \"this is bad, let's\nmake it better\" makes commit messages easier to write than ones which\nposit some hypothetical future state. ;)\n\n-Peff\n"},{"id":"465417","messageId":"Y1Iwx0uD+wx4jzD/@coredump.intra.peff.net","threadId":"58597","inReplyTo":"3665488fc95ee3b097f2e0e1e64d23d523c073fa.1666320509.git.me@ttaylorr.com","subject":"Re: [PATCH v2 3/6] shortlog: extract `--group` fragment for translation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-21T05:40:23Z","receivedAt":"2022-10-21T05:40:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 20, 2022 at 11:11:35PM -0400, Taylor Blau wrote:\n\n> The subsequent commit will add another unhandled case in\n> `read_from_stdin()` which will want to use the same message as with\n> `--group=trailer`.\n> \n> Extract the \"--group=trailer\" part from this message so the same\n> translation key can be used for both cases.\n\nContinuing my thoughts from the previous patch, this one actually makes\nmore sense to me in the order you have it. I.e., making it more generic\nahead of time, instead of afterwards saying \"hey, these two messages are\nvery similar\".\n\nI'm not sure what makes me see them differently. It's possible I'm\nsimply not consistent and you should ignore me. :)\n\n-Peff\n"},{"id":"465419","messageId":"Y1IyppKatD7piHOO@coredump.intra.peff.net","threadId":"58597","inReplyTo":"4a36c0ca4e840d53e2fd257e2d97498ced6fb28c.1666320509.git.me@ttaylorr.com","subject":"Re: [PATCH v2 4/6] shortlog: support arbitrary commit format `--group`s","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-21T05:48:22Z","receivedAt":"2022-10-21T05:48:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 20, 2022 at 11:11:38PM -0400, Taylor Blau wrote:\n\n> +static void insert_records_from_format(struct shortlog *log,\n> +\t\t\t\t struct strset *dups,\n> +\t\t\t\t struct commit *commit,\n> +\t\t\t\t struct pretty_print_context *ctx,\n> +\t\t\t\t const char *oneline)\n> +{\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tstruct string_list_item *item;\n> +\n> +\tfor_each_string_list_item(item, &log->format) {\n> +\t\tstrbuf_reset(&buf);\n> +\n> +\t\tformat_commit_message(commit, item->string, &buf, ctx);\n> +\n> +\t\tif (!(log->format.nr > 1 || log->trailers.nr) ||\n> +\t\t strset_add(dups, buf.buf))\n> +\t\t\tinsert_one_record(log, buf.buf, oneline);\n> +\t}\n\nJust to be clear, since I talked about this conditional in the other\nthread: what you have here is correct, and now seeing it again in\ncontext, I think it's OK in terms of readability.\n\n> diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh\n> index 7547da539d..13ac0bac64 100755\n> --- a/t/t4201-shortlog.sh\n> +++ b/t/t4201-shortlog.sh\n> @@ -244,6 +244,36 @@ test_expect_success 'shortlog --group=trailer:signed-off-by' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'shortlog --group=format' '\n> +\tgit shortlog -s --date=\"format:%Y\" --group=\"format:%cN (%cd)\" \\\n> +\t\tHEAD >actual &&\n> +\tcat >expect <<-\\EOF &&\n> +\t 4\tC O Mitter (2005)\n> +\t 1\tSin Nombre (2005)\n> +\tEOF\n> +\ttest_cmp expect actual\n> +'\n\nOK, so this is a basic test of the feature, and makes sure the --date\nsupport works.\n\n> +test_expect_success 'shortlog --group= DWIM' '\n> +\tgit shortlog -s --date=\"format:%Y\" --group=\"%cN (%cd)\" HEAD >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nAnd this is the same thing but without \"format:\". Good...\n\n> +test_expect_success 'shortlog multiple --group=format' '\n> +\tgit shortlog -s --date=\"format:%Y\" --group=\"format:%cN (%cd)\" \\\n> +\t\tHEAD >actual &&\n> +\tcat >expect <<-\\EOF &&\n> +\t 4\tC O Mitter (2005)\n> +\t 1\tSin Nombre (2005)\n> +\tEOF\n> +\ttest_cmp expect actual\n> +'\n\n...but this one seems redundant. It is not using multiple formats. And\nyou test that later, in the \"can match multiple format groups\" test. Is\nthis one just leftover from development?\n\n> +test_expect_success 'shortlog bogus --group' '\n> +\ttest_must_fail git shortlog --group=bogus HEAD 2>err &&\n> +\tgrep \"unknown group type\" err\n> +'\n\nThis one is a nice inclusion. I was surprised we didn't have it already. :)\n\n> +test_expect_success 'shortlog can match multiple format groups' '\n> +\tcat >expect <<-\\EOF &&\n> +\t 2\tUser B \n> +\t 1\tA U Thor \n> +\t 1\tUser A \n> +\tEOF\n> +\tgit shortlog -ns \\\n> +\t\t--group=\"%(trailers:valueonly,key=some-trailer)\" \\\n> +\t\t--group=\"%(trailers:valueonly,key=another-trailer)\" \\\n> +\t\t-2 HEAD >actual.raw &&\n> +\tgrep -v \"^$\" actual.raw >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nAnd this one is correct, though I think avoiding trailers is more to the\npoint; see the other mail I sent.\n\n-Peff\n"},{"id":"465420","messageId":"Y1IzQprA+QQFQXGz@coredump.intra.peff.net","threadId":"58597","inReplyTo":"f0044682be70f876f6f92455f1fa46dd2e8aa09f.1666320509.git.me@ttaylorr.com","subject":"Re: [PATCH v2 5/6] shortlog: implement `--group=author` in terms of `--group=`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-21T05:50:58Z","receivedAt":"2022-10-21T05:51:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 20, 2022 at 11:11:41PM -0400, Taylor Blau wrote:\n\n> Instead of handling SHORTLOG_GROUP_AUTHOR separately, reimplement it as\n> a special case of the new `--group=` mode, where the author mode\n> is a shorthand for `--group='%aN <%aE>'.\n> \n> Note that we still need to keep the SHORTLOG_GROUP_AUTHOR enum since it\n> has a different meaning in `read_from_stdin()`, where it is still used\n> for a different purpose.\n\nYep, makes sense. The implementation all looks correct to me, although:\n\n> diff --git a/builtin/log.c b/builtin/log.c\n> index ee19dc5d45..b4d5420217 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -1334,6 +1334,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,\n> \tlog.in2 = 4;\n> \tlog.file = rev->diffopt.file;\n> \tlog.groups = SHORTLOG_GROUP_AUTHOR;\n> +\tshortlog_finish_setup(&log);\n> \tfor (i = 0; i < nr; i++)\n> \t\tshortlog_add_commit(&log, list[i]);\n\nI'd probably have pulled out this \"finish_setup()\" stuff into its own\npatch. I can live with it either way, though. The advantage of having it\nhere is that it becomes obvious why this caller needs it (whereas if\ndone as a preparatory patch, it would just do the trailer sorting, which\nmake_cover_letter() obviously does not need).\n\n-Peff\n"},{"id":"465421","messageId":"Y1IzdY3ECyHTdJCu@coredump.intra.peff.net","threadId":"58597","inReplyTo":"6a9e204177d486e786574ec682e6d73907a3ce7e.1666320509.git.me@ttaylorr.com","subject":"Re: [PATCH v2 6/6] shortlog: implement `--group=committer` in terms of `--group=`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-21T05:51:49Z","receivedAt":"2022-10-21T05:51:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 20, 2022 at 11:11:44PM -0400, Taylor Blau wrote:\n\n> In the same spirit as the previous commit, reimplement\n> `--group=committer` as a special case of `--group=`, too.\n> \n> Signed-off-by: Taylor Blau \n> ---\n> builtin/shortlog.c | 14 +++-----------\n> 1 file changed, 3 insertions(+), 11 deletions(-)\n\nAnd this one is nice and simple, the AUTHOR one having dealt with all of\nthe shortlog_finish_setup() parts for us. :)\n\n-Peff\n"},{"id":"465423","messageId":"Y1Izx2Gw4C7dJEex@coredump.intra.peff.net","threadId":"58597","inReplyTo":"cover.1666320509.git.me@ttaylorr.com","subject":"Re: [PATCH v2 0/6] shortlog: introduce `--group=`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-21T05:53:11Z","receivedAt":"2022-10-21T05:53:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 20, 2022 at 11:11:25PM -0400, Taylor Blau wrote:\n\n> Here is a reroll of my series to implement arbitrary pretty formats as\n> shortlog `--group`'s, based on a suggestion from Jacob Stopak.\n> \n> The changes are somewhat minimal, including a rebase onto the current\n> tip of master. Less minimal, however, is dropping the reimplementation\n> of `--group=trailer:` in terms of the format group, since this\n> ended up being more trouble than it was worth.\n> \n> There are also a handful of small tweaks throughout based on feedback\n> from Peff.\n\nThanks. This mostly looks good to me, though I think there are a few doc\nand test nits that make it worth one more reroll. I had various style\nand ordering suggestions, too. I'll leave it to you whether you think\nit's worth taking any of them during the re-roll, or just ignoring them. :)\n\n-Peff\n"},{"id":"465517","messageId":"Y1MVRh5flnyXiSH8@nand.local","threadId":"58597","inReplyTo":"Y1Iu1x6dR6GwiA2Y@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/6] shortlog: accept `--date`-related options","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-21T21:55:18Z","receivedAt":"2022-10-21T21:55:24Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Oct 21, 2022 at 01:32:07AM -0400, Jeff King wrote:\n> On Thu, Oct 20, 2022 at 11:11:29PM -0400, Taylor Blau wrote:\n>\n> > diff --git a/Documentation/git-shortlog.txt b/Documentation/git-shortlog.txt\n> > index f64e77047b..311c041c06 100644\n> > --- a/Documentation/git-shortlog.txt\n> > +++ b/Documentation/git-shortlog.txt\n> > @@ -47,6 +47,11 @@ OPTIONS\n> >\n> > \tEach pretty-printed commit will be rewrapped before it is shown.\n> >\n> > +--date=::\n> > +\tWith a `--group=format:`, show dates formatted\n> > +\taccording to the given date string. (See the `--date` options\n> > +\tin the \"COMMIT FORMATTING\" section of linkgit:git-log[1].)\n>\n> I like this much better than including the whole date-formats section.\n> Two nits:\n\nThanks, both make sense.\n\nThanks,\nTaylor\n"},{"id":"465518","messageId":"Y1MV4DEa8y0I5W/8@nand.local","threadId":"58597","inReplyTo":"Y1IwVIoVsl1FVklI@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/6] shortlog: make trailer insertion a noop when appropriate","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-21T21:57:52Z","receivedAt":"2022-10-21T21:57:58Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Oct 21, 2022 at 01:38:28AM -0400, Jeff King wrote:\n> On Thu, Oct 20, 2022 at 11:11:32PM -0400, Taylor Blau wrote:\n>\n> > When there are no trailers to insert, it is natural that\n> > insert_records_from_trailers() should return without having done any\n> > work.\n> >\n> > But instead we guard this call unnecessarily by first checking whether\n> > `log->groups` has the `SHORTLOG_GROUP_TRAILER` bit set.\n> >\n> > Prepare to match a similar pattern in the future where a function which\n> > inserts records of a certain type does no work when no specifiers\n> > matching that type are given.\n>\n> The patch looks good. And knowing what the rest of the series looks\n> like, this is clearly the right thing to do. But I wonder if the\n> rationale would be easier if it came at the end. Then rather than\n> \"prepare to match a similar pattern in the future\", you can just say\n> \"it's weird that we check the bit for SHORTLOG_GROUP_TRAILER but nothing\n> else; let's make them consistent\".\n\nHmm. I see what you're saying, but in fact by this point in the series\nthere are two other spots in shortlog_add_commit() that check whether\nindividual bits are set (one for SHORTLOG_GROUP_AUTHOR, and another for\nSHORTLOG_GROUP_COMMITTER).\n\nSo I think that the semi-awkward commit message may in fact be the least\nawkward thing to do here ;-).\n\nThanks,\nTaylor\n"},{"id":"465521","messageId":"Y1MXqATtx5OTQtNU@nand.local","threadId":"58597","inReplyTo":"Y1IoLBvVfo+pIC+6@coredump.intra.peff.net","subject":"Re: [PATCH 5/7] shortlog: implement `--group=author` in terms of `--group=`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-21T22:05:28Z","receivedAt":"2022-10-21T22:05:36Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Oct 21, 2022 at 01:03:40AM -0400, Jeff King wrote:\n> Right. I wondered if it might be a little clearer to drop the outer \"!\",\n> which yields:\n>\n> if ((log->format.nr <= 1 && !log->trailers.nr) ||\n> strset_add(dups, buf.buf))\n>\n> but it is not really any less confusing. If we gave that first part of\n> the conditional a name, like:\n>\n> if (!needs_dedup(log) || strset_add(dups, buf.buf))\n>\n> maybe that is better. I dunno.\n\nYeah, I think that this is best. I briefly wondered whether it would be\nbetter to invert the check _and_ name it, but `if\n(does_not_need_dedup(log))` is too wordy.\n\n> Regardless, what you wrote above is correct. Thanks for catching it.\n\nNo problem, it was an honest mistake.\n\nThanks,\nTaylor\n"},{"id":"465522","messageId":"Y1MYBj5zcPza70/f@nand.local","threadId":"58597","inReplyTo":"Y1IyppKatD7piHOO@coredump.intra.peff.net","subject":"Re: [PATCH v2 4/6] shortlog: support arbitrary commit format `--group`s","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-21T22:07:02Z","receivedAt":"2022-10-21T22:07:07Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Oct 21, 2022 at 01:48:22AM -0400, Jeff King wrote:\n> > +test_expect_success 'shortlog multiple --group=format' '\n> > +\tgit shortlog -s --date=\"format:%Y\" --group=\"format:%cN (%cd)\" \\\n> > +\t\tHEAD >actual &&\n> > +\tcat >expect <<-\\EOF &&\n> > +\t 4\tC O Mitter (2005)\n> > +\t 1\tSin Nombre (2005)\n> > +\tEOF\n> > +\ttest_cmp expect actual\n> > +'\n>\n> ...but this one seems redundant. It is not using multiple formats. And\n> you test that later, in the \"can match multiple format groups\" test. Is\n> this one just leftover from development?\n\nOops, yes. Thanks for catching it, I'll drop this one.\n\n> > +test_expect_success 'shortlog bogus --group' '\n> > +\ttest_must_fail git shortlog --group=bogus HEAD 2>err &&\n> > +\tgrep \"unknown group type\" err\n> > +'\n>\n> This one is a nice inclusion. I was surprised we didn't have it already. :)\n\n:-).\n\nThanks,\nTaylor\n"},{"id":"465523","messageId":"Y1MZNISVkOFFXX9D@nand.local","threadId":"58597","inReplyTo":"Y1IsWt0ZrW+0hy1v@coredump.intra.peff.net","subject":"Re: [PATCH 4/7] shortlog: support arbitrary commit format `--group`s","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-21T22:12:04Z","receivedAt":"2022-10-21T22:12:10Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Oct 21, 2022 at 01:21:30AM -0400, Jeff King wrote:\n> On Thu, Oct 20, 2022 at 10:39:54PM -0400, Taylor Blau wrote:\n>\n> > --- 8< ---\n> > diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh\n> > index 0abe5354fc..566d581e1b 100755\n> > --- a/t/t4201-shortlog.sh\n> > +++ b/t/t4201-shortlog.sh\n> > @@ -356,6 +356,19 @@ test_expect_success 'shortlog can match multiple groups' '\n> > \ttest_cmp expect actual\n> > '\n> >\n> > +test_expect_success 'shortlog can match multiple format groups' '\n> > +\tcat >expect <<-\\EOF &&\n> > +\t 2\tUser B \n> > +\t 1\tA U Thor \n> > +\t 1\tUser A \n> > +\tEOF\n> > +\tgit shortlog -ns \\\n> > +\t\t--group=\"%(trailers:valueonly,separator=%0x00,key=some-trailer)\" \\\n> > +\t\t--group=\"%(trailers:valueonly,separator=%0x00,key=another-trailer)\" \\\n> > +\t\t-2 HEAD >actual &&\n> > +\ttest_cmp expect actual\n> > +'\n> > +\n> > test_expect_success 'set up option selection tests' '\n> > \tgit commit --allow-empty -F - <<-\\EOF\n> > \tsubject\n> > --- >8 ---\n> >\n> > The gross bit there really is the 'separator=%0x00' thing, since the\n> > \"trailers\" pretty format gives us a NL character already. I suppose that\n> > you could do something like this on top instead:\n>\n> IMHO the new test should avoid using trailers entirely, to avoid the\n> notion that they are necessary to create the duplicate situation. In a\n> normal repository, the most obvious one is just asking about both\n> authors and committers:\n>\n> git shortlog --group=format:%cn --group=format:%an\n\nYeah, that's fair. I was worried enough about whether or not this was\ngoing to cause significant test fallout, but it ended up being extremely\nstraightforward and simplified the tests nicely. Thanks!\n\nThanks,\nTaylor\n"},{"id":"465525","messageId":"cover.1666391136.git.me@ttaylorr.com","threadId":"58597","inReplyTo":"cover.1665448437.git.me@ttaylorr.com","subject":"[PATCH v3 0/7] shortlog: introduce `--group=`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-21T22:25:37Z","receivedAt":"2022-10-21T22:25:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Here is a (likely final) reroll of my series to implement arbitrary\npretty formats as shortlog `--group`'s, based on a suggestion from Jacob\nStopak.\n\nThe changes are cosmetic, based on the latest round of review from Peff.\nThere aren't any substantive changes from the last round.\n\nThanks in advance for your review.\n\nJeff King (1):\n shortlog: accept `--date`-related options\n\nTaylor Blau (6):\n shortlog: make trailer insertion a noop when appropriate\n shortlog: extract `--group` fragment for translation\n shortlog: support arbitrary commit format `--group`s\n shortlog: extract `shortlog_finish_setup()`\n shortlog: implement `--group=author` in terms of `--group=`\n shortlog: implement `--group=committer` in terms of `--group=`\n\n Documentation/git-shortlog.txt | 8 ++++\n builtin/log.c | 1 +\n builtin/shortlog.c | 87 +++++++++++++++++++++++-----------\n shortlog.h | 5 ++\n t/t4201-shortlog.sh | 39 +++++++++++++++\n 5 files changed, 113 insertions(+), 27 deletions(-)\n\nRange-diff against v2:\n1: 58baccbaa8 ! 1: add457f319 shortlog: accept `--date`-related options\n @@ Documentation/git-shortlog.txt: OPTIONS\n \tEach pretty-printed commit will be rewrapped before it is shown.\n \n +--date=::\n -+\tWith a `--group=format:`, show dates formatted\n -+\taccording to the given date string. (See the `--date` options\n -+\tin the \"COMMIT FORMATTING\" section of linkgit:git-log[1].)\n ++\tShow dates formatted according to the given date string. (See\n ++\tthe `--date` option in the \"Commit Formatting\" section of\n ++\tlinkgit:git-log[1]).\n +\n --group=::\n \tGroup commits based on ``. If no `--group` option is\n2: 7decccad7c = 2: 25cdc28215 shortlog: make trailer insertion a noop when appropriate\n3: 3665488fc9 = 3: cf84f829aa shortlog: extract `--group` fragment for translation\n4: 4a36c0ca4e ! 4: f75a952421 shortlog: support arbitrary commit format `--group`s\n @@ Commit message\n Signed-off-by: Taylor Blau \n \n ## Documentation/git-shortlog.txt ##\n +@@ Documentation/git-shortlog.txt: OPTIONS\n + --date=::\n + \tShow dates formatted according to the given date string. (See\n + \tthe `--date` option in the \"Commit Formatting\" section of\n +-\tlinkgit:git-log[1]).\n ++\tlinkgit:git-log[1]). Useful with `--group=format:`.\n + \n + --group=::\n + \tGroup commits based on ``. If no `--group` option is\n @@ Documentation/git-shortlog.txt: OPTIONS\n example, if your project uses `Reviewed-by` trailers, you might want\n to see who has been reviewing with\n @@ builtin/shortlog.c: static void insert_records_from_trailers(struct shortlog *lo\n \tunuse_commit_buffer(commit, commit_buffer);\n }\n \n ++static int shortlog_needs_dedup(const struct shortlog *log)\n ++{\n ++\treturn log->format.nr > 1 || log->trailers.nr;\n ++}\n ++\n +static void insert_records_from_format(struct shortlog *log,\n +\t\t\t\t struct strset *dups,\n +\t\t\t\t struct commit *commit,\n @@ builtin/shortlog.c: static void insert_records_from_trailers(struct shortlog *lo\n +\n +\t\tformat_commit_message(commit, item->string, &buf, ctx);\n +\n -+\t\tif (!(log->format.nr > 1 || log->trailers.nr) ||\n -+\t\t strset_add(dups, buf.buf))\n ++\t\tif (!shortlog_needs_dedup(log) || strset_add(dups, buf.buf))\n +\t\t\tinsert_one_record(log, buf.buf, oneline);\n +\t}\n +\n @@ t/t4201-shortlog.sh: test_expect_success 'shortlog --group=trailer:signed-off-by\n +\ttest_cmp expect actual\n +'\n +\n -+test_expect_success 'shortlog multiple --group=format' '\n -+\tgit shortlog -s --date=\"format:%Y\" --group=\"format:%cN (%cd)\" \\\n -+\t\tHEAD >actual &&\n -+\tcat >expect <<-\\EOF &&\n -+\t 4\tC O Mitter (2005)\n -+\t 1\tSin Nombre (2005)\n -+\tEOF\n -+\ttest_cmp expect actual\n -+'\n -+\n +test_expect_success 'shortlog bogus --group' '\n +\ttest_must_fail git shortlog --group=bogus HEAD 2>err &&\n +\tgrep \"unknown group type\" err\n @@ t/t4201-shortlog.sh: test_expect_success 'shortlog can match multiple groups' '\n '\n \n +test_expect_success 'shortlog can match multiple format groups' '\n ++\tGIT_COMMITTER_NAME=\"$GIT_AUTHOR_NAME\" \\\n ++\t\tgit commit --allow-empty -m \"identical names\" &&\n ++\ttest_tick &&\n +\tcat >expect <<-\\EOF &&\n -+\t 2\tUser B \n -+\t 1\tA U Thor \n -+\t 1\tUser A \n ++\t 2\tA U Thor\n ++\t 1\tC O Mitter\n +\tEOF\n -+\tgit shortlog -ns \\\n -+\t\t--group=\"%(trailers:valueonly,key=some-trailer)\" \\\n -+\t\t--group=\"%(trailers:valueonly,key=another-trailer)\" \\\n -+\t\t-2 HEAD >actual.raw &&\n -+\tgrep -v \"^$\" actual.raw >actual &&\n ++\tgit shortlog -ns --group=\"%cn\" --group=\"%an\" -2 HEAD >actual &&\n +\ttest_cmp expect actual\n +'\n +\n5: f0044682be ! 5: 8dd69edcf8 shortlog: implement `--group=author` in terms of `--group=`\n @@ Metadata\n Author: Taylor Blau \n \n ## Commit message ##\n - shortlog: implement `--group=author` in terms of `--group=`\n + shortlog: extract `shortlog_finish_setup()`\n \n - Instead of handling SHORTLOG_GROUP_AUTHOR separately, reimplement it as\n - a special case of the new `--group=` mode, where the author mode\n - is a shorthand for `--group='%aN <%aE>'.\n + Extract a function which finishes setting up the shortlog struct for\n + use. The caller in `make_cover_letter()` does not care about trailer\n + sorting, so it isn't strictly necessary to add a call there in this\n + patch.\n \n - Note that we still need to keep the SHORTLOG_GROUP_AUTHOR enum since it\n - has a different meaning in `read_from_stdin()`, where it is still used\n - for a different purpose.\n + But the next patch will add additional functionality to the new\n + `shortlog_finish_setup()` function, which the caller in\n + `make_cover_letter()` will care about.\n \n Signed-off-by: Taylor Blau \n \n @@ builtin/log.c: static void make_cover_letter(struct rev_info *rev, int use_separ\n \n \n ## builtin/shortlog.c ##\n -@@ builtin/shortlog.c: void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n - \t}\n - \toneline_str = oneline.len ? oneline.buf : \"\";\n - \n --\tif (log->groups & SHORTLOG_GROUP_AUTHOR) {\n --\t\tstrbuf_reset(&ident);\n --\t\tformat_commit_message(commit,\n --\t\t\t\t log->email ? \"%aN <%aE>\" : \"%aN\",\n --\t\t\t\t &ident, &ctx);\n --\t\tif (!HAS_MULTI_BITS(log->groups) ||\n --\t\t strset_add(&dups, ident.buf))\n --\t\t\tinsert_one_record(log, ident.buf, oneline_str);\n --\t}\n - \tif (log->groups & SHORTLOG_GROUP_COMMITTER) {\n - \t\tstrbuf_reset(&ident);\n - \t\tformat_commit_message(commit,\n @@ builtin/shortlog.c: void shortlog_init(struct shortlog *log)\n \tlog->format.strdup_strings = 1;\n }\n \n +void shortlog_finish_setup(struct shortlog *log)\n +{\n -+\tif (log->groups & SHORTLOG_GROUP_AUTHOR)\n -+\t\tstring_list_append(&log->format,\n -+\t\t\t\t log->email ? \"%aN <%aE>\" : \"%aN\");\n -+\n +\tstring_list_sort(&log->trailers);\n +}\n +\n-: ---------- > 6: f4f233fb48 shortlog: implement `--group=author` in terms of `--group=`\n6: 6a9e204177 = 7: 6ce06a054e shortlog: implement `--group=committer` in terms of `--group=`\n-- \n2.38.0.16.g393fd4c6db\n"},{"id":"465526","messageId":"add457f31916f81226f9104730af5a1dc58a041a.1666391136.git.me@ttaylorr.com","threadId":"58597","inReplyTo":"cover.1666391136.git.me@ttaylorr.com","subject":"[PATCH v3 1/7] shortlog: accept `--date`-related options","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-21T22:25:40Z","receivedAt":"2022-10-21T22:25:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"From: Jeff King \n\nPrepare for a future patch which will introduce arbitrary pretty formats\nvia the `--group` argument.\n\nTo allow additional customizability (for example, to support something\nlike `git shortlog -s --group='%aD' --date='format:%Y-%m' ...` (which\ngroups commits by the datestring 'YYYY-mm' according to author date), we\nmust store off the `--date` parsed from calling `parse_revision_opt()`.\n\nNote that this also affects custom output `--format` strings in `git\nshortlog`. Though this is a behavior change, this is arguably fixing a\nlong-standing bug (ie., that `--format` strings are not affected by\n`--date` specifiers as they should be).\n\nSigned-off-by: Jeff King \nSigned-off-by: Taylor Blau \n---\n Documentation/git-shortlog.txt | 5 +++++\n builtin/shortlog.c | 3 ++-\n shortlog.h | 2 ++\n t/t4201-shortlog.sh | 7 +++++++\n 4 files changed, 16 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-shortlog.txt b/Documentation/git-shortlog.txt\nindex f64e77047b..9ed9d6a9e7 100644\n--- a/Documentation/git-shortlog.txt\n+++ b/Documentation/git-shortlog.txt\n@@ -47,6 +47,11 @@ OPTIONS\n \n \tEach pretty-printed commit will be rewrapped before it is shown.\n \n+--date=::\n+\tShow dates formatted according to the given date string. (See\n+\tthe `--date` option in the \"Commit Formatting\" section of\n+\tlinkgit:git-log[1]).\n+\n --group=::\n \tGroup commits based on ``. If no `--group` option is\n \tspecified, the default is `author`. `` is one of:\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 7a1e1fe7c0..53c379a51d 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -211,7 +211,7 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n \tctx.fmt = CMIT_FMT_USERFORMAT;\n \tctx.abbrev = log->abbrev;\n \tctx.print_email_subject = 1;\n-\tctx.date_mode.type = DATE_NORMAL;\n+\tctx.date_mode = log->date_mode;\n \tctx.output_encoding = get_log_output_encoding();\n \n \tif (!log->summary) {\n@@ -407,6 +407,7 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)\n \tlog.user_format = rev.commit_format == CMIT_FMT_USERFORMAT;\n \tlog.abbrev = rev.abbrev;\n \tlog.file = rev.diffopt.file;\n+\tlog.date_mode = rev.date_mode;\n \n \tif (!log.groups)\n \t\tlog.groups = SHORTLOG_GROUP_AUTHOR;\ndiff --git a/shortlog.h b/shortlog.h\nindex 3f7e9aabca..dc388dd459 100644\n--- a/shortlog.h\n+++ b/shortlog.h\n@@ -2,6 +2,7 @@\n #define SHORTLOG_H\n \n #include \"string-list.h\"\n+#include \"date.h\"\n \n struct commit;\n \n@@ -15,6 +16,7 @@ struct shortlog {\n \tint in2;\n \tint user_format;\n \tint abbrev;\n+\tstruct date_mode date_mode;\n \n \tenum {\n \t\tSHORTLOG_GROUP_AUTHOR = (1 << 0),\ndiff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh\nindex 3095b1b2ff..7547da539d 100755\n--- a/t/t4201-shortlog.sh\n+++ b/t/t4201-shortlog.sh\n@@ -83,6 +83,13 @@ test_expect_success 'pretty format' '\n \ttest_cmp expect log.predictable\n '\n \n+test_expect_success 'pretty format (with --date)' '\n+\tsed \"s/SUBJECT/2005-04-07 OBJECT_NAME/\" expect.template >expect &&\n+\tgit shortlog --format=\"%ad %H\" --date=short HEAD >log &&\n+\tfuzz log >log.predictable &&\n+\ttest_cmp expect log.predictable\n+'\n+\n test_expect_success '--abbrev' '\n \tsed s/SUBJECT/OBJID/ expect.template >expect &&\n \tgit shortlog --format=\"%h\" --abbrev=35 HEAD >log &&\n-- \n2.38.0.16.g393fd4c6db\n\n"},{"id":"465527","messageId":"25cdc28215973b2bcefb1c14d5415e7a6f163352.1666391136.git.me@ttaylorr.com","threadId":"58597","inReplyTo":"cover.1666391136.git.me@ttaylorr.com","subject":"[PATCH v3 2/7] shortlog: make trailer insertion a noop when appropriate","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-21T22:25:43Z","receivedAt":"2022-10-21T22:25:55Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When there are no trailers to insert, it is natural that\ninsert_records_from_trailers() should return without having done any\nwork.\n\nBut instead we guard this call unnecessarily by first checking whether\n`log->groups` has the `SHORTLOG_GROUP_TRAILER` bit set.\n\nPrepare to match a similar pattern in the future where a function which\ninserts records of a certain type does no work when no specifiers\nmatching that type are given.\n\nSigned-off-by: Taylor Blau \n---\n builtin/shortlog.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 53c379a51d..18f0800c82 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -170,6 +170,9 @@ static void insert_records_from_trailers(struct shortlog *log,\n \tconst char *commit_buffer, *body;\n \tstruct strbuf ident = STRBUF_INIT;\n \n+\tif (!log->trailers.nr)\n+\t\treturn;\n+\n \t/*\n \t * Using format_commit_message(\"%B\") would be simpler here, but\n \t * this saves us copying the message.\n@@ -240,9 +243,7 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n \t\t strset_add(&dups, ident.buf))\n \t\t\tinsert_one_record(log, ident.buf, oneline_str);\n \t}\n-\tif (log->groups & SHORTLOG_GROUP_TRAILER) {\n-\t\tinsert_records_from_trailers(log, &dups, commit, &ctx, oneline_str);\n-\t}\n+\tinsert_records_from_trailers(log, &dups, commit, &ctx, oneline_str);\n \n \tstrset_clear(&dups);\n \tstrbuf_release(&ident);\n-- \n2.38.0.16.g393fd4c6db\n\n"},{"id":"465528","messageId":"f75a952421ee1ab531d9977252d6594346a94583.1666391136.git.me@ttaylorr.com","threadId":"58597","inReplyTo":"cover.1666391136.git.me@ttaylorr.com","subject":"[PATCH v3 4/7] shortlog: support arbitrary commit format `--group`s","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-21T22:25:49Z","receivedAt":"2022-10-21T22:26:04Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In addition to generating a shortlog based on committer, author, or the\nidentity in one or more specified trailers, it can be useful to generate\na shortlog based on an arbitrary commit format.\n\nThis can be used, for example, to generate a distribution of commit\nactivity over time, like so:\n\n $ git shortlog --group='%cd' --date='format:%Y-%m' -s v2.37.0..\n 117 2022-06\n 274 2022-07\n 324 2022-08\n 263 2022-09\n 7 2022-10\n\nArbitrary commit formats can be used. In fact, `git shortlog`'s default\nbehavior (to count by commit authors) can be emulated as follows:\n\n $ git shortlog --group='%aN <%aE>' ...\n\nand future patches will make the default behavior (as well as\n`--committer`, and `--group=trailer:`) special cases of the\nmore flexible `--group` option.\n\nNote also that the SHORTLOG_GROUP_FORMAT enum value is used only to\ndesignate that `--group:` is in use when in stdin mode to\ndeclare that the combination is invalid.\n\nSigned-off-by: Taylor Blau \n---\n Documentation/git-shortlog.txt | 5 ++++-\n builtin/shortlog.c | 41 +++++++++++++++++++++++++++++++++-\n shortlog.h | 2 ++\n t/t4201-shortlog.sh | 32 ++++++++++++++++++++++++++\n 4 files changed, 78 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-shortlog.txt b/Documentation/git-shortlog.txt\nindex 9ed9d6a9e7..7d0277d033 100644\n--- a/Documentation/git-shortlog.txt\n+++ b/Documentation/git-shortlog.txt\n@@ -50,7 +50,7 @@ OPTIONS\n --date=::\n \tShow dates formatted according to the given date string. (See\n \tthe `--date` option in the \"Commit Formatting\" section of\n-\tlinkgit:git-log[1]).\n+\tlinkgit:git-log[1]). Useful with `--group=format:`.\n \n --group=::\n \tGroup commits based on ``. If no `--group` option is\n@@ -64,6 +64,9 @@ OPTIONS\n example, if your project uses `Reviewed-by` trailers, you might want\n to see who has been reviewing with\n `git shortlog -ns --group=trailer:reviewed-by`.\n+ - `format:`, any string accepted by the `--format` option of\n+ 'git log'. (See the \"PRETTY FORMATS\" section of\n+ linkgit:git-log[1].)\n +\n Note that commits that do not include the trailer will not be counted.\n Likewise, commits with multiple trailers (e.g., multiple signoffs) may\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex d0645769d7..a52288acab 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -133,6 +133,8 @@ static void read_from_stdin(struct shortlog *log)\n \t\tbreak;\n \tcase SHORTLOG_GROUP_TRAILER:\n \t\tdie(_(\"using %s with stdin is not supported\"), \"--group=trailer\");\n+\tcase SHORTLOG_GROUP_FORMAT:\n+\t\tdie(_(\"using %s with stdin is not supported\"), \"--group=format\");\n \tdefault:\n \t\tBUG(\"unhandled shortlog group\");\n \t}\n@@ -203,6 +205,32 @@ static void insert_records_from_trailers(struct shortlog *log,\n \tunuse_commit_buffer(commit, commit_buffer);\n }\n \n+static int shortlog_needs_dedup(const struct shortlog *log)\n+{\n+\treturn log->format.nr > 1 || log->trailers.nr;\n+}\n+\n+static void insert_records_from_format(struct shortlog *log,\n+\t\t\t\t struct strset *dups,\n+\t\t\t\t struct commit *commit,\n+\t\t\t\t struct pretty_print_context *ctx,\n+\t\t\t\t const char *oneline)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct string_list_item *item;\n+\n+\tfor_each_string_list_item(item, &log->format) {\n+\t\tstrbuf_reset(&buf);\n+\n+\t\tformat_commit_message(commit, item->string, &buf, ctx);\n+\n+\t\tif (!shortlog_needs_dedup(log) || strset_add(dups, buf.buf))\n+\t\t\tinsert_one_record(log, buf.buf, oneline);\n+\t}\n+\n+\tstrbuf_release(&buf);\n+}\n+\n void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n {\n \tstruct strbuf ident = STRBUF_INIT;\n@@ -244,6 +272,7 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n \t\t\tinsert_one_record(log, ident.buf, oneline_str);\n \t}\n \tinsert_records_from_trailers(log, &dups, commit, &ctx, oneline_str);\n+\tinsert_records_from_format(log, &dups, commit, &ctx, oneline_str);\n \n \tstrset_clear(&dups);\n \tstrbuf_release(&ident);\n@@ -315,6 +344,7 @@ static int parse_group_option(const struct option *opt, const char *arg, int uns\n \tif (unset) {\n \t\tlog->groups = 0;\n \t\tstring_list_clear(&log->trailers, 0);\n+\t\tstring_list_clear(&log->format, 0);\n \t} else if (!strcasecmp(arg, \"author\"))\n \t\tlog->groups |= SHORTLOG_GROUP_AUTHOR;\n \telse if (!strcasecmp(arg, \"committer\"))\n@@ -322,8 +352,15 @@ static int parse_group_option(const struct option *opt, const char *arg, int uns\n \telse if (skip_prefix(arg, \"trailer:\", &field)) {\n \t\tlog->groups |= SHORTLOG_GROUP_TRAILER;\n \t\tstring_list_append(&log->trailers, field);\n-\t} else\n+\t} else if (skip_prefix(arg, \"format:\", &field)) {\n+\t\tlog->groups |= SHORTLOG_GROUP_FORMAT;\n+\t\tstring_list_append(&log->format, field);\n+\t} else if (strchr(arg, '%')) {\n+\t\tlog->groups |= SHORTLOG_GROUP_FORMAT;\n+\t\tstring_list_append(&log->format, arg);\n+\t} else {\n \t\treturn error(_(\"unknown group type: %s\"), arg);\n+\t}\n \n \treturn 0;\n }\n@@ -341,6 +378,7 @@ void shortlog_init(struct shortlog *log)\n \tlog->in2 = DEFAULT_INDENT2;\n \tlog->trailers.strdup_strings = 1;\n \tlog->trailers.cmp = strcasecmp;\n+\tlog->format.strdup_strings = 1;\n }\n \n int cmd_shortlog(int argc, const char **argv, const char *prefix)\n@@ -481,4 +519,5 @@ void shortlog_output(struct shortlog *log)\n \tlog->list.strdup_strings = 1;\n \tstring_list_clear(&log->list, 1);\n \tclear_mailmap(&log->mailmap);\n+\tstring_list_clear(&log->format, 0);\n }\ndiff --git a/shortlog.h b/shortlog.h\nindex dc388dd459..4850a8c30f 100644\n--- a/shortlog.h\n+++ b/shortlog.h\n@@ -22,8 +22,10 @@ struct shortlog {\n \t\tSHORTLOG_GROUP_AUTHOR = (1 << 0),\n \t\tSHORTLOG_GROUP_COMMITTER = (1 << 1),\n \t\tSHORTLOG_GROUP_TRAILER = (1 << 2),\n+\t\tSHORTLOG_GROUP_FORMAT = (1 << 3),\n \t} groups;\n \tstruct string_list trailers;\n+\tstruct string_list format;\n \n \tint email;\n \tstruct string_list mailmap;\ndiff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh\nindex 7547da539d..8e4effebdb 100755\n--- a/t/t4201-shortlog.sh\n+++ b/t/t4201-shortlog.sh\n@@ -244,6 +244,26 @@ test_expect_success 'shortlog --group=trailer:signed-off-by' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'shortlog --group=format' '\n+\tgit shortlog -s --date=\"format:%Y\" --group=\"format:%cN (%cd)\" \\\n+\t\tHEAD >actual &&\n+\tcat >expect <<-\\EOF &&\n+\t 4\tC O Mitter (2005)\n+\t 1\tSin Nombre (2005)\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'shortlog --group= DWIM' '\n+\tgit shortlog -s --date=\"format:%Y\" --group=\"%cN (%cd)\" HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'shortlog bogus --group' '\n+\ttest_must_fail git shortlog --group=bogus HEAD 2>err &&\n+\tgrep \"unknown group type\" err\n+'\n+\n test_expect_success 'trailer idents are split' '\n \tcat >expect <<-\\EOF &&\n \t 2\tC O Mitter\n@@ -326,6 +346,18 @@ test_expect_success 'shortlog can match multiple groups' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'shortlog can match multiple format groups' '\n+\tGIT_COMMITTER_NAME=\"$GIT_AUTHOR_NAME\" \\\n+\t\tgit commit --allow-empty -m \"identical names\" &&\n+\ttest_tick &&\n+\tcat >expect <<-\\EOF &&\n+\t 2\tA U Thor\n+\t 1\tC O Mitter\n+\tEOF\n+\tgit shortlog -ns --group=\"%cn\" --group=\"%an\" -2 HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'set up option selection tests' '\n \tgit commit --allow-empty -F - <<-\\EOF\n \tsubject\n-- \n2.38.0.16.g393fd4c6db\n\n"},{"id":"465529","messageId":"8dd69edcf8cd15c631138cb6bdc130c5da808870.1666391136.git.me@ttaylorr.com","threadId":"58597","inReplyTo":"cover.1666391136.git.me@ttaylorr.com","subject":"[PATCH v3 5/7] shortlog: extract `shortlog_finish_setup()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-21T22:25:51Z","receivedAt":"2022-10-21T22:26:11Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Extract a function which finishes setting up the shortlog struct for\nuse. The caller in `make_cover_letter()` does not care about trailer\nsorting, so it isn't strictly necessary to add a call there in this\npatch.\n\nBut the next patch will add additional functionality to the new\n`shortlog_finish_setup()` function, which the caller in\n`make_cover_letter()` will care about.\n\nSigned-off-by: Taylor Blau \n---\n builtin/log.c | 1 +\n builtin/shortlog.c | 7 ++++++-\n shortlog.h | 1 +\n 3 files changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex ee19dc5d45..b4d5420217 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1334,6 +1334,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,\n \tlog.in2 = 4;\n \tlog.file = rev->diffopt.file;\n \tlog.groups = SHORTLOG_GROUP_AUTHOR;\n+\tshortlog_finish_setup(&log);\n \tfor (i = 0; i < nr; i++)\n \t\tshortlog_add_commit(&log, list[i]);\n \ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex a52288acab..ebf2bf3d3b 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -381,6 +381,11 @@ void shortlog_init(struct shortlog *log)\n \tlog->format.strdup_strings = 1;\n }\n \n+void shortlog_finish_setup(struct shortlog *log)\n+{\n+\tstring_list_sort(&log->trailers);\n+}\n+\n int cmd_shortlog(int argc, const char **argv, const char *prefix)\n {\n \tstruct shortlog log = { STRING_LIST_INIT_NODUP };\n@@ -450,7 +455,7 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)\n \n \tif (!log.groups)\n \t\tlog.groups = SHORTLOG_GROUP_AUTHOR;\n-\tstring_list_sort(&log.trailers);\n+\tshortlog_finish_setup(&log);\n \n \t/* assume HEAD if from a tty */\n \tif (!nongit && !rev.pending.nr && isatty(0))\ndiff --git a/shortlog.h b/shortlog.h\nindex 4850a8c30f..28d04f951a 100644\n--- a/shortlog.h\n+++ b/shortlog.h\n@@ -33,6 +33,7 @@ struct shortlog {\n };\n \n void shortlog_init(struct shortlog *log);\n+void shortlog_finish_setup(struct shortlog *log);\n \n void shortlog_add_commit(struct shortlog *log, struct commit *commit);\n \n-- \n2.38.0.16.g393fd4c6db\n\n"},{"id":"465530","messageId":"f4f233fb488cb7080f112e515923672671a1c233.1666391136.git.me@ttaylorr.com","threadId":"58597","inReplyTo":"cover.1666391136.git.me@ttaylorr.com","subject":"[PATCH v3 6/7] shortlog: implement `--group=author` in terms of `--group=`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-21T22:25:54Z","receivedAt":"2022-10-21T22:26:13Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Instead of handling SHORTLOG_GROUP_AUTHOR separately, reimplement it as\na special case of the new `--group=` mode, where the author mode\nis a shorthand for `--group='%aN <%aE>'.\n\nNote that we still need to keep the SHORTLOG_GROUP_AUTHOR enum since it\nhas a different meaning in `read_from_stdin()`, where it is still used\nfor a different purpose.\n\nSigned-off-by: Taylor Blau \n---\n builtin/shortlog.c | 13 ++++---------\n 1 file changed, 4 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex ebf2bf3d3b..4b95820bca 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -253,15 +253,6 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n \t}\n \toneline_str = oneline.len ? oneline.buf : \"\";\n \n-\tif (log->groups & SHORTLOG_GROUP_AUTHOR) {\n-\t\tstrbuf_reset(&ident);\n-\t\tformat_commit_message(commit,\n-\t\t\t\t log->email ? \"%aN <%aE>\" : \"%aN\",\n-\t\t\t\t &ident, &ctx);\n-\t\tif (!HAS_MULTI_BITS(log->groups) ||\n-\t\t strset_add(&dups, ident.buf))\n-\t\t\tinsert_one_record(log, ident.buf, oneline_str);\n-\t}\n \tif (log->groups & SHORTLOG_GROUP_COMMITTER) {\n \t\tstrbuf_reset(&ident);\n \t\tformat_commit_message(commit,\n@@ -383,6 +374,10 @@ void shortlog_init(struct shortlog *log)\n \n void shortlog_finish_setup(struct shortlog *log)\n {\n+\tif (log->groups & SHORTLOG_GROUP_AUTHOR)\n+\t\tstring_list_append(&log->format,\n+\t\t\t\t log->email ? \"%aN <%aE>\" : \"%aN\");\n+\n \tstring_list_sort(&log->trailers);\n }\n \n-- \n2.38.0.16.g393fd4c6db\n\n"},{"id":"465531","messageId":"cf84f829aae4cc12732246c5b71d4fa40e0f24db.1666391136.git.me@ttaylorr.com","threadId":"58597","inReplyTo":"cover.1666391136.git.me@ttaylorr.com","subject":"[PATCH v3 3/7] shortlog: extract `--group` fragment for translation","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-21T22:25:46Z","receivedAt":"2022-10-21T22:26:29Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The subsequent commit will add another unhandled case in\n`read_from_stdin()` which will want to use the same message as with\n`--group=trailer`.\n\nExtract the \"--group=trailer\" part from this message so the same\ntranslation key can be used for both cases.\n\nSigned-off-by: Taylor Blau \n---\n builtin/shortlog.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 18f0800c82..d0645769d7 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -132,7 +132,7 @@ static void read_from_stdin(struct shortlog *log)\n \t\tmatch = committer_match;\n \t\tbreak;\n \tcase SHORTLOG_GROUP_TRAILER:\n-\t\tdie(_(\"using --group=trailer with stdin is not supported\"));\n+\t\tdie(_(\"using %s with stdin is not supported\"), \"--group=trailer\");\n \tdefault:\n \t\tBUG(\"unhandled shortlog group\");\n \t}\n-- \n2.38.0.16.g393fd4c6db\n\n"},{"id":"465532","messageId":"6ce06a054eb0cf10f59ecd1526671a40c8908edb.1666391136.git.me@ttaylorr.com","threadId":"58597","inReplyTo":"cover.1666391136.git.me@ttaylorr.com","subject":"[PATCH v3 7/7] shortlog: implement `--group=committer` in terms of `--group=`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-21T22:25:57Z","receivedAt":"2022-10-21T22:26:33Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In the same spirit as the previous commit, reimplement\n`--group=committer` as a special case of `--group=`, too.\n\nSigned-off-by: Taylor Blau \n---\n builtin/shortlog.c | 14 +++-----------\n 1 file changed, 3 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 4b95820bca..27b057940d 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -233,7 +233,6 @@ static void insert_records_from_format(struct shortlog *log,\n \n void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n {\n-\tstruct strbuf ident = STRBUF_INIT;\n \tstruct strbuf oneline = STRBUF_INIT;\n \tstruct strset dups = STRSET_INIT;\n \tstruct pretty_print_context ctx = {0};\n@@ -253,20 +252,10 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n \t}\n \toneline_str = oneline.len ? oneline.buf : \"\";\n \n-\tif (log->groups & SHORTLOG_GROUP_COMMITTER) {\n-\t\tstrbuf_reset(&ident);\n-\t\tformat_commit_message(commit,\n-\t\t\t\t log->email ? \"%cN <%cE>\" : \"%cN\",\n-\t\t\t\t &ident, &ctx);\n-\t\tif (!HAS_MULTI_BITS(log->groups) ||\n-\t\t strset_add(&dups, ident.buf))\n-\t\t\tinsert_one_record(log, ident.buf, oneline_str);\n-\t}\n \tinsert_records_from_trailers(log, &dups, commit, &ctx, oneline_str);\n \tinsert_records_from_format(log, &dups, commit, &ctx, oneline_str);\n \n \tstrset_clear(&dups);\n-\tstrbuf_release(&ident);\n \tstrbuf_release(&oneline);\n }\n \n@@ -377,6 +366,9 @@ void shortlog_finish_setup(struct shortlog *log)\n \tif (log->groups & SHORTLOG_GROUP_AUTHOR)\n \t\tstring_list_append(&log->format,\n \t\t\t\t log->email ? \"%aN <%aE>\" : \"%aN\");\n+\tif (log->groups & SHORTLOG_GROUP_COMMITTER)\n+\t\tstring_list_append(&log->format,\n+\t\t\t\t log->email ? \"%cN <%cE>\" : \"%cN\");\n \n \tstring_list_sort(&log->trailers);\n }\n-- \n2.38.0.16.g393fd4c6db\n"},{"id":"465557","messageId":"Y1M5OH/FiWNaKO6p@coredump.intra.peff.net","threadId":"58597","inReplyTo":"f75a952421ee1ab531d9977252d6594346a94583.1666391136.git.me@ttaylorr.com","subject":"Re: [PATCH v3 4/7] shortlog: support arbitrary commit format `--group`s","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-22T00:28:40Z","receivedAt":"2022-10-22T00:28:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 21, 2022 at 06:25:49PM -0400, Taylor Blau wrote:\n\n> @@ -203,6 +205,32 @@ static void insert_records_from_trailers(struct shortlog *log,\n> \tunuse_commit_buffer(commit, commit_buffer);\n> }\n> \n> +static int shortlog_needs_dedup(const struct shortlog *log)\n> +{\n> +\treturn log->format.nr > 1 || log->trailers.nr;\n> +}\n\nSo this is obviously much nicer to read. But I think there might be a\nweird logic error during the series. Since we haven't yet folded\nSHORTLOG_GROUP_AUTHOR, etc, into the \"format\" code, I think at this\npoint it still needs:\n\n HAS_MULTI_BITS(log->groups)\n\nat the beginning of the OR-chain. Otherwise \"shortlog --group=author\n--format=%an\" would fail to dedup, I think.\n\nIt's hard to care too much, since the end result of the series is\ncorrect, and you'd end up just removing that part of the line in the\nfinal patch. So I could go either way on re-rolling.\n\nSorry for not catching this in the last round.\n\n-Peff\n"},{"id":"465558","messageId":"Y1M51qzvymXHp/kI@coredump.intra.peff.net","threadId":"58597","inReplyTo":"cover.1666391136.git.me@ttaylorr.com","subject":"Re: [PATCH v3 0/7] shortlog: introduce `--group=`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-22T00:31:18Z","receivedAt":"2022-10-22T00:31:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 21, 2022 at 06:25:37PM -0400, Taylor Blau wrote:\n\n> Here is a (likely final) reroll of my series to implement arbitrary\n> pretty formats as shortlog `--group`'s, based on a suggestion from Jacob\n> Stopak.\n> \n> The changes are cosmetic, based on the latest round of review from Peff.\n> There aren't any substantive changes from the last round.\n\nI looked over the range-diff and gave another careful read of patch 4.\nIt mostly looks good, though I did find an oddity in patch 4. It's a\nreally subtle breakage that goes away by the end, so I'm not entirely\nsure it's worth dealing with.\n\nSo I'd be happy enough with this version, or if you want to do a final\nre-roll that addresses that, I'm happy to review it.\n\n-Peff\n"},{"id":"465637","messageId":"e4191ada3700a0ce6e45d3136491472cef9f4ae4.1666637725.git.me@ttaylorr.com","threadId":"58597","inReplyTo":"cover.1666637725.git.me@ttaylorr.com","subject":"[PATCH 7/7] shortlog: implement `--group=committer` in terms of `--group=`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-24T18:55:47Z","receivedAt":"2022-10-24T20:49:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In the same spirit as the previous commit, reimplement\n`--group=committer` as a special case of `--group=`, too.\n\nSigned-off-by: Taylor Blau \n---\n builtin/shortlog.c | 14 +++-----------\n 1 file changed, 3 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex f6032c6328..27a87167e1 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -233,7 +233,6 @@ static void insert_records_from_format(struct shortlog *log,\n \n void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n {\n-\tstruct strbuf ident = STRBUF_INIT;\n \tstruct strbuf oneline = STRBUF_INIT;\n \tstruct strset dups = STRSET_INIT;\n \tstruct pretty_print_context ctx = {0};\n@@ -253,20 +252,10 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n \t}\n \toneline_str = oneline.len ? oneline.buf : \"\";\n \n-\tif (log->groups & SHORTLOG_GROUP_COMMITTER) {\n-\t\tstrbuf_reset(&ident);\n-\t\tformat_commit_message(commit,\n-\t\t\t\t log->email ? \"%cN <%cE>\" : \"%cN\",\n-\t\t\t\t &ident, &ctx);\n-\t\tif (!HAS_MULTI_BITS(log->groups) ||\n-\t\t strset_add(&dups, ident.buf))\n-\t\t\tinsert_one_record(log, ident.buf, oneline_str);\n-\t}\n \tinsert_records_from_trailers(log, &dups, commit, &ctx, oneline_str);\n \tinsert_records_from_format(log, &dups, commit, &ctx, oneline_str);\n \n \tstrset_clear(&dups);\n-\tstrbuf_release(&ident);\n \tstrbuf_release(&oneline);\n }\n \n@@ -377,6 +366,9 @@ void shortlog_finish_setup(struct shortlog *log)\n \tif (log->groups & SHORTLOG_GROUP_AUTHOR)\n \t\tstring_list_append(&log->format,\n \t\t\t\t log->email ? \"%aN <%aE>\" : \"%aN\");\n+\tif (log->groups & SHORTLOG_GROUP_COMMITTER)\n+\t\tstring_list_append(&log->format,\n+\t\t\t\t log->email ? \"%cN <%cE>\" : \"%cN\");\n \n \tstring_list_sort(&log->trailers);\n }\n-- \n2.38.0.16.g393fd4c6db\n"},{"id":"465638","messageId":"3c565450654673134bd4c8584350d475b9f1b38d.1666637725.git.me@ttaylorr.com","threadId":"58597","inReplyTo":"cover.1666637725.git.me@ttaylorr.com","subject":"[PATCH 5/7] shortlog: extract `shortlog_finish_setup()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-24T18:55:41Z","receivedAt":"2022-10-24T20:49:51Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Extract a function which finishes setting up the shortlog struct for\nuse. The caller in `make_cover_letter()` does not care about trailer\nsorting, so it isn't strictly necessary to add a call there in this\npatch.\n\nBut the next patch will add additional functionality to the new\n`shortlog_finish_setup()` function, which the caller in\n`make_cover_letter()` will care about.\n\nSigned-off-by: Taylor Blau \n---\n builtin/log.c | 1 +\n builtin/shortlog.c | 7 ++++++-\n shortlog.h | 1 +\n 3 files changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex ee19dc5d45..b4d5420217 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1334,6 +1334,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,\n \tlog.in2 = 4;\n \tlog.file = rev->diffopt.file;\n \tlog.groups = SHORTLOG_GROUP_AUTHOR;\n+\tshortlog_finish_setup(&log);\n \tfor (i = 0; i < nr; i++)\n \t\tshortlog_add_commit(&log, list[i]);\n \ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex f3b237c5ff..808bae9baa 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -381,6 +381,11 @@ void shortlog_init(struct shortlog *log)\n \tlog->format.strdup_strings = 1;\n }\n \n+void shortlog_finish_setup(struct shortlog *log)\n+{\n+\tstring_list_sort(&log->trailers);\n+}\n+\n int cmd_shortlog(int argc, const char **argv, const char *prefix)\n {\n \tstruct shortlog log = { STRING_LIST_INIT_NODUP };\n@@ -450,7 +455,7 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)\n \n \tif (!log.groups)\n \t\tlog.groups = SHORTLOG_GROUP_AUTHOR;\n-\tstring_list_sort(&log.trailers);\n+\tshortlog_finish_setup(&log);\n \n \t/* assume HEAD if from a tty */\n \tif (!nongit && !rev.pending.nr && isatty(0))\ndiff --git a/shortlog.h b/shortlog.h\nindex 4850a8c30f..28d04f951a 100644\n--- a/shortlog.h\n+++ b/shortlog.h\n@@ -33,6 +33,7 @@ struct shortlog {\n };\n \n void shortlog_init(struct shortlog *log);\n+void shortlog_finish_setup(struct shortlog *log);\n \n void shortlog_add_commit(struct shortlog *log, struct commit *commit);\n \n-- \n2.38.0.16.g393fd4c6db\n\n"},{"id":"465639","messageId":"baee19cb8c0375478072ddfce06dc4708a49ef77.1666637725.git.me@ttaylorr.com","threadId":"58597","inReplyTo":"cover.1666637725.git.me@ttaylorr.com","subject":"[PATCH 4/7] shortlog: support arbitrary commit format `--group`s","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-24T18:55:39Z","receivedAt":"2022-10-24T20:49:53Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In addition to generating a shortlog based on committer, author, or the\nidentity in one or more specified trailers, it can be useful to generate\na shortlog based on an arbitrary commit format.\n\nThis can be used, for example, to generate a distribution of commit\nactivity over time, like so:\n\n $ git shortlog --group='%cd' --date='format:%Y-%m' -s v2.37.0..\n 117 2022-06\n 274 2022-07\n 324 2022-08\n 263 2022-09\n 7 2022-10\n\nArbitrary commit formats can be used. In fact, `git shortlog`'s default\nbehavior (to count by commit authors) can be emulated as follows:\n\n $ git shortlog --group='%aN <%aE>' ...\n\nand future patches will make the default behavior (as well as\n`--committer`, and `--group=trailer:`) special cases of the\nmore flexible `--group` option.\n\nNote also that the SHORTLOG_GROUP_FORMAT enum value is used only to\ndesignate that `--group:` is in use when in stdin mode to\ndeclare that the combination is invalid.\n\nSigned-off-by: Taylor Blau \n---\n Documentation/git-shortlog.txt | 5 ++++-\n builtin/shortlog.c | 41 +++++++++++++++++++++++++++++++++-\n shortlog.h | 2 ++\n t/t4201-shortlog.sh | 32 ++++++++++++++++++++++++++\n 4 files changed, 78 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-shortlog.txt b/Documentation/git-shortlog.txt\nindex 9ed9d6a9e7..7d0277d033 100644\n--- a/Documentation/git-shortlog.txt\n+++ b/Documentation/git-shortlog.txt\n@@ -50,7 +50,7 @@ OPTIONS\n --date=::\n \tShow dates formatted according to the given date string. (See\n \tthe `--date` option in the \"Commit Formatting\" section of\n-\tlinkgit:git-log[1]).\n+\tlinkgit:git-log[1]). Useful with `--group=format:`.\n \n --group=::\n \tGroup commits based on ``. If no `--group` option is\n@@ -64,6 +64,9 @@ OPTIONS\n example, if your project uses `Reviewed-by` trailers, you might want\n to see who has been reviewing with\n `git shortlog -ns --group=trailer:reviewed-by`.\n+ - `format:`, any string accepted by the `--format` option of\n+ 'git log'. (See the \"PRETTY FORMATS\" section of\n+ linkgit:git-log[1].)\n +\n Note that commits that do not include the trailer will not be counted.\n Likewise, commits with multiple trailers (e.g., multiple signoffs) may\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex d0645769d7..f3b237c5ff 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -133,6 +133,8 @@ static void read_from_stdin(struct shortlog *log)\n \t\tbreak;\n \tcase SHORTLOG_GROUP_TRAILER:\n \t\tdie(_(\"using %s with stdin is not supported\"), \"--group=trailer\");\n+\tcase SHORTLOG_GROUP_FORMAT:\n+\t\tdie(_(\"using %s with stdin is not supported\"), \"--group=format\");\n \tdefault:\n \t\tBUG(\"unhandled shortlog group\");\n \t}\n@@ -203,6 +205,32 @@ static void insert_records_from_trailers(struct shortlog *log,\n \tunuse_commit_buffer(commit, commit_buffer);\n }\n \n+static int shortlog_needs_dedup(const struct shortlog *log)\n+{\n+\treturn HAS_MULTI_BITS(log->groups) || log->format.nr > 1 || log->trailers.nr;\n+}\n+\n+static void insert_records_from_format(struct shortlog *log,\n+\t\t\t\t struct strset *dups,\n+\t\t\t\t struct commit *commit,\n+\t\t\t\t struct pretty_print_context *ctx,\n+\t\t\t\t const char *oneline)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct string_list_item *item;\n+\n+\tfor_each_string_list_item(item, &log->format) {\n+\t\tstrbuf_reset(&buf);\n+\n+\t\tformat_commit_message(commit, item->string, &buf, ctx);\n+\n+\t\tif (!shortlog_needs_dedup(log) || strset_add(dups, buf.buf))\n+\t\t\tinsert_one_record(log, buf.buf, oneline);\n+\t}\n+\n+\tstrbuf_release(&buf);\n+}\n+\n void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n {\n \tstruct strbuf ident = STRBUF_INIT;\n@@ -244,6 +272,7 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n \t\t\tinsert_one_record(log, ident.buf, oneline_str);\n \t}\n \tinsert_records_from_trailers(log, &dups, commit, &ctx, oneline_str);\n+\tinsert_records_from_format(log, &dups, commit, &ctx, oneline_str);\n \n \tstrset_clear(&dups);\n \tstrbuf_release(&ident);\n@@ -315,6 +344,7 @@ static int parse_group_option(const struct option *opt, const char *arg, int uns\n \tif (unset) {\n \t\tlog->groups = 0;\n \t\tstring_list_clear(&log->trailers, 0);\n+\t\tstring_list_clear(&log->format, 0);\n \t} else if (!strcasecmp(arg, \"author\"))\n \t\tlog->groups |= SHORTLOG_GROUP_AUTHOR;\n \telse if (!strcasecmp(arg, \"committer\"))\n@@ -322,8 +352,15 @@ static int parse_group_option(const struct option *opt, const char *arg, int uns\n \telse if (skip_prefix(arg, \"trailer:\", &field)) {\n \t\tlog->groups |= SHORTLOG_GROUP_TRAILER;\n \t\tstring_list_append(&log->trailers, field);\n-\t} else\n+\t} else if (skip_prefix(arg, \"format:\", &field)) {\n+\t\tlog->groups |= SHORTLOG_GROUP_FORMAT;\n+\t\tstring_list_append(&log->format, field);\n+\t} else if (strchr(arg, '%')) {\n+\t\tlog->groups |= SHORTLOG_GROUP_FORMAT;\n+\t\tstring_list_append(&log->format, arg);\n+\t} else {\n \t\treturn error(_(\"unknown group type: %s\"), arg);\n+\t}\n \n \treturn 0;\n }\n@@ -341,6 +378,7 @@ void shortlog_init(struct shortlog *log)\n \tlog->in2 = DEFAULT_INDENT2;\n \tlog->trailers.strdup_strings = 1;\n \tlog->trailers.cmp = strcasecmp;\n+\tlog->format.strdup_strings = 1;\n }\n \n int cmd_shortlog(int argc, const char **argv, const char *prefix)\n@@ -481,4 +519,5 @@ void shortlog_output(struct shortlog *log)\n \tlog->list.strdup_strings = 1;\n \tstring_list_clear(&log->list, 1);\n \tclear_mailmap(&log->mailmap);\n+\tstring_list_clear(&log->format, 0);\n }\ndiff --git a/shortlog.h b/shortlog.h\nindex dc388dd459..4850a8c30f 100644\n--- a/shortlog.h\n+++ b/shortlog.h\n@@ -22,8 +22,10 @@ struct shortlog {\n \t\tSHORTLOG_GROUP_AUTHOR = (1 << 0),\n \t\tSHORTLOG_GROUP_COMMITTER = (1 << 1),\n \t\tSHORTLOG_GROUP_TRAILER = (1 << 2),\n+\t\tSHORTLOG_GROUP_FORMAT = (1 << 3),\n \t} groups;\n \tstruct string_list trailers;\n+\tstruct string_list format;\n \n \tint email;\n \tstruct string_list mailmap;\ndiff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh\nindex 7547da539d..8e4effebdb 100755\n--- a/t/t4201-shortlog.sh\n+++ b/t/t4201-shortlog.sh\n@@ -244,6 +244,26 @@ test_expect_success 'shortlog --group=trailer:signed-off-by' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'shortlog --group=format' '\n+\tgit shortlog -s --date=\"format:%Y\" --group=\"format:%cN (%cd)\" \\\n+\t\tHEAD >actual &&\n+\tcat >expect <<-\\EOF &&\n+\t 4\tC O Mitter (2005)\n+\t 1\tSin Nombre (2005)\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'shortlog --group= DWIM' '\n+\tgit shortlog -s --date=\"format:%Y\" --group=\"%cN (%cd)\" HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'shortlog bogus --group' '\n+\ttest_must_fail git shortlog --group=bogus HEAD 2>err &&\n+\tgrep \"unknown group type\" err\n+'\n+\n test_expect_success 'trailer idents are split' '\n \tcat >expect <<-\\EOF &&\n \t 2\tC O Mitter\n@@ -326,6 +346,18 @@ test_expect_success 'shortlog can match multiple groups' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'shortlog can match multiple format groups' '\n+\tGIT_COMMITTER_NAME=\"$GIT_AUTHOR_NAME\" \\\n+\t\tgit commit --allow-empty -m \"identical names\" &&\n+\ttest_tick &&\n+\tcat >expect <<-\\EOF &&\n+\t 2\tA U Thor\n+\t 1\tC O Mitter\n+\tEOF\n+\tgit shortlog -ns --group=\"%cn\" --group=\"%an\" -2 HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'set up option selection tests' '\n \tgit commit --allow-empty -F - <<-\\EOF\n \tsubject\n-- \n2.38.0.16.g393fd4c6db\n\n"},{"id":"465640","messageId":"987a8484ac457cf16aa07e0411dcc4bbca83b06b.1666637725.git.me@ttaylorr.com","threadId":"58597","inReplyTo":"cover.1666637725.git.me@ttaylorr.com","subject":"[PATCH 3/7] shortlog: extract `--group` fragment for translation","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-24T18:55:36Z","receivedAt":"2022-10-24T20:49:56Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The subsequent commit will add another unhandled case in\n`read_from_stdin()` which will want to use the same message as with\n`--group=trailer`.\n\nExtract the \"--group=trailer\" part from this message so the same\ntranslation key can be used for both cases.\n\nSigned-off-by: Taylor Blau \n---\n builtin/shortlog.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 18f0800c82..d0645769d7 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -132,7 +132,7 @@ static void read_from_stdin(struct shortlog *log)\n \t\tmatch = committer_match;\n \t\tbreak;\n \tcase SHORTLOG_GROUP_TRAILER:\n-\t\tdie(_(\"using --group=trailer with stdin is not supported\"));\n+\t\tdie(_(\"using %s with stdin is not supported\"), \"--group=trailer\");\n \tdefault:\n \t\tBUG(\"unhandled shortlog group\");\n \t}\n-- \n2.38.0.16.g393fd4c6db\n\n"},{"id":"465641","messageId":"cover.1666637725.git.me@ttaylorr.com","threadId":"58597","inReplyTo":"cover.1665448437.git.me@ttaylorr.com","subject":"[PATCH 0/7] shortlog: introduce `--group=`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-24T18:55:26Z","receivedAt":"2022-10-24T20:49:58Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Here is one more very tiny reroll of my series to implement arbitrary\npretty formats as shortlog `--group`'s, based on a suggestion from Jacob\nStopak.\n\nThere are only a couple of changes from last time: one to rebase onto\nthe current tip of 'master', and another to address a bug in 4/7 (which\nwas resolved by the end of the series, but now works consistently\nthroughout the series).\n\nThis was pointed out by Peff in [1], and he indicated there:\n\n> It's hard to care too much, since the end result of the series is\n> correct, and you'd end up just removing that part of the line in\n> the final patch. So I could go either way on re-rolling.\n\n...but not re-rolling would be somewhat unsatisfying. So here is a\nreroll that I think should be good to go.\n\nThanks in advance for your review.\n\n[1]: https://lore.kernel.org/git/Y1M5OH%2FFiWNaKO6p@coredump.intra.peff.net/\n\nJeff King (1):\n shortlog: accept `--date`-related options\n\nTaylor Blau (6):\n shortlog: make trailer insertion a noop when appropriate\n shortlog: extract `--group` fragment for translation\n shortlog: support arbitrary commit format `--group`s\n shortlog: extract `shortlog_finish_setup()`\n shortlog: implement `--group=author` in terms of `--group=`\n shortlog: implement `--group=committer` in terms of `--group=`\n\n Documentation/git-shortlog.txt | 8 ++++\n builtin/log.c | 1 +\n builtin/shortlog.c | 87 +++++++++++++++++++++++-----------\n shortlog.h | 5 ++\n t/t4201-shortlog.sh | 39 +++++++++++++++\n 5 files changed, 113 insertions(+), 27 deletions(-)\n\n-- \n2.38.0.16.g393fd4c6db\n"},{"id":"465642","messageId":"38b60641c0f09a4cc6f8a9a69faafe8e3bd7635f.1666637725.git.me@ttaylorr.com","threadId":"58597","inReplyTo":"cover.1666637725.git.me@ttaylorr.com","subject":"[PATCH 1/7] shortlog: accept `--date`-related options","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-24T18:55:30Z","receivedAt":"2022-10-24T20:50:33Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"From: Jeff King \n\nPrepare for a future patch which will introduce arbitrary pretty formats\nvia the `--group` argument.\n\nTo allow additional customizability (for example, to support something\nlike `git shortlog -s --group='%aD' --date='format:%Y-%m' ...` (which\ngroups commits by the datestring 'YYYY-mm' according to author date), we\nmust store off the `--date` parsed from calling `parse_revision_opt()`.\n\nNote that this also affects custom output `--format` strings in `git\nshortlog`. Though this is a behavior change, this is arguably fixing a\nlong-standing bug (ie., that `--format` strings are not affected by\n`--date` specifiers as they should be).\n\nSigned-off-by: Jeff King \nSigned-off-by: Taylor Blau \n---\n Documentation/git-shortlog.txt | 5 +++++\n builtin/shortlog.c | 3 ++-\n shortlog.h | 2 ++\n t/t4201-shortlog.sh | 7 +++++++\n 4 files changed, 16 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-shortlog.txt b/Documentation/git-shortlog.txt\nindex f64e77047b..9ed9d6a9e7 100644\n--- a/Documentation/git-shortlog.txt\n+++ b/Documentation/git-shortlog.txt\n@@ -47,6 +47,11 @@ OPTIONS\n \n \tEach pretty-printed commit will be rewrapped before it is shown.\n \n+--date=::\n+\tShow dates formatted according to the given date string. (See\n+\tthe `--date` option in the \"Commit Formatting\" section of\n+\tlinkgit:git-log[1]).\n+\n --group=::\n \tGroup commits based on ``. If no `--group` option is\n \tspecified, the default is `author`. `` is one of:\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 7a1e1fe7c0..53c379a51d 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -211,7 +211,7 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n \tctx.fmt = CMIT_FMT_USERFORMAT;\n \tctx.abbrev = log->abbrev;\n \tctx.print_email_subject = 1;\n-\tctx.date_mode.type = DATE_NORMAL;\n+\tctx.date_mode = log->date_mode;\n \tctx.output_encoding = get_log_output_encoding();\n \n \tif (!log->summary) {\n@@ -407,6 +407,7 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)\n \tlog.user_format = rev.commit_format == CMIT_FMT_USERFORMAT;\n \tlog.abbrev = rev.abbrev;\n \tlog.file = rev.diffopt.file;\n+\tlog.date_mode = rev.date_mode;\n \n \tif (!log.groups)\n \t\tlog.groups = SHORTLOG_GROUP_AUTHOR;\ndiff --git a/shortlog.h b/shortlog.h\nindex 3f7e9aabca..dc388dd459 100644\n--- a/shortlog.h\n+++ b/shortlog.h\n@@ -2,6 +2,7 @@\n #define SHORTLOG_H\n \n #include \"string-list.h\"\n+#include \"date.h\"\n \n struct commit;\n \n@@ -15,6 +16,7 @@ struct shortlog {\n \tint in2;\n \tint user_format;\n \tint abbrev;\n+\tstruct date_mode date_mode;\n \n \tenum {\n \t\tSHORTLOG_GROUP_AUTHOR = (1 << 0),\ndiff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh\nindex 3095b1b2ff..7547da539d 100755\n--- a/t/t4201-shortlog.sh\n+++ b/t/t4201-shortlog.sh\n@@ -83,6 +83,13 @@ test_expect_success 'pretty format' '\n \ttest_cmp expect log.predictable\n '\n \n+test_expect_success 'pretty format (with --date)' '\n+\tsed \"s/SUBJECT/2005-04-07 OBJECT_NAME/\" expect.template >expect &&\n+\tgit shortlog --format=\"%ad %H\" --date=short HEAD >log &&\n+\tfuzz log >log.predictable &&\n+\ttest_cmp expect log.predictable\n+'\n+\n test_expect_success '--abbrev' '\n \tsed s/SUBJECT/OBJID/ expect.template >expect &&\n \tgit shortlog --format=\"%h\" --abbrev=35 HEAD >log &&\n-- \n2.38.0.16.g393fd4c6db\n\n"},{"id":"465643","messageId":"6a373815bf5cd92da762c7cf5e40c0cecaa33be5.1666637725.git.me@ttaylorr.com","threadId":"58597","inReplyTo":"cover.1666637725.git.me@ttaylorr.com","subject":"[PATCH 6/7] shortlog: implement `--group=author` in terms of `--group=`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-24T18:55:44Z","receivedAt":"2022-10-24T20:59:38Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Instead of handling SHORTLOG_GROUP_AUTHOR separately, reimplement it as\na special case of the new `--group=` mode, where the author mode\nis a shorthand for `--group='%aN <%aE>'.\n\nNote that we still need to keep the SHORTLOG_GROUP_AUTHOR enum since it\nhas a different meaning in `read_from_stdin()`, where it is still used\nfor a different purpose.\n\nSigned-off-by: Taylor Blau \n---\n builtin/shortlog.c | 13 ++++---------\n 1 file changed, 4 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 808bae9baa..f6032c6328 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -253,15 +253,6 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n \t}\n \toneline_str = oneline.len ? oneline.buf : \"\";\n \n-\tif (log->groups & SHORTLOG_GROUP_AUTHOR) {\n-\t\tstrbuf_reset(&ident);\n-\t\tformat_commit_message(commit,\n-\t\t\t\t log->email ? \"%aN <%aE>\" : \"%aN\",\n-\t\t\t\t &ident, &ctx);\n-\t\tif (!HAS_MULTI_BITS(log->groups) ||\n-\t\t strset_add(&dups, ident.buf))\n-\t\t\tinsert_one_record(log, ident.buf, oneline_str);\n-\t}\n \tif (log->groups & SHORTLOG_GROUP_COMMITTER) {\n \t\tstrbuf_reset(&ident);\n \t\tformat_commit_message(commit,\n@@ -383,6 +374,10 @@ void shortlog_init(struct shortlog *log)\n \n void shortlog_finish_setup(struct shortlog *log)\n {\n+\tif (log->groups & SHORTLOG_GROUP_AUTHOR)\n+\t\tstring_list_append(&log->format,\n+\t\t\t\t log->email ? \"%aN <%aE>\" : \"%aN\");\n+\n \tstring_list_sort(&log->trailers);\n }\n \n-- \n2.38.0.16.g393fd4c6db\n\n"},{"id":"465647","messageId":"2153c6c6649c54588c24db1ace647719ad0431f3.1666637725.git.me@ttaylorr.com","threadId":"58597","inReplyTo":"cover.1666637725.git.me@ttaylorr.com","subject":"[PATCH 2/7] shortlog: make trailer insertion a noop when appropriate","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-24T18:55:33Z","receivedAt":"2022-10-24T21:50:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When there are no trailers to insert, it is natural that\ninsert_records_from_trailers() should return without having done any\nwork.\n\nBut instead we guard this call unnecessarily by first checking whether\n`log->groups` has the `SHORTLOG_GROUP_TRAILER` bit set.\n\nPrepare to match a similar pattern in the future where a function which\ninserts records of a certain type does no work when no specifiers\nmatching that type are given.\n\nSigned-off-by: Taylor Blau \n---\n builtin/shortlog.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 53c379a51d..18f0800c82 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -170,6 +170,9 @@ static void insert_records_from_trailers(struct shortlog *log,\n \tconst char *commit_buffer, *body;\n \tstruct strbuf ident = STRBUF_INIT;\n \n+\tif (!log->trailers.nr)\n+\t\treturn;\n+\n \t/*\n \t * Using format_commit_message(\"%B\") would be simpler here, but\n \t * this saves us copying the message.\n@@ -240,9 +243,7 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n \t\t strset_add(&dups, ident.buf))\n \t\t\tinsert_one_record(log, ident.buf, oneline_str);\n \t}\n-\tif (log->groups & SHORTLOG_GROUP_TRAILER) {\n-\t\tinsert_records_from_trailers(log, &dups, commit, &ctx, oneline_str);\n-\t}\n+\tinsert_records_from_trailers(log, &dups, commit, &ctx, oneline_str);\n \n \tstrset_clear(&dups);\n \tstrbuf_release(&ident);\n-- \n2.38.0.16.g393fd4c6db\n\n"},{"id":"465663","messageId":"xmqqwn8orihk.fsf@gitster.g","threadId":"58597","inReplyTo":"cover.1666637725.git.me@ttaylorr.com","subject":"Re: [PATCH 0/7] shortlog: introduce `--group=`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-24T21:59:03Z","receivedAt":"2022-10-24T23:40:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> Here is one more very tiny reroll of my series to implement arbitrary\n> pretty formats as shortlog `--group`'s, based on a suggestion from Jacob\n> Stopak.\n>\n> There are only a couple of changes from last time: one to rebase onto\n> the current tip of 'master', and another to address a bug in 4/7 (which\n> was resolved by the end of the series, but now works consistently\n> throughout the series).\n>\n> This was pointed out by Peff in [1], and he indicated there:\n>\n>> It's hard to care too much, since the end result of the series is\n>> correct, and you'd end up just removing that part of the line in\n>> the final patch. So I could go either way on re-rolling.\n\nOne \"huh?\" is that the final patch in this round hasn't changed from\nthe last round, so the check for HAS_MULTI_BITS(log->groups) remains\nin the final result.\n\nIOW, after replacing the previous series with this round,\n\n $ git diff @{1}\n diff --git c/builtin/shortlog.c w/builtin/shortlog.c\n index 27b057940d..27a87167e1 100644\n --- c/builtin/shortlog.c\n +++ w/builtin/shortlog.c\n @@ -207,7 +207,7 @@ static void insert_records_from_trailers(struct shortlog *log,\n\n static int shortlog_needs_dedup(const struct shortlog *log)\n {\n -\treturn log->format.nr > 1 || log->trailers.nr;\n +\treturn HAS_MULTI_BITS(log->groups) || log->format.nr > 1 || log->trailers.nr;\n }\n\n static void insert_records_from_format(struct shortlog *log,\n\nis the difference in the end result.\n\n"},{"id":"465673","messageId":"Y1cjmJY8YUv7Pv21@coredump.intra.peff.net","threadId":"58597","inReplyTo":"cover.1666637725.git.me@ttaylorr.com","subject":"Re: [PATCH 0/7] shortlog: introduce `--group=`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-24T23:45:28Z","receivedAt":"2022-10-25T00:57:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 24, 2022 at 02:55:26PM -0400, Taylor Blau wrote:\n\n> There are only a couple of changes from last time: one to rebase onto\n> the current tip of 'master', and another to address a bug in 4/7 (which\n> was resolved by the end of the series, but now works consistently\n> throughout the series).\n> \n> This was pointed out by Peff in [1], and he indicated there:\n> \n> > It's hard to care too much, since the end result of the series is\n> > correct, and you'd end up just removing that part of the line in\n> > the final patch. So I could go either way on re-rolling.\n> \n> ...but not re-rolling would be somewhat unsatisfying. So here is a\n> reroll that I think should be good to go.\n\nOK, good. It offended my sensibilities, too, but I didn't want to force\nextra work on you because of them. I'm glad you agreed. ;)\n\nThis isn't mark v4, but the range-diff from v3 is:\n\n1: 31487229e6 = 1: e79db4b987 shortlog: accept `--date`-related options\n2: be2c6c0f4c = 2: 81e91f7049 shortlog: make trailer insertion a noop when appropriate\n3: bd38ac66f2 = 3: cde611e3b0 shortlog: extract `--group` fragment for translation\n4: 277ffe92ce ! 4: 020a2175cb shortlog: support arbitrary commit format `--group`s\n @@ builtin/shortlog.c: static void insert_records_from_trailers(struct shortlog *lo\n \n +static int shortlog_needs_dedup(const struct shortlog *log)\n +{\n -+\treturn log->format.nr > 1 || log->trailers.nr;\n ++\treturn HAS_MULTI_BITS(log->groups) || log->format.nr > 1 || log->trailers.nr;\n +}\n +\n +static void insert_records_from_format(struct shortlog *log,\n5: 6c02a2daab = 5: 13a1f1b8c8 shortlog: extract `shortlog_finish_setup()`\n6: 969bdaae39 = 6: 8af036c9f8 shortlog: implement `--group=author` in terms of `--group=`\n7: bad7a2bc68 = 7: 09d138353b shortlog: implement `--group=committer` in terms of `--group=`\n\n\nI had assumed you would drop the HAS_MULTI_BITS() part from this\nfunction after patch 7, as we know that everything is either a format or\na trailer. But actually, I like keeping it. It future-proofs us against\nadding more non-format group types (though I have trouble imagining what\nthose would be), and it's not like it's expensive to keep around.\n\nSo this iteration looks good to me. Thanks!\n\n-Peff\n"}]}