| Message ID | 20220826213203.3258022-2-gitster@pobox.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | format-patch --force-inbody-from | expand |
Hi Junio, On Fri, 26 Aug 2022, Junio C Hamano wrote: > When pretty-printing the log message for a given commit in e-mail > format (e.g. "git format-patch"), we add in-body "From:" header when > the author identity of the commit is different from the identity of > the person who is "sending" the mail. The quotes around "sending" made me stumble over this a bit. Maybe replace it by saying "the person running the command"? > Split out the logic into a helper function. Because the "from_ident > must be set" condition is there not because it is used in the > ident_cmp() next, but because the codepath that is entered when this > logic says "Yes, you should use in-body from" requires values there > in from_ident member, so separate it out into an if() statement on > its own to clarify it. Even after reading this three times, I had trouble understanding it. I then consulted the diff and started to grasp what you mean. I have no good idea how to improve the wording, but maybe you can give it another go? Or simply state that the condition was untangled a bit. The diff looks good. Ciao, Dscho P.S.: I do not know how strongly you feel these days about lines longer than 80 columns, but personally I do not care about this rule, so I am more than just fine with adding such a line here. > diff --git a/pretty.c b/pretty.c > index 6d819103fb..51e3fa5736 100644 > --- a/pretty.c > +++ b/pretty.c > @@ -477,6 +477,15 @@ static void append_line_with_color(struct strbuf *sb, struct grep_opt *opt, > } > } > > +static int use_inbody_from(const struct pretty_print_context *pp, const struct ident_split *ident) > +{ > + if (!pp->from_ident) > + return 0; > + if (ident_cmp(pp->from_ident, ident)) > + return 1; > + return 0; > +} > + > void pp_user_info(struct pretty_print_context *pp, > const char *what, struct strbuf *sb, > const char *line, const char *encoding) > @@ -503,7 +512,7 @@ void pp_user_info(struct pretty_print_context *pp, > map_user(pp->mailmap, &mailbuf, &maillen, &namebuf, &namelen); > > if (cmit_fmt_is_mail(pp->fmt)) { > - if (pp->from_ident && ident_cmp(pp->from_ident, &ident)) { > + if (use_inbody_from(pp, &ident)) { > struct strbuf buf = STRBUF_INIT; > > strbuf_addstr(&buf, "From: "); > -- > 2.37.2-587-g47adba97a9 > >
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > Hi Junio, > > On Fri, 26 Aug 2022, Junio C Hamano wrote: > >> When pretty-printing the log message for a given commit in e-mail >> format (e.g. "git format-patch"), we add in-body "From:" header when >> the author identity of the commit is different from the identity of >> the person who is "sending" the mail. > > The quotes around "sending" made me stumble over this a bit. Maybe replace > it by saying "the person running the command"? It reads much better. >> Split out the logic into a helper function. Because the "from_ident >> must be set" condition is there not because it is used in the >> ident_cmp() next, but because the codepath that is entered when this >> logic says "Yes, you should use in-body from" requires values there >> in from_ident member, so separate it out into an if() statement on >> its own to clarify it. > > Even after reading this three times, I had trouble understanding it. I > then consulted the diff and started to grasp what you mean. I have no > good idea how to improve the wording, but maybe you can give it another > go? Or simply state that the condition was untangled a bit. Yeah, (pp->from_ident != NULL) there serves two purposes. Whatever else is in that if() statement, the body of the statement depends on the pp->from_ident being non-NULL and the control must not enter there otherwise. The other purpose is to guard the other half of the if() statement, which happens to be ident_cmp() that looks at the same pp->from_ident and depends on it being non-NULL. I think the condition gets much cleaner by untangling it to - use_inbody_from() function does *not* check pp->from_ident; it just assumes it is not NULL - the caller becomes if (pp->from_ident && use_inbody_from(...)) { ... stuff that use pp->from_ident ... } > P.S.: I do not know how strongly you feel these days about lines longer > than 80 columns, but personally I do not care about this rule, so I am > more than just fine with adding such a line here. I allowed wider line for function decl, by inertia, for greppability, but I should fix that. Thanks for noticing.
diff --git a/pretty.c b/pretty.c index 6d819103fb..51e3fa5736 100644 --- a/pretty.c +++ b/pretty.c @@ -477,6 +477,15 @@ static void append_line_with_color(struct strbuf *sb, struct grep_opt *opt, } } +static int use_inbody_from(const struct pretty_print_context *pp, const struct ident_split *ident) +{ + if (!pp->from_ident) + return 0; + if (ident_cmp(pp->from_ident, ident)) + return 1; + return 0; +} + void pp_user_info(struct pretty_print_context *pp, const char *what, struct strbuf *sb, const char *line, const char *encoding) @@ -503,7 +512,7 @@ void pp_user_info(struct pretty_print_context *pp, map_user(pp->mailmap, &mailbuf, &maillen, &namebuf, &namelen); if (cmit_fmt_is_mail(pp->fmt)) { - if (pp->from_ident && ident_cmp(pp->from_ident, &ident)) { + if (use_inbody_from(pp, &ident)) { struct strbuf buf = STRBUF_INIT; strbuf_addstr(&buf, "From: ");
When pretty-printing the log message for a given commit in e-mail format (e.g. "git format-patch"), we add in-body "From:" header when the author identity of the commit is different from the identity of the person who is "sending" the mail. Split out the logic into a helper function. Because the "from_ident must be set" condition is there not because it is used in the ident_cmp() next, but because the codepath that is entered when this logic says "Yes, you should use in-body from" requires values there in from_ident member, so separate it out into an if() statement on its own to clarify it. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- pretty.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)