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

From: Martin Ågren <martin.agren@gmail.com>
Date: Sep 22, 2022, 15:46 UTC
Message-ID: <CAN0heSr8HFR1y+aZUjFaeY3y-9yn+nfyDrkxQh82punLnSonGg@mail.gmail.com>
In-Reply-To: <20220922061824.16988-1-jacob@initialcommit.io>

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.

Show 8 quoted lines

> +-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.

Show 7 quoted lines

>         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