| Message ID | 20240320002851.GB904136@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 69aff6200c51cc8a91111b80fbfb84792ce0908c |
| Headers | show |
| Series | [1/6] shortlog: stop setting pp.print_email_subject | expand |
On Wed, Mar 20, 2024, at 01:28, Jeff King wrote: > The pp_title_line() function is used for two formats: the oneline format > and the subject line of the email format. But most of the logic in the > function does not make any sense for oneline; it is about special > formatting of email headers. > > Lumping the two formats together made sense long ago in 4234a76167 > (Extend --pretty=oneline to cover the first paragraph, 2007-06-11), when > there was a lot of manual logic to paste lines together. But later, > 88c44735ab (pretty: factor out format_subject(), 2008-12-27) pulled that > logic into its own function. > > We can implement the oneline format by just calling that one function. > This makes the intention of the code much more clear, as we know we only > need to worry about those extra email options when dealing with actual > email. > > While the intent here is cleanup, it is possible to trigger these cases > in practice by running format-patch with an explicit --oneline option. > But if you did, the results are basically nonsense. For example, with > the preserve_subject flag: > > $ printf "%s\n" one two three | git commit --allow-empty -F - > $ git format-patch -1 --stdout -k | grep ^Subject > Subject: =?UTF-8?q?one=0Atwo=0Athree?= > $ git format-patch -1 --stdout -k --oneline --no-signature > 2af7fbe one > two > three > > Or with extra headers: > > $ git format-patch -1 --stdout --cc=me --oneline --no-signature > 2af7fbe one two three > Cc: me > > So I'd actually consider this to be an improvement, though you are > probably crazy to use other formats with format-patch in the first place > (arguably it should forbid non-email formats entirely, but that's a > bigger change). Makes sense. This make the code more focused. > As a bonus, it eliminates some pointless extra allocations for the > oneline output. The email code, since it has to deal with wrapping, > formats into an extra auxiliary buffer. The speedup is tiny, though like > "rev-list --no-abbrev --format=oneline" seems to improve by a consistent > 1-2% for me. Nice. That could add up when formatting a moderate amount of patches.
diff --git a/builtin/log.c b/builtin/log.c index e5da0d1043..89cce9c29d 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -1297,7 +1297,7 @@ static void prepare_cover_text(struct pretty_print_context *pp, subject = subject_sb.buf; do_pp: - pp_title_line(pp, &subject, sb, encoding, need_8bit_cte); + pp_email_subject(pp, &subject, sb, encoding, need_8bit_cte); pp_remainder(pp, &body, sb, 0); strbuf_release(&description_sb); diff --git a/pretty.c b/pretty.c index bdbed4295a..be0f2f566d 100644 --- a/pretty.c +++ b/pretty.c @@ -2077,11 +2077,11 @@ static void pp_header(struct pretty_print_context *pp, } } -void pp_title_line(struct pretty_print_context *pp, - const char **msg_p, - struct strbuf *sb, - const char *encoding, - int need_8bit_cte) +void pp_email_subject(struct pretty_print_context *pp, + const char **msg_p, + struct strbuf *sb, + const char *encoding, + int need_8bit_cte) { static const int max_length = 78; /* per rfc2047 */ struct strbuf title; @@ -2126,9 +2126,8 @@ void pp_title_line(struct pretty_print_context *pp, if (pp->after_subject) { strbuf_addstr(sb, pp->after_subject); } - if (cmit_fmt_is_mail(pp->fmt)) { - strbuf_addch(sb, '\n'); - } + + strbuf_addch(sb, '\n'); if (pp->in_body_headers.nr) { int i; @@ -2328,8 +2327,11 @@ void pretty_print_commit(struct pretty_print_context *pp, msg = skip_blank_lines(msg); /* These formats treat the title line specially. */ - if (pp->fmt == CMIT_FMT_ONELINE || cmit_fmt_is_mail(pp->fmt)) - pp_title_line(pp, &msg, sb, encoding, need_8bit_cte); + if (pp->fmt == CMIT_FMT_ONELINE) { + msg = format_subject(sb, msg, " "); + strbuf_addch(sb, '\n'); + } else if (cmit_fmt_is_mail(pp->fmt)) + pp_email_subject(pp, &msg, sb, encoding, need_8bit_cte); beginning_of_body = sb->len; if (pp->fmt != CMIT_FMT_ONELINE) diff --git a/pretty.h b/pretty.h index 421209e9ec..d4ff79deb3 100644 --- a/pretty.h +++ b/pretty.h @@ -96,13 +96,13 @@ void pp_user_info(struct pretty_print_context *pp, const char *what, const char *encoding); /* - * Format title line of commit message taken from "msg_p" and + * Format subject line of commit message taken from "msg_p" and * put it into "sb". * First line of "msg_p" is also affected. */ -void pp_title_line(struct pretty_print_context *pp, const char **msg_p, - struct strbuf *sb, const char *encoding, - int need_8bit_cte); +void pp_email_subject(struct pretty_print_context *pp, const char **msg_p, + struct strbuf *sb, const char *encoding, + int need_8bit_cte); /* * Get current state of commit message from "msg_p" and continue formatting
The pp_title_line() function is used for two formats: the oneline format and the subject line of the email format. But most of the logic in the function does not make any sense for oneline; it is about special formatting of email headers. Lumping the two formats together made sense long ago in 4234a76167 (Extend --pretty=oneline to cover the first paragraph, 2007-06-11), when there was a lot of manual logic to paste lines together. But later, 88c44735ab (pretty: factor out format_subject(), 2008-12-27) pulled that logic into its own function. We can implement the oneline format by just calling that one function. This makes the intention of the code much more clear, as we know we only need to worry about those extra email options when dealing with actual email. While the intent here is cleanup, it is possible to trigger these cases in practice by running format-patch with an explicit --oneline option. But if you did, the results are basically nonsense. For example, with the preserve_subject flag: $ printf "%s\n" one two three | git commit --allow-empty -F - $ git format-patch -1 --stdout -k | grep ^Subject Subject: =?UTF-8?q?one=0Atwo=0Athree?= $ git format-patch -1 --stdout -k --oneline --no-signature 2af7fbe one two three Or with extra headers: $ git format-patch -1 --stdout --cc=me --oneline --no-signature 2af7fbe one two three Cc: me So I'd actually consider this to be an improvement, though you are probably crazy to use other formats with format-patch in the first place (arguably it should forbid non-email formats entirely, but that's a bigger change). As a bonus, it eliminates some pointless extra allocations for the oneline output. The email code, since it has to deal with wrapping, formats into an extra auxiliary buffer. The speedup is tiny, though like "rev-list --no-abbrev --format=oneline" seems to improve by a consistent 1-2% for me. Signed-off-by: Jeff King <peff@peff.net> --- builtin/log.c | 2 +- pretty.c | 22 ++++++++++++---------- pretty.h | 8 ++++---- 3 files changed, 17 insertions(+), 15 deletions(-)