Re: [RFC PATCH] shortlog: add group-by options for year and month

["Martin Ågren" <martin.agren@gmail.com>]

Hi Jacob,

On Thu, 22 Sept 2022 at 09:48, Jacob Stopak <jacob@initialcommit.io> wrote:
>
> Oh and third, for documentation, I updated "git-shortlog.txt", but
> wasn't able to test "git help shortlog" locally and see the updates. Is
> there a way to make that work locally or did I miss a step somewhere?

I can think of two steps. You need to build the documentation (`make
doc`). Look for "make doc" in INSTALL for some variants and
dependencies. Then you need to convince `git help ...` to pick up your
built docs. I would actually skip using/testing `git help` and just go
straight for the rendered page using, e.g, something like

  cd Documentation
  make ./git-shortlog.{1,html}
  man ./git-shortlog.1
  a-browser ./git-shortlog.html

Your docs render fine for me. Thanks for `backticking` for monospace.

> +-m::
> +--month::
> +       This is an alias for `--group=month`.
> +
> +-y::
> +--year::
> +       This is an alias for `--group=year`.
> +

Commit 2338c450b ("shortlog: add grouping option", 2020-09-27)
introduced `--group` and redefined `-c`  and `--committer` to alias to
that new thing. You could simply add a `--group` variant without
actually adding `--year` and `--month`. One of the nice things about
`--group` is that we can potentially have many groupings without having
to carry correspondingly many `--option`s.

In particular, it might be wise to wait with implementing `-y` and `-m`
until we know that your new feature turns out to be so hugely successful
that people start craving `-m` as a short form for `--group=month`. ;-)

See commit 47beb37bc6 ("shortlog: match commit trailers with --group",
2020-09-27) for some prior art of not adding a new `--option` for a new
way of grouping.

>         struct strbuf ident = STRBUF_INIT;
>         struct strbuf oneline = STRBUF_INIT;
> +       struct strbuf buffer;
> +       strbuf_init(&buffer, 100);
> +
>         struct strset dups = STRSET_INIT;
>         struct pretty_print_context ctx = {0};

This trips up `-Werror=declaration-after-statement`. If you build with
`DEVELOPER=Yes`, you should see the same thing.

I played a little with this functionality and it's quite cute. I can
easily imagine going even more granular with this (`--group=week`?), but
that can wait for some other time. :-)

BTW, I got this when `git am`-ing your patch:

  Applying: shortlog: add group-by options for year and month
  .git/rebase-apply/patch:82: space before tab in indent.
                  strftime(sb->buf, strbuf_avail(sb), "%Y/%m", &commit_date);
  .git/rebase-apply/patch:85: space before tab in indent.
                  strftime(sb->buf, strbuf_avail(sb), "%Y", &commit_date);
  .git/rebase-apply/patch:110: trailing whitespace.
                  format_commit_message(commit,
  .git/rebase-apply/patch:111: trailing whitespace, indent with spaces.
                                        buffer.buf,
  .git/rebase-apply/patch:112: indent with spaces.
                                        &ident, &ctx);
  warning: squelched 6 whitespace errors
  warning: 11 lines add whitespace errors.

Martin