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

From: Junio C Hamano <[email-masked]>
Date: Fri, 23 Sep 2022 09:17:10 -0700

Jacob Stopak <[email-masked]> writes:

> To justify why the new year/month groups should have shorthand flags
> while "--group=trailer:value" does not, I'd say that the fact that
> trailer requires a custom value would make the shorthand version clunky,
> and it wouldn't fit in well with other shorthand options like -n or -s.
> Since year/month have no custom value, the flags make a bit more sense
> and would match up with how "-c, --committer" currently works.

It is an explanation why it is easier to implement and design --year
etc., and does not justify why adding --year etc. is a good idea at
all.  Let's not add the "aliases" in the same patch.

>   - `author`, commits are grouped by author
>   - `committer`, commits are grouped by committer (the same as `-c`)
> + - `month`, commits are grouped by month (the same as `-m`)
> + - `year`, commits are grouped by year (the same as `-y`)

It is unclear what timestamp is used, how a "month" is defined, etc.
As "git shortlog --since=2.years" cuts off based on the committer
timestamp, I would expect that the committer timestamps are used for
this grouping as well?  If I make a commit on the first day of the
month in my timezone, but that instant happens to be still on the
last day of the previous month in your timezone, which month would
your invocation of "git shortlog --group=month" would the commit be
attributed?  My month, or your month?

Does it make sense to even say "group by month and year"?  I expect
that it would mean the same thing as "group by month", and if that
is the case, the command probably should error out or at least warn
if both are given.  An alternative interpretation could be, when
told to "group by month", group the commits made in September 2022
into the same group as the group for commits made in September in
all other years, but I do not know how useful it would be.

Not a suggestion to use a different implementation or add a new
feature on top of this --group-by-time-range idea, but I wonder if
it is a more flexible and generalizeable approach to say "formulate
this value given by the --format=<format> string, apply this regular
expression match, and group by the subexpression value".  E.g.

    git shortlog \
	--group-by-value="%cI" \
	--group-by-regexp="^(\d{4}-\d{2})"

would "formulate the 'committer date in ISO format' value, and apply
the 'grab leading 4 digits followed by a dash followed by 2 digits'
regexp, and group by the matched part".

That's a better way to implement "group by month" internally, and
allow more flexibility.  If a project is well disciplined and its
commit titles follow the "<area>: <description>" convention, you
probably could do

    git shortlog --no-merges \
	--group-by-value="%s" \
	--group-by-regexp="^([^:]+):"

and group by <area> each commit touches.  Of course, existing
--committer and --author can also be internally reimplemented using
the same mechanism.

> @@ -80,6 +82,14 @@ counts both authors and co-authors.
>  --committer::
>  	This is an alias for `--group=committer`.
>  
> +-m::
> +--month::
> +	This is an alias for `--group=month`.
> +
> +-y::
> +--year::
> +	This is an alias for `--group=year`.
> +

Let's not add this in the same patch.  I am fairly negative on
adding more, outside "--group".  Besides, we do not have a good
answer to those who want to group by week.  -w is already taken.