| Message ID | cover.1665448437.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | shortlog: introduce `--group=<format>` | expand |
Here is one more very tiny reroll of my series to implement arbitrary pretty formats as shortlog `--group`'s, based on a suggestion from Jacob Stopak. There are only a couple of changes from last time: one to rebase onto the current tip of 'master', and another to address a bug in 4/7 (which was resolved by the end of the series, but now works consistently throughout the series). This was pointed out by Peff in [1], and he indicated there: > It's hard to care too much, since the end result of the series is > correct, and you'd end up just removing that part of the line in > the final patch. So I could go either way on re-rolling. ...but not re-rolling would be somewhat unsatisfying. So here is a reroll that I think should be good to go. Thanks in advance for your review. [1]: https://lore.kernel.org/git/Y1M5OH%2FFiWNaKO6p@coredump.intra.peff.net/ Jeff King (1): shortlog: accept `--date`-related options Taylor Blau (6): shortlog: make trailer insertion a noop when appropriate shortlog: extract `--group` fragment for translation shortlog: support arbitrary commit format `--group`s shortlog: extract `shortlog_finish_setup()` shortlog: implement `--group=author` in terms of `--group=<format>` shortlog: implement `--group=committer` in terms of `--group=<format>` Documentation/git-shortlog.txt | 8 ++++ builtin/log.c | 1 + builtin/shortlog.c | 87 +++++++++++++++++++++++----------- shortlog.h | 5 ++ t/t4201-shortlog.sh | 39 +++++++++++++++ 5 files changed, 113 insertions(+), 27 deletions(-)
Taylor Blau <me@ttaylorr.com> writes: > Here is one more very tiny reroll of my series to implement arbitrary > pretty formats as shortlog `--group`'s, based on a suggestion from Jacob > Stopak. > > There are only a couple of changes from last time: one to rebase onto > the current tip of 'master', and another to address a bug in 4/7 (which > was resolved by the end of the series, but now works consistently > throughout the series). > > This was pointed out by Peff in [1], and he indicated there: > >> It's hard to care too much, since the end result of the series is >> correct, and you'd end up just removing that part of the line in >> the final patch. So I could go either way on re-rolling. One "huh?" is that the final patch in this round hasn't changed from the last round, so the check for HAS_MULTI_BITS(log->groups) remains in the final result. IOW, after replacing the previous series with this round, $ git diff @{1} diff --git c/builtin/shortlog.c w/builtin/shortlog.c index 27b057940d..27a87167e1 100644 --- c/builtin/shortlog.c +++ w/builtin/shortlog.c @@ -207,7 +207,7 @@ static void insert_records_from_trailers(struct shortlog *log, static int shortlog_needs_dedup(const struct shortlog *log) { - return log->format.nr > 1 || log->trailers.nr; + return HAS_MULTI_BITS(log->groups) || log->format.nr > 1 || log->trailers.nr; } static void insert_records_from_format(struct shortlog *log, is the difference in the end result.
On Mon, Oct 24, 2022 at 02:55:26PM -0400, Taylor Blau wrote: > There are only a couple of changes from last time: one to rebase onto > the current tip of 'master', and another to address a bug in 4/7 (which > was resolved by the end of the series, but now works consistently > throughout the series). > > This was pointed out by Peff in [1], and he indicated there: > > > It's hard to care too much, since the end result of the series is > > correct, and you'd end up just removing that part of the line in > > the final patch. So I could go either way on re-rolling. > > ...but not re-rolling would be somewhat unsatisfying. So here is a > reroll that I think should be good to go. OK, good. It offended my sensibilities, too, but I didn't want to force extra work on you because of them. I'm glad you agreed. ;) This isn't mark v4, but the range-diff from v3 is: 1: 31487229e6 = 1: e79db4b987 shortlog: accept `--date`-related options 2: be2c6c0f4c = 2: 81e91f7049 shortlog: make trailer insertion a noop when appropriate 3: bd38ac66f2 = 3: cde611e3b0 shortlog: extract `--group` fragment for translation 4: 277ffe92ce ! 4: 020a2175cb shortlog: support arbitrary commit format `--group`s @@ builtin/shortlog.c: static void insert_records_from_trailers(struct shortlog *lo +static int shortlog_needs_dedup(const struct shortlog *log) +{ -+ return log->format.nr > 1 || log->trailers.nr; ++ return HAS_MULTI_BITS(log->groups) || log->format.nr > 1 || log->trailers.nr; +} + +static void insert_records_from_format(struct shortlog *log, 5: 6c02a2daab = 5: 13a1f1b8c8 shortlog: extract `shortlog_finish_setup()` 6: 969bdaae39 = 6: 8af036c9f8 shortlog: implement `--group=author` in terms of `--group=<format>` 7: bad7a2bc68 = 7: 09d138353b shortlog: implement `--group=committer` in terms of `--group=<format>` I had assumed you would drop the HAS_MULTI_BITS() part from this function after patch 7, as we know that everything is either a format or a trailer. But actually, I like keeping it. It future-proofs us against adding more non-format group types (though I have trouble imagining what those would be), and it's not like it's expensive to keep around. So this iteration looks good to me. Thanks! -Peff
This series is based on a suggestion[1] from Junio in response to Jacob Stopak's series to add committer month/year options as shortlog groups. This series pursues a more flexible approach, where arbitrary pretty formats can be used to generate group names in shortlog. For example, to group by committer month/year, you can do the following: $ git.compile shortlog -s --group='%cd' --date='format:%Y-%m' v2.37.0.. 117 2022-06 274 2022-07 325 2022-08 271 2022-09 17 2022-10 , which is cute. It can also be used to reimplement existing functionality in shortlog, which is what the final three patches in this series do. Author and committer options are implemented under the hood as if they were shorthands for `--group='%aN <%aE>'` (or `--group='%cN <%cE>'`, respectively). The `--group=trailer:<key>` option is also reimplemented, but it is slightly trickier than the rest. See the final patch for more details there. Thanks in advance for your review. [1]: https://lore.kernel.org/git/xmqqillevzeh.fsf@gitster.g/ Jeff King (1): shortlog: accept `--date`-related options Taylor Blau (6): Documentation: extract date-options.txt shortlog: extract `--group` fragment for translation shortlog: support arbitrary commit format `--group`s shortlog: implement `--group=author` in terms of `--group=<format>` shortlog: implement `--group=committer` in terms of `--group=<format>` shortlog: implement `--group=trailer` in terms of `--group=<format>` Documentation/date-options.txt | 66 +++++++++++++ Documentation/git-shortlog.txt | 4 + Documentation/rev-list-options.txt | 67 +------------ builtin/log.c | 1 + builtin/shortlog.c | 145 +++++++++++++++-------------- shortlog.h | 6 +- t/t4201-shortlog.sh | 9 ++ 7 files changed, 163 insertions(+), 135 deletions(-) create mode 100644 Documentation/date-options.txt