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