{"thread":{"id":"58487","subject":"[RFC PATCH] shortlog: add group-by options for year and month","startedAt":"2022-09-22T06:19:44Z","lastAt":"2022-10-11T01:00:21Z","messageCount":16,"participants":["Jacob Stopak","Martin Ågren","Junio C Hamano","Jeff King","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"463423","messageId":"20220922061824.16988-1-jacob@initialcommit.io","threadId":"58487","inReplyTo":null,"subject":"[RFC PATCH] shortlog: add group-by options for year and month","fromName":"Jacob Stopak","fromEmail":"jacob@initialcommit.io","sentAt":"2022-09-22T06:18:24Z","receivedAt":"2022-09-22T06:19:44Z","isPatch":true,"sender":{"key":"jacob@initialcommit.io","avatar":"https://avatars.githubusercontent.com/u/49353917?v=4"},"body":"It can be useful to group commits using time-based attributes in\naddition to author/committer. Currently, this can somewhat be\naccomplished using \"git shortlog --since=x --until=y\", however\nall commits will be displayed in that single time chunk, grouped\nby author.\n\nHowever, much more versatile time groupings can be achieved by adding\noptions to group by year or month. This can lead to more interesting\ncommit summaries breaking down the commits an author made during each\nyear or month, using something like:\n\n\"git shortlog --group=month --group=author --author=Stopak\"\n\nShorthand flags added for month grouping are \"-m\" or \"--month\", and for\nyear groupings are \"-y\" or \"--year\".\n\nNote that if grouped _only_ by month or year (with no \"--group=author\"\noption), shortlog will group commits made by ALL authors during each time\nperiod.\n\nIt turns out that combining these with existing flags \"-s\" or \"-n\" or\nboth leads to various useful grouped commit summaries which can be\nordered chronologically (default) or based on number of commits during\neach time period (when the \"-n\" flag is added).\n\nFurthermore, these new groupings can be combined with \"--since\" or\n\"--until\" to generate yearly or monthly groupings within those\noverarching time slices.\n\nSince the year and/or month part used for grouping comes directly\nfrom each commit, and commits are already being parsed by the existing\nshortlog logic, I don't think adding these new flags should have a\nnoticeable performance impact. The only added time should be to format\nthe month or year into the shortlog messages. The ordering was already\nhandled by the existing shortlog output logic.\n\nSigned-off-by: Jacob Stopak <jacob@initialcommit.io>\n---\nConsidering this is my first (rfc) patch that actually touches code,\nI figured I'd mention a few things I'm not totally sure about.\n\nFirst is my usage of \"strbuf\" and associated functions, especially my\nguess at an initial buffer size of 100 bytes.\n\nSecond is my direct usage of functions localtime_r(), strftime(), and\nsnprintf(). I searched around a bit for non-static api functions to use\ninstead, but maybe I missed the right ones to use.\n\nOh and third, for documentation, I updated \"git-shortlog.txt\", but\nwasn't able to test \"git help shortlog\" locally and see the updates. Is\nthere a way to make that work locally or did I miss a step somewhere?\n\nOne last note - I added some curly braces for consistency on an if/else\nblock related to some code that I touched.\n\n-Jack\n\n Documentation/git-shortlog.txt | 10 +++++\n builtin/shortlog.c             | 82 +++++++++++++++++++++++++++++-----\n shortlog.h                     |  2 +\n t/t4201-shortlog.sh            | 42 +++++++++++++++++\n 4 files changed, 126 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git-shortlog.txt b/Documentation/git-shortlog.txt\nindex f64e77047b..ab68b287d8 100644\n--- a/Documentation/git-shortlog.txt\n+++ b/Documentation/git-shortlog.txt\n@@ -54,6 +54,8 @@ OPTIONS\n --\n  - `author`, commits are grouped by author\n  - `committer`, commits are grouped by committer (the same as `-c`)\n+ - `month`, commits are grouped by month (the same as `-m`)\n+ - `year`, commits are grouped by year (the same as `-y`)\n  - `trailer:<field>`, the `<field>` is interpreted as a case-insensitive\n    commit message trailer (see linkgit:git-interpret-trailers[1]). For\n    example, if your project uses `Reviewed-by` trailers, you might want\n@@ -80,6 +82,14 @@ counts both authors and co-authors.\n --committer::\n \tThis is an alias for `--group=committer`.\n \n+-m::\n+--month::\n+\tThis is an alias for `--group=month`.\n+\n+-y::\n+--year::\n+\tThis is an alias for `--group=year`.\n+\n -w[<width>[,<indent1>[,<indent2>]]]::\n \tLinewrap the output by wrapping each line at `width`.  The first\n \tline of each entry is indented by `indent1` spaces, and the second\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 7a1e1fe7c0..99592f1c59 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -133,6 +133,10 @@ static void read_from_stdin(struct shortlog *log)\n \t\tbreak;\n \tcase SHORTLOG_GROUP_TRAILER:\n \t\tdie(_(\"using --group=trailer with stdin is not supported\"));\n+\tcase SHORTLOG_GROUP_YEAR:\n+\t\tdie(_(\"using --group=year with stdin is not supported\"));\n+\tcase SHORTLOG_GROUP_MONTH:\n+\t\tdie(_(\"using --group=month with stdin is not supported\"));\n \tdefault:\n \t\tBUG(\"unhandled shortlog group\");\n \t}\n@@ -200,10 +204,29 @@ static void insert_records_from_trailers(struct shortlog *log,\n \tunuse_commit_buffer(commit, commit_buffer);\n }\n \n+static void format_commit_date(struct commit *commit, struct strbuf *sb,\n+\t\t\t       char *format, struct shortlog *log)\n+{\n+\ttime_t t = (time_t) commit->date;\n+\tstruct tm commit_date;\n+\tlocaltime_r(&t, &commit_date);\n+\n+\tif (log->groups & SHORTLOG_GROUP_MONTH) {\n+        \tstrftime(sb->buf, strbuf_avail(sb), \"%Y/%m\", &commit_date);\n+\t\tsnprintf(sb->buf+7, strbuf_avail(sb), \"%s\", format);\n+\t} else if (log->groups & SHORTLOG_GROUP_YEAR) {\n+        \tstrftime(sb->buf, strbuf_avail(sb), \"%Y\", &commit_date);\n+\t\tsnprintf(sb->buf+4, strbuf_avail(sb), \"%s\", format);\n+\t}\n+}\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 strbuf buffer;\n+\tstrbuf_init(&buffer, 100);\n+\n \tstruct strset dups = STRSET_INIT;\n \tstruct pretty_print_context ctx = {0};\n \tconst char *oneline_str;\n@@ -222,20 +245,47 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n \t}\n \toneline_str = oneline.len ? oneline.buf : \"<none>\";\n \n+\tif ((log->groups & SHORTLOG_GROUP_MONTH) && (log->groups & SHORTLOG_GROUP_YEAR))\n+\t\tlog->groups ^= SHORTLOG_GROUP_YEAR;\n+\n+\tif (((log->groups & SHORTLOG_GROUP_MONTH) || (log->groups & SHORTLOG_GROUP_YEAR))\n+\t      && !HAS_MULTI_BITS(log->groups)) {\n+\t\tformat_commit_date(commit, &buffer, \"\", log);\n+\t\tformat_commit_message(commit, \n+                                      buffer.buf, \n+                                      &ident, &ctx);\n+\n+\t\tif (strset_add(&dups, ident.buf))\n+\t\t\tinsert_one_record(log, ident.buf, oneline_str);\n+\t}\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 ((log->groups & SHORTLOG_GROUP_MONTH) || (log->groups & SHORTLOG_GROUP_YEAR)) {\n+\t\t\tformat_commit_date(commit, &buffer, log->email ? \" %aN <%aE>\" : \" %aN\", log);\n+\t\t\tformat_commit_message(commit,\n+\t\t\t\t\t      buffer.buf,\n+\t\t\t\t\t      &ident, &ctx);\n+\t\t} else {\n+\t\t\tformat_commit_message(commit,\n+\t\t\t\t\t      log->email ? \"%aN <%aE>\" : \"%aN\",\n+\t\t\t\t\t      &ident, &ctx);\n+\t\t}\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-\t\t\t\t      log->email ? \"%cN <%cE>\" : \"%cN\",\n-\t\t\t\t      &ident, &ctx);\n+\t\tif ((log->groups & SHORTLOG_GROUP_MONTH) || (log->groups & SHORTLOG_GROUP_YEAR)) {\n+\t\t\tformat_commit_date(commit, &buffer, log->email ? \" %cN <%cE>\" : \" %cN\", log);\n+\t\t\tformat_commit_message(commit,\n+\t\t\t\t\t      buffer.buf,\n+\t\t\t\t\t      &ident, &ctx);\n+\t\t} else {\n+\t\t\tformat_commit_message(commit,\n+\t\t\t\t      \t      log->email ? \"%cN <%cE>\" : \"%cN\",\n+\t\t\t\t      \t      &ident, &ctx);\n+\t\t}\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@@ -247,6 +297,7 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n \tstrset_clear(&dups);\n \tstrbuf_release(&ident);\n \tstrbuf_release(&oneline);\n+\tstrbuf_release(&buffer);\n }\n \n static void get_from_rev(struct rev_info *rev, struct shortlog *log)\n@@ -314,15 +365,20 @@ 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} else if (!strcasecmp(arg, \"author\"))\n+\t} else if (!strcasecmp(arg, \"author\")) {\n \t\tlog->groups |= SHORTLOG_GROUP_AUTHOR;\n-\telse if (!strcasecmp(arg, \"committer\"))\n+\t} else if (!strcasecmp(arg, \"committer\")) {\n \t\tlog->groups |= SHORTLOG_GROUP_COMMITTER;\n-\telse if (skip_prefix(arg, \"trailer:\", &field)) {\n+\t} else 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 (!strcasecmp(arg, \"month\")) {\n+\t\tlog->groups |= SHORTLOG_GROUP_MONTH;\n+\t} else if (!strcasecmp(arg, \"year\")) {\n+\t\tlog->groups |= SHORTLOG_GROUP_YEAR;\n+\t} else {\n \t\treturn error(_(\"unknown group type: %s\"), arg);\n+\t}\n \n \treturn 0;\n }\n@@ -363,6 +419,12 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)\n \t\t\t&parse_wrap_args),\n \t\tOPT_CALLBACK(0, \"group\", &log, N_(\"field\"),\n \t\t\tN_(\"group by field\"), parse_group_option),\n+\t\tOPT_BIT('m', \"month\", &log.groups,\n+                         N_(\"group by month rather than author\"),\n+                         SHORTLOG_GROUP_MONTH),\n+\t\tOPT_BIT('y', \"year\", &log.groups,\n+                          N_(\"group by year rather than author\"),\n+                          SHORTLOG_GROUP_YEAR),\n \t\tOPT_END(),\n \t};\n \ndiff --git a/shortlog.h b/shortlog.h\nindex 3f7e9aabca..45b5efb6dc 100644\n--- a/shortlog.h\n+++ b/shortlog.h\n@@ -20,6 +20,8 @@ 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_MONTH = (1 << 3),\n+\t\tSHORTLOG_GROUP_YEAR = (1 << 4),\n \t} groups;\n \tstruct string_list trailers;\n \ndiff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh\nindex 3095b1b2ff..981f45f732 100755\n--- a/t/t4201-shortlog.sh\n+++ b/t/t4201-shortlog.sh\n@@ -359,4 +359,46 @@ test_expect_success 'stdin with multiple groups reports error' '\n \ttest_must_fail git shortlog --group=author --group=committer <log\n '\n \n+test_expect_success '--group=year groups output by year' '\n+\tgit commit --allow-empty -m \"git shortlog --group=year test\" &&\n+\tcat >expect <<-\\EOF &&\n+\t     1\t2005\n+\tEOF\n+\tgit shortlog -ns \\\n+\t\t--group=year \\\n+\t\t-1 HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'stdin with --group=year reports error' '\n+\ttest_must_fail git shortlog --group=year\n+'\n+\n+test_expect_success '--group=month groups output by month' '\n+\tgit commit --allow-empty -m \"git shortlog --group=month test\" &&\n+\tcat >expect <<-\\EOF &&\n+\t     1\t2005/04\n+\tEOF\n+\tgit shortlog -ns \\\n+\t\t--group=month \\\n+\t\t-1 HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'stdin with --group=month reports error' '\n+\ttest_must_fail git shortlog --group=month\n+'\n+\n+test_expect_success '--group=month and --group=year defaults to month' '\n+\tgit commit --allow-empty -m \"git shortlog --group=month --group=year test\" &&\n+\tcat >expect <<-\\EOF &&\n+\t     1\t2005/04\n+\tEOF\n+\tgit shortlog -ns \\\n+\t\t--group=month \\\n+\t\t--group=year \\\n+\t\t-1 HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n\nbase-commit: dda7228a83e2e9ff584bf6adbf55910565b41e14\n-- \n2.37.3\n\n"},{"id":"463451","messageId":"CAN0heSr8HFR1y+aZUjFaeY3y-9yn+nfyDrkxQh82punLnSonGg@mail.gmail.com","threadId":"58487","inReplyTo":"20220922061824.16988-1-jacob@initialcommit.io","subject":"Re: [RFC PATCH] shortlog: add group-by options for year and month","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2022-09-22T15:46:30Z","receivedAt":"2022-09-22T15:46:58Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Hi Jacob,\n\nOn Thu, 22 Sept 2022 at 09:48, Jacob Stopak <jacob@initialcommit.io> wrote:\n>\n> Oh and third, for documentation, I updated \"git-shortlog.txt\", but\n> wasn't able to test \"git help shortlog\" locally and see the updates. Is\n> there a way to make that work locally or did I miss a step somewhere?\n\nI can think of two steps. You need to build the documentation (`make\ndoc`). Look for \"make doc\" in INSTALL for some variants and\ndependencies. Then you need to convince `git help ...` to pick up your\nbuilt docs. I would actually skip using/testing `git help` and just go\nstraight for the rendered page using, e.g, something like\n\n  cd Documentation\n  make ./git-shortlog.{1,html}\n  man ./git-shortlog.1\n  a-browser ./git-shortlog.html\n\nYour docs render fine for me. Thanks for `backticking` for monospace.\n\n> +-m::\n> +--month::\n> +       This is an alias for `--group=month`.\n> +\n> +-y::\n> +--year::\n> +       This is an alias for `--group=year`.\n> +\n\nCommit 2338c450b (\"shortlog: add grouping option\", 2020-09-27)\nintroduced `--group` and redefined `-c`  and `--committer` to alias to\nthat new thing. You could simply add a `--group` variant without\nactually adding `--year` and `--month`. One of the nice things about\n`--group` is that we can potentially have many groupings without having\nto carry correspondingly many `--option`s.\n\nIn particular, it might be wise to wait with implementing `-y` and `-m`\nuntil we know that your new feature turns out to be so hugely successful\nthat people start craving `-m` as a short form for `--group=month`. ;-)\n\nSee commit 47beb37bc6 (\"shortlog: match commit trailers with --group\",\n2020-09-27) for some prior art of not adding a new `--option` for a new\nway of grouping.\n\n>         struct strbuf ident = STRBUF_INIT;\n>         struct strbuf oneline = STRBUF_INIT;\n> +       struct strbuf buffer;\n> +       strbuf_init(&buffer, 100);\n> +\n>         struct strset dups = STRSET_INIT;\n>         struct pretty_print_context ctx = {0};\n\nThis trips up `-Werror=declaration-after-statement`. If you build with\n`DEVELOPER=Yes`, you should see the same thing.\n\nI played a little with this functionality and it's quite cute. I can\neasily imagine going even more granular with this (`--group=week`?), but\nthat can wait for some other time. :-)\n\nBTW, I got this when `git am`-ing your patch:\n\n  Applying: shortlog: add group-by options for year and month\n  .git/rebase-apply/patch:82: space before tab in indent.\n                  strftime(sb->buf, strbuf_avail(sb), \"%Y/%m\", &commit_date);\n  .git/rebase-apply/patch:85: space before tab in indent.\n                  strftime(sb->buf, strbuf_avail(sb), \"%Y\", &commit_date);\n  .git/rebase-apply/patch:110: trailing whitespace.\n                  format_commit_message(commit,\n  .git/rebase-apply/patch:111: trailing whitespace, indent with spaces.\n                                        buffer.buf,\n  .git/rebase-apply/patch:112: indent with spaces.\n                                        &ident, &ctx);\n  warning: squelched 6 whitespace errors\n  warning: 11 lines add whitespace errors.\n\nMartin\n"},{"id":"463484","messageId":"20220922232536.40807-1-jacob@initialcommit.io","threadId":"58487","inReplyTo":"20220922061824.16988-1-jacob@initialcommit.io","subject":"[RFC PATCH v2] shortlog: add group-by options for year and month","fromName":"Jacob Stopak","fromEmail":"jacob@initialcommit.io","sentAt":"2022-09-22T23:25:36Z","receivedAt":"2022-09-22T23:26:30Z","isPatch":true,"sender":{"key":"jacob@initialcommit.io","avatar":"https://avatars.githubusercontent.com/u/49353917?v=4"},"body":"It can be useful to group commits using time-based attributes in\naddition to author/committer. Currently, this can somewhat be\ndone using \"git shortlog --since=x --until=y\", however all commits\nwill be displayed in that single time chunk, grouped by author.\n\nHowever, much more versatile time groupings can be achieved by adding\noptions to group by year or month. This can lead to more interesting\ncommit summaries breaking down the commits an author made during each\nyear or month, using something like:\n\n\"git shortlog --group=month --group=author --author=Stopak\"\n\nShorthand flags added for month grouping are \"-m\" or \"--month\", and for\nyear groupings are \"-y\" or \"--year\".\n\nIf grouped _only_ by month or year (ie no \"--group=author\" option),\nshortlog will group commits made by ALL authors during each time period.\n\nIt turns out that combining these with existing flags \"-s\" or \"-n\" or\nboth leads to various useful grouped commit summaries which can be\nordered chronologically (default) or based on number of commits during\neach time period (when the \"-n\" flag is added), as in:\n\n\"git shortlog -nsy\"\n\nFurthermore, these new groupings can be combined with \"--since\" or\n\"--until\" to generate yearly or monthly groupings within those\noverarching time slices.\n\nNote that (at least for now) using the year and month groupings\nare not supported when shortlog is reading from stdin, since the\ndefault log output for date might require some assumptions to reformat\nduring parsing, mainly regarding the first 2 digits of the year.\n\nSince the year and/or month part used for grouping comes directly\nfrom each commit, and commits are already being parsed by the existing\nshortlog logic, I don't think adding these new flags should have a\nnoticeable performance impact. The only added time should be to format\nthe month or year into the shortlog messages. The ordering was already\nhandled by the existing shortlog output logic.\n\nSigned-off-by: Jacob Stopak <jacob@initialcommit.io>\n---\nThx a lot for the feedback! Added v2 patch here.\n\n> I would actually skip using/testing `git help` and just go\n> straight for the rendered page using, e.g, something like ...\n\nCool I was able to use your steps to get the local man page working.\n\n> One of the nice things about `--group` is that we can potentially\n> have many groupings without having to carry correspondingly many `--option`s.\n>\n> In particular, it might be wise to wait with implementing `-y` and `-m`\n> until we know that your new feature turns out to be so hugely successful\n> that people start craving `-m` as a short form for `--group=month`. ;-)\n\nHaha yes I might have gotten a bit excited with the shorthand flags, BUT\nlet me give my pitch for why I think it makes sense to keep them :)\n\nAdding these new time-based groups makes it more likely that folks will\nspecify multiple groups at once, (like pairing with \"--group=author\")\nwhich makes it painful to write \"--group=...\" over & over again,\nespecially when running/editing the command many times.\n\nAlso, having shorthands -y and -m pairs very nicely when using other\nshorthand flags -s, -n, and -c, for stuff like \"git shortlog -nsy\".\n\nTo justify why the new year/month groups should have shorthand flags\nwhile \"--group=trailer:value\" does not, I'd say that the fact that\ntrailer requires a custom value would make the shorthand version clunky,\nand it wouldn't fit in well with other shorthand options like -n or -s.\nSince year/month have no custom value, the flags make a bit more sense\nand would match up with how \"-c, --committer\" currently works.\n\nSo overall the shorthand flags can be more convenient in several ways,\nwhich increases the odds folks will use the new feature often :D. Thoughts?\n\n> This trips up `-Werror=declaration-after-statement`. If you build with\n> `DEVELOPER=Yes`, you should see the same thing.\n\nHm, I tried setting the DEVELOPER flag at the top of Makefile, and also\npassing as argument to \"make DEVELOPER=Yes git-shortlog\", but didn't see\nthose warnings - I'm on a mac fwiw. Anyway I moved the statement after\nthe declarations so I think it should be fixed.\n\n> I can easily imagine going even more granular with this\n> (`--group=week`?), but that can wait for some other time. :-)\n\nI'd love to add a week option in the future if this gets accepted...\n\n> BTW, I got this when `git am`-ing your patch: ...\n\nFixed those pesky whitespace issues!\n\n Documentation/git-shortlog.txt | 10 ++++\n builtin/shortlog.c             | 83 ++++++++++++++++++++++++++++++----\n shortlog.h                     |  2 +\n t/t4201-shortlog.sh            | 42 +++++++++++++++++\n 4 files changed, 127 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git-shortlog.txt b/Documentation/git-shortlog.txt\nindex f64e77047b..ab68b287d8 100644\n--- a/Documentation/git-shortlog.txt\n+++ b/Documentation/git-shortlog.txt\n@@ -54,6 +54,8 @@ OPTIONS\n --\n  - `author`, commits are grouped by author\n  - `committer`, commits are grouped by committer (the same as `-c`)\n+ - `month`, commits are grouped by month (the same as `-m`)\n+ - `year`, commits are grouped by year (the same as `-y`)\n  - `trailer:<field>`, the `<field>` is interpreted as a case-insensitive\n    commit message trailer (see linkgit:git-interpret-trailers[1]). For\n    example, if your project uses `Reviewed-by` trailers, you might want\n@@ -80,6 +82,14 @@ counts both authors and co-authors.\n --committer::\n \tThis is an alias for `--group=committer`.\n \n+-m::\n+--month::\n+\tThis is an alias for `--group=month`.\n+\n+-y::\n+--year::\n+\tThis is an alias for `--group=year`.\n+\n -w[<width>[,<indent1>[,<indent2>]]]::\n \tLinewrap the output by wrapping each line at `width`.  The first\n \tline of each entry is indented by `indent1` spaces, and the second\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 7a1e1fe7c0..1beba9b91c 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -133,6 +133,10 @@ static void read_from_stdin(struct shortlog *log)\n \t\tbreak;\n \tcase SHORTLOG_GROUP_TRAILER:\n \t\tdie(_(\"using --group=trailer with stdin is not supported\"));\n+\tcase SHORTLOG_GROUP_YEAR:\n+\t\tdie(_(\"using --group=year with stdin is not supported\"));\n+\tcase SHORTLOG_GROUP_MONTH:\n+\t\tdie(_(\"using --group=month with stdin is not supported\"));\n \tdefault:\n \t\tBUG(\"unhandled shortlog group\");\n \t}\n@@ -200,10 +204,28 @@ static void insert_records_from_trailers(struct shortlog *log,\n \tunuse_commit_buffer(commit, commit_buffer);\n }\n \n+static void format_commit_date(struct commit *commit, struct strbuf *sb,\n+\t\t\t       char *format, struct shortlog *log)\n+{\n+\ttime_t t = (time_t) commit->date;\n+\tstruct tm commit_date;\n+\tlocaltime_r(&t, &commit_date);\n+\n+\tif (log->groups & SHORTLOG_GROUP_MONTH) {\n+\t\tstrftime(sb->buf, strbuf_avail(sb), \"%Y/%m\", &commit_date);\n+\t\tsnprintf(sb->buf+7, strbuf_avail(sb), \"%s\", format);\n+\t} else if (log->groups & SHORTLOG_GROUP_YEAR) {\n+\t\tstrftime(sb->buf, strbuf_avail(sb), \"%Y\", &commit_date);\n+\t\tsnprintf(sb->buf+4, strbuf_avail(sb), \"%s\", format);\n+\t}\n+}\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 strbuf buffer;\n+\n \tstruct strset dups = STRSET_INIT;\n \tstruct pretty_print_context ctx = {0};\n \tconst char *oneline_str;\n@@ -214,6 +236,8 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n \tctx.date_mode.type = DATE_NORMAL;\n \tctx.output_encoding = get_log_output_encoding();\n \n+\tstrbuf_init(&buffer, 100);\n+\n \tif (!log->summary) {\n \t\tif (log->user_format)\n \t\t\tpretty_print_commit(&ctx, commit, &oneline);\n@@ -222,20 +246,47 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n \t}\n \toneline_str = oneline.len ? oneline.buf : \"<none>\";\n \n-\tif (log->groups & SHORTLOG_GROUP_AUTHOR) {\n-\t\tstrbuf_reset(&ident);\n+\tif ((log->groups & SHORTLOG_GROUP_MONTH) && (log->groups & SHORTLOG_GROUP_YEAR))\n+\t\tlog->groups ^= SHORTLOG_GROUP_YEAR;\n+\n+\tif (((log->groups & SHORTLOG_GROUP_MONTH) || (log->groups & SHORTLOG_GROUP_YEAR))\n+\t      && !HAS_MULTI_BITS(log->groups)) {\n+\t\tformat_commit_date(commit, &buffer, \"\", log);\n \t\tformat_commit_message(commit,\n-\t\t\t\t      log->email ? \"%aN <%aE>\" : \"%aN\",\n+\t\t\t\t      buffer.buf,\n \t\t\t\t      &ident, &ctx);\n+\n+\t\tif (strset_add(&dups, ident.buf))\n+\t\t\tinsert_one_record(log, ident.buf, oneline_str);\n+\t}\n+\tif (log->groups & SHORTLOG_GROUP_AUTHOR) {\n+\t\tstrbuf_reset(&ident);\n+\t\tif ((log->groups & SHORTLOG_GROUP_MONTH) || (log->groups & SHORTLOG_GROUP_YEAR)) {\n+\t\t\tformat_commit_date(commit, &buffer, log->email ? \" %aN <%aE>\" : \" %aN\", log);\n+\t\t\tformat_commit_message(commit,\n+\t\t\t\t\t      buffer.buf,\n+\t\t\t\t\t      &ident, &ctx);\n+\t\t} else {\n+\t\t\tformat_commit_message(commit,\n+\t\t\t\t\t      log->email ? \"%aN <%aE>\" : \"%aN\",\n+\t\t\t\t\t      &ident, &ctx);\n+\t\t}\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-\t\t\t\t      log->email ? \"%cN <%cE>\" : \"%cN\",\n-\t\t\t\t      &ident, &ctx);\n+\t\tif ((log->groups & SHORTLOG_GROUP_MONTH) || (log->groups & SHORTLOG_GROUP_YEAR)) {\n+\t\t\tformat_commit_date(commit, &buffer, log->email ? \" %cN <%cE>\" : \" %cN\", log);\n+\t\t\tformat_commit_message(commit,\n+\t\t\t\t\t      buffer.buf,\n+\t\t\t\t\t      &ident, &ctx);\n+\t\t} else {\n+\t\t\tformat_commit_message(commit,\n+\t\t\t\t\t      log->email ? \"%cN <%cE>\" : \"%cN\",\n+\t\t\t\t\t      &ident, &ctx);\n+\t\t}\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@@ -247,6 +298,7 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n \tstrset_clear(&dups);\n \tstrbuf_release(&ident);\n \tstrbuf_release(&oneline);\n+\tstrbuf_release(&buffer);\n }\n \n static void get_from_rev(struct rev_info *rev, struct shortlog *log)\n@@ -314,15 +366,20 @@ 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} else if (!strcasecmp(arg, \"author\"))\n+\t} else if (!strcasecmp(arg, \"author\")) {\n \t\tlog->groups |= SHORTLOG_GROUP_AUTHOR;\n-\telse if (!strcasecmp(arg, \"committer\"))\n+\t} else if (!strcasecmp(arg, \"committer\")) {\n \t\tlog->groups |= SHORTLOG_GROUP_COMMITTER;\n-\telse if (skip_prefix(arg, \"trailer:\", &field)) {\n+\t} else 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 (!strcasecmp(arg, \"month\")) {\n+\t\tlog->groups |= SHORTLOG_GROUP_MONTH;\n+\t} else if (!strcasecmp(arg, \"year\")) {\n+\t\tlog->groups |= SHORTLOG_GROUP_YEAR;\n+\t} else {\n \t\treturn error(_(\"unknown group type: %s\"), arg);\n+\t}\n \n \treturn 0;\n }\n@@ -363,6 +420,12 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)\n \t\t\t&parse_wrap_args),\n \t\tOPT_CALLBACK(0, \"group\", &log, N_(\"field\"),\n \t\t\tN_(\"group by field\"), parse_group_option),\n+\t\tOPT_BIT('m', \"month\", &log.groups,\n+\t\t\tN_(\"group by month rather than author\"),\n+\t\t\tSHORTLOG_GROUP_MONTH),\n+\t\tOPT_BIT('y', \"year\", &log.groups,\n+\t\t\tN_(\"group by year rather than author\"),\n+\t\t\tSHORTLOG_GROUP_YEAR),\n \t\tOPT_END(),\n \t};\n \ndiff --git a/shortlog.h b/shortlog.h\nindex 3f7e9aabca..45b5efb6dc 100644\n--- a/shortlog.h\n+++ b/shortlog.h\n@@ -20,6 +20,8 @@ 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_MONTH = (1 << 3),\n+\t\tSHORTLOG_GROUP_YEAR = (1 << 4),\n \t} groups;\n \tstruct string_list trailers;\n \ndiff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh\nindex 3095b1b2ff..981f45f732 100755\n--- a/t/t4201-shortlog.sh\n+++ b/t/t4201-shortlog.sh\n@@ -359,4 +359,46 @@ test_expect_success 'stdin with multiple groups reports error' '\n \ttest_must_fail git shortlog --group=author --group=committer <log\n '\n \n+test_expect_success '--group=year groups output by year' '\n+\tgit commit --allow-empty -m \"git shortlog --group=year test\" &&\n+\tcat >expect <<-\\EOF &&\n+\t     1\t2005\n+\tEOF\n+\tgit shortlog -ns \\\n+\t\t--group=year \\\n+\t\t-1 HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'stdin with --group=year reports error' '\n+\ttest_must_fail git shortlog --group=year\n+'\n+\n+test_expect_success '--group=month groups output by month' '\n+\tgit commit --allow-empty -m \"git shortlog --group=month test\" &&\n+\tcat >expect <<-\\EOF &&\n+\t     1\t2005/04\n+\tEOF\n+\tgit shortlog -ns \\\n+\t\t--group=month \\\n+\t\t-1 HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'stdin with --group=month reports error' '\n+\ttest_must_fail git shortlog --group=month\n+'\n+\n+test_expect_success '--group=month and --group=year defaults to month' '\n+\tgit commit --allow-empty -m \"git shortlog --group=month --group=year test\" &&\n+\tcat >expect <<-\\EOF &&\n+\t     1\t2005/04\n+\tEOF\n+\tgit shortlog -ns \\\n+\t\t--group=month \\\n+\t\t--group=year \\\n+\t\t-1 HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.37.3\n\n"},{"id":"463517","messageId":"xmqqillevzeh.fsf@gitster.g","threadId":"58487","inReplyTo":"20220922232536.40807-1-jacob@initialcommit.io","subject":"Re: [RFC PATCH v2] shortlog: add group-by options for year and month","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-23T16:17:10Z","receivedAt":"2022-09-23T16:19:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Stopak <jacob@initialcommit.io> writes:\n\n> To justify why the new year/month groups should have shorthand flags\n> while \"--group=trailer:value\" does not, I'd say that the fact that\n> trailer requires a custom value would make the shorthand version clunky,\n> and it wouldn't fit in well with other shorthand options like -n or -s.\n> Since year/month have no custom value, the flags make a bit more sense\n> and would match up with how \"-c, --committer\" currently works.\n\nIt is an explanation why it is easier to implement and design --year\netc., and does not justify why adding --year etc. is a good idea at\nall.  Let's not add the \"aliases\" in the same patch.\n\n>   - `author`, commits are grouped by author\n>   - `committer`, commits are grouped by committer (the same as `-c`)\n> + - `month`, commits are grouped by month (the same as `-m`)\n> + - `year`, commits are grouped by year (the same as `-y`)\n\nIt is unclear what timestamp is used, how a \"month\" is defined, etc.\nAs \"git shortlog --since=2.years\" cuts off based on the committer\ntimestamp, I would expect that the committer timestamps are used for\nthis grouping as well?  If I make a commit on the first day of the\nmonth in my timezone, but that instant happens to be still on the\nlast day of the previous month in your timezone, which month would\nyour invocation of \"git shortlog --group=month\" would the commit be\nattributed?  My month, or your month?\n\nDoes it make sense to even say \"group by month and year\"?  I expect\nthat it would mean the same thing as \"group by month\", and if that\nis the case, the command probably should error out or at least warn\nif both are given.  An alternative interpretation could be, when\ntold to \"group by month\", group the commits made in September 2022\ninto the same group as the group for commits made in September in\nall other years, but I do not know how useful it would be.\n\nNot a suggestion to use a different implementation or add a new\nfeature on top of this --group-by-time-range idea, but I wonder if\nit is a more flexible and generalizeable approach to say \"formulate\nthis value given by the --format=<format> string, apply this regular\nexpression match, and group by the subexpression value\".  E.g.\n\n    git shortlog \\\n\t--group-by-value=\"%cI\" \\\n\t--group-by-regexp=\"^(\\d{4}-\\d{2})\"\n\nwould \"formulate the 'committer date in ISO format' value, and apply\nthe 'grab leading 4 digits followed by a dash followed by 2 digits'\nregexp, and group by the matched part\".\n\nThat's a better way to implement \"group by month\" internally, and\nallow more flexibility.  If a project is well disciplined and its\ncommit titles follow the \"<area>: <description>\" convention, you\nprobably could do\n\n    git shortlog --no-merges \\\n\t--group-by-value=\"%s\" \\\n\t--group-by-regexp=\"^([^:]+):\"\n\nand group by <area> each commit touches.  Of course, existing\n--committer and --author can also be internally reimplemented using\nthe same mechanism.\n\n> @@ -80,6 +82,14 @@ counts both authors and co-authors.\n>  --committer::\n>  \tThis is an alias for `--group=committer`.\n>  \n> +-m::\n> +--month::\n> +\tThis is an alias for `--group=month`.\n> +\n> +-y::\n> +--year::\n> +\tThis is an alias for `--group=year`.\n> +\n\nLet's not add this in the same patch.  I am fairly negative on\nadding more, outside \"--group\".  Besides, we do not have a good\nanswer to those who want to group by week.  -w is already taken.\n"},{"id":"463553","messageId":"Yy4i4I15RjOy+sLm.jacob@initialcommit.io","threadId":"58487","inReplyTo":"xmqqillevzeh.fsf@gitster.g","subject":"Re: [RFC PATCH v2] shortlog: add group-by options for year and month","fromName":"Jacob Stopak","fromEmail":"jacob@initialcommit.io","sentAt":"2022-09-23T21:19:28Z","receivedAt":"2022-09-23T21:19:39Z","isPatch":true,"sender":{"key":"jacob@initialcommit.io","avatar":"https://avatars.githubusercontent.com/u/49353917?v=4"},"body":"On Fri, Sep 23, 2022 at 09:17:10AM -0700, Junio C Hamano wrote:\n\n> It is unclear what timestamp is used, how a \"month\" is defined, etc.\n> As \"git shortlog --since=2.years\" cuts off based on the committer\n> timestamp, I would expect that the committer timestamps are used for\n> this grouping as well?\n\nIt uses the \"commit->date\" member from the commit struct in commit.h,\nwhich I assumed was the committer timestamp, but I'll confirm that's\nwhat actually gets populated in there since I don't see a separate\nmember for the author timestamp...\n\n> If I make a commit on the first day of the\n> month in my timezone, but that instant happens to be still on the\n> last day of the previous month in your timezone, which month would\n> your invocation of \"git shortlog --group=month\" would the commit be\n> attributed?  My month, or your month?\n\nI need to look into how Git typically handles these timezone differences\nand will try to apply similar behavior for these time-based groupings.\n\n> Does it make sense to even say \"group by month and year\"?  I expect\n> that it would mean the same thing as \"group by month\", and if that\n> is the case, the command probably should error out or at least warn\n> if both are given.\n\nYes, \"group by month and year\" and \"group by month\" means the same\nthing the way I implemented. If both groups are supplied, it will\njust ignore the year group and group by month like you mentioned,\nby flipping off the YEAR bit as follows:\n\nif ((log->groups & SHORTLOG_GROUP_MONTH) &&\n    (log->groups & SHORTLOG_GROUP_YEAR))\n\tlog->groups ^= SHORTLOG_GROUP_YEAR;\n\nI can add a warning message to make this more clear to the user.\n\n> An alternative interpretation could be, when\n> told to \"group by month\", group the commits made in September 2022\n> into the same group as the group for commits made in September in\n> all other years, but I do not know how useful it would be.\n\nIn data analytics terms this is usually referred to as \"month of year\",\nand personally I see it less useful in Git's shortlog context because I\nenvision more folks would find output useful for single or consecutive\ntime periods. However, adding a \"month of year\" grouping could be useful\nto answer questions like \"what periods throughout the year are contributors\nmost active?\". If we decide to add a \"month of year\" grouping option as\nwell, it would be trivial to include.\n \n> Not a suggestion to use a different implementation or add a new\n> feature on top of this --group-by-time-range idea, but I wonder if\n> it is a more flexible and generalizeable approach to say \"formulate\n> this value given by the --format=<format> string, apply this regular\n> expression match, and group by the subexpression value\".  E.g.\n> \n>     git shortlog \\\n> \t--group-by-value=\"%cI\" \\\n> \t--group-by-regexp=\"^(\\d{4}-\\d{2})\"\n> \n> would \"formulate the 'committer date in ISO format' value, and apply\n> the 'grab leading 4 digits followed by a dash followed by 2 digits'\n> regexp, and group by the matched part\".\n> \n> That's a better way to implement \"group by month\" internally, and\n> allow more flexibility.  If a project is well disciplined and its\n> commit titles follow the \"<area>: <description>\" convention, you\n> probably could do\n> \n>     git shortlog --no-merges \\\n> \t--group-by-value=\"%s\" \\\n> \t--group-by-regexp=\"^([^:]+):\"\n> \n> and group by <area> each commit touches.  Of course, existing\n> --committer and --author can also be internally reimplemented using\n> the same mechanism.\n\nAt first look this sounds very flexible and appealing, and I would be \ninterested in exploring a refactor to this in the future. I think the\nrub is that supplying custom patterns wouldn't necessarily stack up\nneatly into good groups, which could lead to confusing results for the\nuser in terms of both grouping and sorting. But like you mentioned\nit could be really cool if used judiciously for a consistent history\nlike Git's. And the generalized re-implementation of the current\nshortlog groups would be a nice bonus.\n\n> > @@ -80,6 +82,14 @@ counts both authors and co-authors.\n> >  --committer::\n> >  \tThis is an alias for `--group=committer`.\n> >  \n> > +-m::\n> > +--month::\n> > +\tThis is an alias for `--group=month`.\n> > +\n> > +-y::\n> > +--year::\n> > +\tThis is an alias for `--group=year`.\n> > +\n> \n> Let's not add this in the same patch.  I am fairly negative on\n> adding more, outside \"--group\".  Besides, we do not have a good\n> answer to those who want to group by week.  -w is already taken.\n\nNo worries - I'll remove the shorthand flags for v3.\n"},{"id":"463556","messageId":"Yy4sIAHdvp6yRql+@coredump.intra.peff.net","threadId":"58487","inReplyTo":"xmqqillevzeh.fsf@gitster.g","subject":"Re: [RFC PATCH v2] shortlog: add group-by options for year and month","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-23T21:58:56Z","receivedAt":"2022-09-23T21:59:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 23, 2022 at 09:17:10AM -0700, Junio C Hamano wrote:\n\n> Not a suggestion to use a different implementation or add a new\n> feature on top of this --group-by-time-range idea, but I wonder if\n> it is a more flexible and generalizeable approach to say \"formulate\n> this value given by the --format=<format> string, apply this regular\n> expression match, and group by the subexpression value\".  E.g.\n> \n>     git shortlog \\\n> \t--group-by-value=\"%cI\" \\\n> \t--group-by-regexp=\"^(\\d{4}-\\d{2})\"\n\nHeh, I was about to make the exact same suggestion. The existing\n\"--group=author\" could really just be \"--group='%an <%ae>'\" (or variants\ndepending on the \"-e\" flag).\n\nI don't think you even really need the regexp. If we respect --date,\nthen you should be able to ask for --date=format:%Y-%m. Unfortunately\nthere's no way to specify the format as part of the placeholder. The\nfor-each-ref formatter understands this, like:\n\n  %(authordate:format:%Y-%m)\n\nI wouldn't be opposed to teaching the git-log formatter something\nsimilar.\n\n> That's a better way to implement \"group by month\" internally, and\n> allow more flexibility.  If a project is well disciplined and its\n> commit titles follow the \"<area>: <description>\" convention, you\n> probably could do\n> \n>     git shortlog --no-merges \\\n> \t--group-by-value=\"%s\" \\\n> \t--group-by-regexp=\"^([^:]+):\"\n> \n> and group by <area> each commit touches.  Of course, existing\n> --committer and --author can also be internally reimplemented using\n> the same mechanism.\n\nThis example makes the regex feature more interesting, because it's not\nsomething we'd likely have a unique placeholder for (or maybe we\nshould?).\n\nBut there's something else interesting going on in Jack's patch, which\nis that he's not just introducing the date-sorting, but also that it's\nused in conjunction with other sorting. So really the intended use is\nsomething like:\n\n  git shortlog --group:author --group:%Y-%m\n\nI think we'd want to allow the general form to be a series of groupings.\nIn the output from his patch it looks like:\n\n  2022-09 Jeff King\n     some commit message\n     another commit message\n\nI.e., the groups are collapsed into a single string, and unique strings\nbecome their own groups (and are sorted in the usual way).\n\nIf you give up the regex thing, then that naturally falls out as\n(imagining we learn about authordate as a placeholder):\n\n  git shortlog --group='%(authordate:format=%Y-%n) %an'\n\nwithout having to implement multiple groupings as a specific feature\n(which is both more code, but also has user-facing confusion about when\n--group overrides versus appends). That also skips the question of which\n--group-by-regex applies to which --group-by-value.\n\nI do agree the regex thing is more flexible, but if we can't come up\nwith a case more compelling than subsystem matching, I'd just as soon\nadd %(subject:subsystem) or similar. :)\n\n-Peff\n"},{"id":"463558","messageId":"xmqqpmflsq2p.fsf@gitster.g","threadId":"58487","inReplyTo":"Yy4sIAHdvp6yRql+@coredump.intra.peff.net","subject":"Re: [RFC PATCH v2] shortlog: add group-by options for year and month","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-23T22:06:54Z","receivedAt":"2022-09-23T22:07:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> If you give up the regex thing, then that naturally falls out as\n> (imagining we learn about authordate as a placeholder):\n>\n>   git shortlog --group='%(authordate:format=%Y-%n) %an'\n>\n> without having to implement multiple groupings as a specific feature\n> (which is both more code, but also has user-facing confusion about when\n> --group overrides versus appends). That also skips the question of which\n> --group-by-regex applies to which --group-by-value.\n>\n> I do agree the regex thing is more flexible, but if we can't come up\n> with a case more compelling than subsystem matching, I'd just as soon\n> add %(subject:subsystem) or similar. :)\n\n;-)  I like that as a general direction.\n"},{"id":"463564","messageId":"Yy6JxQz4ZxghQnG1.jacob@initialcommit.io","threadId":"58487","inReplyTo":"Yy4sIAHdvp6yRql+@coredump.intra.peff.net","subject":"Re: [RFC PATCH v2] shortlog: add group-by options for year and month","fromName":"Jacob Stopak","fromEmail":"jacob@initialcommit.io","sentAt":"2022-09-24T04:38:29Z","receivedAt":"2022-09-24T04:38:38Z","isPatch":true,"sender":{"key":"jacob@initialcommit.io","avatar":"https://avatars.githubusercontent.com/u/49353917?v=4"},"body":"On Fri, Sep 23, 2022 at 05:58:56PM -0400, Jeff King wrote:\n> I don't think you even really need the regexp. If we respect --date,\n> then you should be able to ask for --date=format:%Y-%m.\n\nHmm I tried passing in --date=format:... to my patched shortlog command\nalong with setting some date placeholder like \"... %cd ...\" in the code,\nbut it's not picking up on the format. Do you know how the date format\ncan be wedged into the format_commit_message(...) \"format\" argument?\n\n> Unfortunately there's no way to specify the format as part of the\n> placeholder. The for-each-ref formatter understands this, like:\n> \n>   %(authordate:format:%Y-%m)\n>\n> I wouldn't be opposed to teaching the git-log formatter something\n> similar.\n\nOh that would solve my problem... Would it be a hefty effort to teach\nthis to the git-log formatter?\n\n> But there's something else interesting going on in Jack's patch, which\n> is that he's not just introducing the date-sorting, but also that it's\n> used in conjunction with other sorting. So really the intended use is\n> something like:\n> \n>   git shortlog --group:author --group:%Y-%m\n\nYes I sort of stumbled on this and realized that this way I wouldn't have\nto touch the actual sorting or grouping functionality at all, which was\nalready working properly. I just needed to reformat the shortlog message to\ninclude the year and/or month in a way that kept things consistent.\n\n> I think we'd want to allow the general form to be a series of groupings.\n> In the output from his patch it looks like:\n> \n>   2022-09 Jeff King\n>      some commit message\n>      another commit message\n> \n> I.e., the groups are collapsed into a single string, and unique strings\n> become their own groups (and are sorted in the usual way).\n> \n> If you give up the regex thing, then that naturally falls out as\n> (imagining we learn about authordate as a placeholder):\n> \n>   git shortlog --group='%(authordate:format=%Y-%n) %an'\n> \n> without having to implement multiple groupings as a specific feature\n> (which is both more code, but also has user-facing confusion about when\n> --group overrides versus appends). That also skips the question of which\n> --group-by-regex applies to which --group-by-value.\n> \n> I do agree the regex thing is more flexible, but if we can't come up\n> with a case more compelling than subsystem matching, I'd just as soon\n> add %(subject:subsystem) or similar. :)\n> \n> -Peff\n\nI like this idea too. Since it requires a larger re-implementation,\nmaybe I can pursue this going forward. I assume if we did this we would\nkeep the existing group options like \"--group=author\" as shortcuts, and\nrefactor them behind the scenes to use the new method. If so it may be\nuseful to add my originally suggested options of \"--group=year\" and\n\"--group=month\" as well for convenient default time-based groupings.\n\nHow do you feel about me submitting a v3 patch of my initial suggested\nimplementation of new group options for year and month? Then going forward\nI can work on generalizing the grouping feature the way Peff suggested.\n\n-Jack\n"},{"id":"464264","messageId":"Yz36eFeGyQ3ha1pw@nand.local","threadId":"58487","inReplyTo":"Yy4sIAHdvp6yRql+@coredump.intra.peff.net","subject":"Re: [RFC PATCH v2] shortlog: add group-by options for year and month","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-05T21:43:20Z","receivedAt":"2022-10-05T21:43:29Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Sep 23, 2022 at 05:58:56PM -0400, Jeff King wrote:\n> On Fri, Sep 23, 2022 at 09:17:10AM -0700, Junio C Hamano wrote:\n>\n> > Not a suggestion to use a different implementation or add a new\n> > feature on top of this --group-by-time-range idea, but I wonder if\n> > it is a more flexible and generalizeable approach to say \"formulate\n> > this value given by the --format=<format> string, apply this regular\n> > expression match, and group by the subexpression value\".  E.g.\n> >\n> >     git shortlog \\\n> > \t--group-by-value=\"%cI\" \\\n> > \t--group-by-regexp=\"^(\\d{4}-\\d{2})\"\n>\n> Heh, I was about to make the exact same suggestion. The existing\n> \"--group=author\" could really just be \"--group='%an <%ae>'\" (or variants\n> depending on the \"-e\" flag).\n\nThis caught my attention, so I wanted to see how hard it would be to\nimplement. It actually is quite straightforward, and gets us most of the\nway to being able to get the same functionality as in Jacob's patch\n(minus being able to do the for-each-ref-style sub-selectors, like\n`%(authordate:format=%Y-%m)`).\n\nHere's the patch:\n\n--- >8 ---\n\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 7a1e1fe7c0..68880e8867 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -200,6 +200,29 @@ static void insert_records_from_trailers(struct shortlog *log,\n \tunuse_commit_buffer(commit, commit_buffer);\n }\n\n+static void insert_record_from_pretty(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 ident = STRBUF_INIT;\n+\tsize_t i;\n+\n+\tfor (i = 0; i < log->pretty.nr; i++) {\n+\t\tif (i)\n+\t\t\tstrbuf_addch(&ident, ' ');\n+\n+\t\tformat_commit_message(commit, log->pretty.items[i].string,\n+\t\t\t\t      &ident, ctx);\n+\t}\n+\n+\tif (strset_add(dups, ident.buf))\n+\t\tinsert_one_record(log, ident.buf, oneline);\n+\n+\tstrbuf_release(&ident);\n+}\n+\n void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n {\n \tstruct strbuf ident = STRBUF_INIT;\n@@ -243,6 +266,8 @@ 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+\tif (log->groups & SHORTLOG_GROUP_PRETTY)\n+\t\tinsert_record_from_pretty(log, &dups, commit, &ctx, oneline_str);\n\n \tstrset_clear(&dups);\n \tstrbuf_release(&ident);\n@@ -321,8 +346,10 @@ 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\treturn error(_(\"unknown group type: %s\"), arg);\n+\t} else {\n+\t\tlog->groups |= SHORTLOG_GROUP_PRETTY;\n+\t\tstring_list_append(&log->pretty, arg);\n+\t}\n\n \treturn 0;\n }\n@@ -340,6 +367,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->pretty.strdup_strings = 1;\n }\n\n int cmd_shortlog(int argc, const char **argv, const char *prefix)\ndiff --git a/shortlog.h b/shortlog.h\nindex 3f7e9aabca..d7caecb76f 100644\n--- a/shortlog.h\n+++ b/shortlog.h\n@@ -20,8 +20,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_PRETTY = (1 << 3),\n \t} groups;\n \tstruct string_list trailers;\n+\tstruct string_list pretty;\n\n \tint email;\n \tstruct string_list mailmap;\n\n--- 8< ---\n\n> I don't think you even really need the regexp. If we respect --date,\n> then you should be able to ask for --date=format:%Y-%m. Unfortunately\n> there's no way to specify the format as part of the placeholder. The\n> for-each-ref formatter understands this, like:\n>\n>   %(authordate:format:%Y-%m)\n>\n> I wouldn't be opposed to teaching the git-log formatter something\n> similar.\n\nYeah, I think having a similar mechanism there would be useful, and\ncertainly a prerequisite to being able to achieve what Jacob has done\nhere with the more general approach.\n\nI think you could also do some cleanup on top, like replacing the\nSHORTLOG_GROUP_AUTHOR mode with adding either \"%aN <%aE>\" (or \"%aN\",\nwithout --email) as an entry in the `pretty` string_list.\n\nThanks,\nTaylor\n"},{"id":"464265","messageId":"Yz4BvCDIMWptDMKC@coredump.intra.peff.net","threadId":"58487","inReplyTo":"Yy6JxQz4ZxghQnG1.jacob@initialcommit.io","subject":"Re: [RFC PATCH v2] shortlog: add group-by options for year and month","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-05T22:14:20Z","receivedAt":"2022-10-05T22:15:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 23, 2022 at 09:38:29PM -0700, Jacob Stopak wrote:\n\n> Hmm I tried passing in --date=format:... to my patched shortlog command\n> along with setting some date placeholder like \"... %cd ...\" in the code,\n> but it's not picking up on the format. Do you know how the date format\n> can be wedged into the format_commit_message(...) \"format\" argument?\n\nIt comes to the format code via the pretty_print_context. And we pick up\nthe --date command via setup_revisions(), where it ends up in\nrev_info.date_mode.\n\nIn a normal git-log, I think that data gets shuffled across by\nshow_log(). But shortlog has its own traversal.\n\nI think something like this:\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..ef3a3dbc65 100644\n--- a/shortlog.h\n+++ b/shortlog.h\n@@ -15,6 +15,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\nis enough. At least it allows:\n\n  git shortlog --format='%ad %s' --date=format:%Y-%m\n\nto work as you'd expect (but of course that's just the output for each\ncommit that we show, not the actual grouping).\n\n> > Unfortunately there's no way to specify the format as part of the\n> > placeholder. The for-each-ref formatter understands this, like:\n> > \n> >   %(authordate:format:%Y-%m)\n> >\n> > I wouldn't be opposed to teaching the git-log formatter something\n> > similar.\n> \n> Oh that would solve my problem... Would it be a hefty effort to teach\n> this to the git-log formatter?\n\nProbably not a huge amount of work. But it puts us in a weird in-between\nsituation where we support _one_ of the more advanced ref-filter\nplaceholders, but not the others. And of course no code is shared.\n\nThat might be OK, as long as the syntax and semantics are identical to\nwhat ref-filter can do. Then in the long run, if we eventually merge the\ntwo implementations, there's no compatibility problem.\n\nThat said, I think it may just be easier to respect --date, as above.\nIt's not quite as flexible, but it's probably flexible enough.\n\n-Peff\n"},{"id":"464269","messageId":"Yz4EsT8noIoygk9b@coredump.intra.peff.net","threadId":"58487","inReplyTo":"Yz36eFeGyQ3ha1pw@nand.local","subject":"Re: [RFC PATCH v2] shortlog: add group-by options for year and month","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-05T22:26:57Z","receivedAt":"2022-10-05T22:27:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 05, 2022 at 05:43:20PM -0400, Taylor Blau wrote:\n\n> > Heh, I was about to make the exact same suggestion. The existing\n> > \"--group=author\" could really just be \"--group='%an <%ae>'\" (or variants\n> > depending on the \"-e\" flag).\n> \n> This caught my attention, so I wanted to see how hard it would be to\n> implement. It actually is quite straightforward, and gets us most of the\n> way to being able to get the same functionality as in Jacob's patch\n> (minus being able to do the for-each-ref-style sub-selectors, like\n> `%(authordate:format=%Y-%m)`).\n\nYeah, your patch is about what I'd expect.\n\nThe date thing I think can be done with --date; I just sent a sketch in\nanother part of the thread.\n\n> +static void insert_record_from_pretty(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 ident = STRBUF_INIT;\n> +\tsize_t i;\n> +\n> +\tfor (i = 0; i < log->pretty.nr; i++) {\n> +\t\tif (i)\n> +\t\t\tstrbuf_addch(&ident, ' ');\n> +\n> +\t\tformat_commit_message(commit, log->pretty.items[i].string,\n> +\t\t\t\t      &ident, ctx);\n> +\t}\n\nSo here you're allowing multiple pretty options. But really, once we\nallow the user an arbitrary format, is there any reason for them to do:\n\n  git shortlog --group=%an --group=%ad\n\nversus just:\n\n  git shortlog --group='%an %ad'\n\n?\n\n>  void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n>  {\n>  \tstruct strbuf ident = STRBUF_INIT;\n> @@ -243,6 +266,8 @@ 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> +\tif (log->groups & SHORTLOG_GROUP_PRETTY)\n> +\t\tinsert_record_from_pretty(log, &dups, commit, &ctx, oneline_str);\n\nI was puzzled at first that this was a bitwise check. But I forgot that\nwe added support for --group options already, in 63d24fa0b0 (shortlog:\nallow multiple groups to be specified, 2020-09-27).\n\nSo a plan like:\n\n  git shortlog --group=author --group=date\n\n(as in the original patch in this thread) doesn't quite work, I think.\nBecause the semantics for multiple --group lines are that the commit is\ncredited individually to each ident. That's what lets you do:\n\n  git shortlog -ns --group=author --group=trailer:co-authored-by\n\nand credit authors and co-authors equally. So likewise, I think multiple\ngroup-format options don't really make sense (or at least, do not make\nsense to concatenate; you'd put each key in its own single format).\n\n> @@ -321,8 +346,10 @@ 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\treturn error(_(\"unknown group type: %s\"), arg);\n> +\t} else {\n> +\t\tlog->groups |= SHORTLOG_GROUP_PRETTY;\n> +\t\tstring_list_append(&log->pretty, arg);\n> +\t}\n\nWe probably want to insist that the format contains a \"%\" sign, and/or\ngit it a keyword like \"format:\". Otherwise a typo like:\n\n  git shortlog --format=autor\n\nstops being an error we detect, and just returns nonsense results\n(every commit has the same ident).\n\nI think you'd want to detect SHORTLOG_GROUP_PRETTY in the\nread_from_stdin() path, too. And probably just die() with \"not\nsupported\", like we do for trailers.\n\n> I think you could also do some cleanup on top, like replacing the\n> SHORTLOG_GROUP_AUTHOR mode with adding either \"%aN <%aE>\" (or \"%aN\",\n> without --email) as an entry in the `pretty` string_list.\n\nYeah, that would be a nice cleanup. I think might even be a good idea to\nexplain the various options to the users in terms of \"--author is\nequivalent to %aN <%aE>\". It may help them understand how the tool\nworks.\n\n-Peff\n"},{"id":"464340","messageId":"Yz93RjrJ00A5QvNe.jacob@initialcommit.io","threadId":"58487","inReplyTo":"Yz4EsT8noIoygk9b@coredump.intra.peff.net","subject":"Re: [RFC PATCH v2] shortlog: add group-by options for year and month","fromName":"Jacob Stopak","fromEmail":"jacob@initialcommit.io","sentAt":"2022-10-07T00:48:06Z","receivedAt":"2022-10-07T00:48:17Z","isPatch":true,"sender":{"key":"jacob@initialcommit.io","avatar":"https://avatars.githubusercontent.com/u/49353917?v=4"},"body":"On Wed, Oct 05, 2022 at 06:26:57PM -0400, Jeff King wrote:\n> On Wed, Oct 05, 2022 at 05:43:20PM -0400, Taylor Blau wrote:\n> \n> > This caught my attention, so I wanted to see how hard it would be to\n> > implement. It actually is quite straightforward, and gets us most of the\n> > way to being able to get the same functionality as in Jacob's patch\n> > (minus being able to do the for-each-ref-style sub-selectors, like\n> > `%(authordate:format=%Y-%m)`).\n\nThanks Taylor!! This looks awesome and helped me understand how the pretty\ncontext stuff works. I was able to apply your patch locally and test,\nand plan to continue working off of this :D. Like Peff mentioned seems to\nbe a few usage details to hammer out.\n\n> The date thing I think can be done with --date; I just sent a sketch in\n> another part of the thread.\n\nPeff - I applied your --date sketch onto Taylor's patch and it worked first try.\n\n> So here you're allowing multiple pretty options. But really, once we\n> allow the user an arbitrary format, is there any reason for them to do:\n> \n>   git shortlog --group=%an --group=%ad\n> \n> versus just:\n> \n>   git shortlog --group='%an %ad'\n> \n> ?\n\nYes I can't think of an advantage of having multiple custom-formatted group\nfields. Also see my note on this below related to your comment on specifying\nmultiple groups.\n\n> >  void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n> >  {\n> >  \tstruct strbuf ident = STRBUF_INIT;\n> > @@ -243,6 +266,8 @@ 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> > +\tif (log->groups & SHORTLOG_GROUP_PRETTY)\n> > +\t\tinsert_record_from_pretty(log, &dups, commit, &ctx, oneline_str);\n> \n> I was puzzled at first that this was a bitwise check. But I forgot that\n> we added support for --group options already, in 63d24fa0b0 (shortlog:\n> allow multiple groups to be specified, 2020-09-27).\n> \n> So a plan like:\n> \n>   git shortlog --group=author --group=date\n> \n> (as in the original patch in this thread) doesn't quite work, I think.\n\nMy first patch addressed this by specifically handling cases where the new\ngrouping options were passed in-tandem with existing options, and making sure\nonly a single shortlog group was generated. But if we're generalizing the custom\ngroup format then it might be unecessary to even allow the custom group in tandem\nwith most other options (like 'author' and 'committer'), since those options can be\nincluded in the custom group format. The trailer option might be an exception, but\nthat could possibly just be handled as a special case.\n\n> We probably want to insist that the format contains a \"%\" sign, and/or\n> git it a keyword like \"format:\". Otherwise a typo like:\n> \n>   git shortlog --format=autor\n> \n> stops being an error we detect, and just returns nonsense results\n> (every commit has the same ident).\n\nSmall aside: I like how Taylor re-used the --group option for the custom format.\nIMO it hammers home that this is a grouping option and not just formatting or\nfiltering which can be confusing to users sometimes when doing data analytics.\n\nBut your points here all still apply. Maybe detecting a \"%\" sign can be the way\nthat git identifies the custom format being passed in. In conjuction with the %\nidentifiers, this would still enable users to add in some arbitrary constant label\nlike --group='Year: %cd' --date='%Y', without affecting the grouped/sorted results\nsince all entries would then include the \"Years: \" prefix (or postfix or however\nthey decide to write it).\n\nThe one case that should probably be handled is when no % sign is used and no other\nmatching flags like --author or --committer are either, because currently that will\njust group all commits under 1 nonsensical group name like \"autor\" as you mentioned.\n\n> I think you'd want to detect SHORTLOG_GROUP_PRETTY in the\n> read_from_stdin() path, too. And probably just die() with \"not\n> supported\", like we do for trailers.\n\nGlad you said this because I applied this in my original patch with the explicit\n--year and --month groups. Didn't seem to be an obvious use-case with the stdin \neven though my original year and month values could possibly be read from the git\nlog output supplied as stdin. But for a generalized group format seems even more\nfar-fetched to try and make it jive with the stdin version of the command.\n\n-Jack\n"},{"id":"464403","messageId":"Y0ChLlG+iOI3li6L@nand.local","threadId":"58487","inReplyTo":"Yz93RjrJ00A5QvNe.jacob@initialcommit.io","subject":"Re: [RFC PATCH v2] shortlog: add group-by options for year and month","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-07T21:59:10Z","receivedAt":"2022-10-07T21:59:15Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Oct 06, 2022 at 05:48:06PM -0700, Jacob Stopak wrote:\n> > The date thing I think can be done with --date; I just sent a sketch in\n> > another part of the thread.\n>\n> Peff - I applied your --date sketch onto Taylor's patch and it worked\n> first try.\n\nYeah; I think that allowing the `--date` argument (similar to how `git\nlog` treats an option by the same name) is sensible, albeit slightly\nless flexible than the proposed `%(authordate:format=%Y-%m)`.\n\nFor what it's worth, I don't have anything against the latter (other\nthan that it is slightly redundant in many cases with `--date`), but it\ndoes seem easier and more worthwhile to repurpose `--date` here instead.\n\nThanks,\nTaylor\n"},{"id":"464405","messageId":"Y0CnJBzTbNgRIqZ+@nand.local","threadId":"58487","inReplyTo":"Yz4EsT8noIoygk9b@coredump.intra.peff.net","subject":"Re: [RFC PATCH v2] shortlog: add group-by options for year and month","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-07T22:24:36Z","receivedAt":"2022-10-07T22:24:45Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Oct 05, 2022 at 06:26:57PM -0400, Jeff King wrote:\n> > +static void insert_record_from_pretty(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 ident = STRBUF_INIT;\n> > +\tsize_t i;\n> > +\n> > +\tfor (i = 0; i < log->pretty.nr; i++) {\n> > +\t\tif (i)\n> > +\t\t\tstrbuf_addch(&ident, ' ');\n> > +\n> > +\t\tformat_commit_message(commit, log->pretty.items[i].string,\n> > +\t\t\t\t      &ident, ctx);\n> > +\t}\n>\n> So here you're allowing multiple pretty options. But really, once we\n> allow the user an arbitrary format, is there any reason for them to do:\n>\n>   git shortlog --group=%an --group=%ad\n>\n> versus just:\n>\n>   git shortlog --group='%an %ad'\n>\n> ?\n\nI think that if you want to unify `--group=author` into the new format\ngroup implementation, you would have to allow multiple `--group`\noptions, but each such option would generate its own shortlog identity\ninstead of getting concatenated together.\n\nThanks,\nTaylor\n"},{"id":"464570","messageId":"Y0S/3Nl6PFWS3rTd@coredump.intra.peff.net","threadId":"58487","inReplyTo":"Yz93RjrJ00A5QvNe.jacob@initialcommit.io","subject":"Re: [RFC PATCH v2] shortlog: add group-by options for year and month","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-11T00:59:08Z","receivedAt":"2022-10-11T00:59:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 06, 2022 at 05:48:06PM -0700, Jacob Stopak wrote:\n\n> > We probably want to insist that the format contains a \"%\" sign, and/or\n> > git it a keyword like \"format:\". Otherwise a typo like:\n> > \n> >   git shortlog --format=autor\n> > \n> > stops being an error we detect, and just returns nonsense results\n> > (every commit has the same ident).\n> \n> Small aside: I like how Taylor re-used the --group option for the custom format.\n> IMO it hammers home that this is a grouping option and not just formatting or\n> filtering which can be confusing to users sometimes when doing data analytics.\n\nYeah, sorry this was just a typo/thinko on my part. It absolutely should\nbe --group, as --format already does something else.\n\n-Peff\n"},{"id":"464571","messageId":"Y0TAG3k1wK+ZfdzY@coredump.intra.peff.net","threadId":"58487","inReplyTo":"Y0CnJBzTbNgRIqZ+@nand.local","subject":"Re: [RFC PATCH v2] shortlog: add group-by options for year and month","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-11T01:00:11Z","receivedAt":"2022-10-11T01:00:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 07, 2022 at 06:24:36PM -0400, Taylor Blau wrote:\n\n> > So here you're allowing multiple pretty options. But really, once we\n> > allow the user an arbitrary format, is there any reason for them to do:\n> >\n> >   git shortlog --group=%an --group=%ad\n> >\n> > versus just:\n> >\n> >   git shortlog --group='%an %ad'\n> >\n> > ?\n> \n> I think that if you want to unify `--group=author` into the new format\n> group implementation, you would have to allow multiple `--group`\n> options, but each such option would generate its own shortlog identity\n> instead of getting concatenated together.\n\nExactly, and I think we have to do that anyway to match the existing\nmultiple-option behavior.\n\n-Peff\n"}]}