On Fri, Sep 23, 2022 at 09:17:10AM -0700, Junio C Hamano wrote:
> 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?
It uses the "commit->date" member from the commit struct in commit.h,
which I assumed was the committer timestamp, but I'll confirm that's
what actually gets populated in there since I don't see a separate
member for the author timestamp...
> 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?
I need to look into how Git typically handles these timezone differences
and will try to apply similar behavior for these time-based groupings.
> 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.
Yes, "group by month and year" and "group by month" means the same
thing the way I implemented. If both groups are supplied, it will
just ignore the year group and group by month like you mentioned,
by flipping off the YEAR bit as follows:
if ((log->groups & SHORTLOG_GROUP_MONTH) &&
(log->groups & SHORTLOG_GROUP_YEAR))
log->groups ^= SHORTLOG_GROUP_YEAR;
I can add a warning message to make this more clear to the user.
> 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.
In data analytics terms this is usually referred to as "month of year",
and personally I see it less useful in Git's shortlog context because I
envision more folks would find output useful for single or consecutive
time periods. However, adding a "month of year" grouping could be useful
to answer questions like "what periods throughout the year are contributors
most active?". If we decide to add a "month of year" grouping option as
well, it would be trivial to include.
> 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.
At first look this sounds very flexible and appealing, and I would be
interested in exploring a refactor to this in the future. I think the
rub is that supplying custom patterns wouldn't necessarily stack up
neatly into good groups, which could lead to confusing results for the
user in terms of both grouping and sorting. But like you mentioned
it could be really cool if used judiciously for a consistent history
like Git's. And the generalized re-implementation of the current
shortlog groups would be a nice bonus.
> > @@ -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.
No worries - I'll remove the shorthand flags for v3.
