* [PATCH 0/2] format-patch --force-inbody-from @ 2022-08-26 21:32 Junio C Hamano 2022-08-26 21:32 ` [PATCH 1/2] pretty: separate out the logic to decide the use of in-body from Junio C Hamano ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: Junio C Hamano @ 2022-08-26 21:32 UTC (permalink / raw) To: git; +Cc: Konstantin Ryabitsev, Rasmus Villemoes Users may be authoring and committing their commits under the same e-mail address they use to send their patches from, in which case they shouldn't need to use the in-body From: line in their outgoing e-mails. At the receiving end, "git am" will use the address on the "From:" header of the incoming e-mail and all should be well. Some mailing lists, however, mangle the From: address from what the original sender had; in such an unfortunate situation, the user may want to add the in-body "From:" header even for their own patch. The two-patch series may help such users. * The first one introduces a small helper to separate the logic that decides when in-body From: is used. * The second one adds a minimum support with a new test. Junio C Hamano (2): pretty: separate out the logic to decide the use of in-body from format-patch: allow forcing the use of in-body From: header builtin/log.c | 2 ++ pretty.c | 13 ++++++++++++- revision.h | 1 + t/t4014-format-patch.sh | 13 +++++++++++++ 4 files changed, 28 insertions(+), 1 deletion(-) -- 2.37.2-587-g47adba97a9 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] pretty: separate out the logic to decide the use of in-body from 2022-08-26 21:32 [PATCH 0/2] format-patch --force-inbody-from Junio C Hamano @ 2022-08-26 21:32 ` Junio C Hamano 2022-08-29 11:32 ` Johannes Schindelin 2022-08-26 21:32 ` [PATCH 2/2] format-patch: allow forcing the use of in-body From: header Junio C Hamano 2022-08-29 21:38 ` [PATCH v2 0/3] format-patch --force-in-body-from Junio C Hamano 2 siblings, 1 reply; 13+ messages in thread From: Junio C Hamano @ 2022-08-26 21:32 UTC (permalink / raw) To: git 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(-) 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] pretty: separate out the logic to decide the use of in-body from 2022-08-26 21:32 ` [PATCH 1/2] pretty: separate out the logic to decide the use of in-body from Junio C Hamano @ 2022-08-29 11:32 ` Johannes Schindelin 2022-08-29 17:29 ` Junio C Hamano 0 siblings, 1 reply; 13+ messages in thread From: Johannes Schindelin @ 2022-08-29 11:32 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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 > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] pretty: separate out the logic to decide the use of in-body from 2022-08-29 11:32 ` Johannes Schindelin @ 2022-08-29 17:29 ` Junio C Hamano 0 siblings, 0 replies; 13+ messages in thread From: Junio C Hamano @ 2022-08-29 17:29 UTC (permalink / raw) To: Johannes Schindelin; +Cc: git 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/2] format-patch: allow forcing the use of in-body From: header 2022-08-26 21:32 [PATCH 0/2] format-patch --force-inbody-from Junio C Hamano 2022-08-26 21:32 ` [PATCH 1/2] pretty: separate out the logic to decide the use of in-body from Junio C Hamano @ 2022-08-26 21:32 ` Junio C Hamano 2022-08-29 11:48 ` Johannes Schindelin 2022-08-29 21:38 ` [PATCH v2 0/3] format-patch --force-in-body-from Junio C Hamano 2 siblings, 1 reply; 13+ messages in thread From: Junio C Hamano @ 2022-08-26 21:32 UTC (permalink / raw) To: git Users may be authoring and committing their commits under the same e-mail address they use to send their patches from, in which case they shouldn't need to use the in-body From: line in their outgoing e-mails. At the receiving end, "git am" will use the address on the "From:" header of the incoming e-mail and all should be well. Some mailing lists, however, mangle the From: address from what the original sender had; in such an unfortunate situation, the user may want to add the in-body "From:" header even for their own patch. "git format-patch --[no-]force-inbody-from" was invented for such users. Note. This is an uncooked early draft. Things to think about include (but not limited to, of course): * Should this rather be --use-inbody-from=yes,no,auto tristate, that defaults to "auto", which is the current behaviour i.e. "when --from is given, add it only when it does not match the payload". "yes" would mean "always emit the --from address as in-body From:" and "no" would mean ... what? "Ignore --from"? Then why is the user giving --from in the first place? * Should it be "inbody" or "in-body"? * Should it have a corresponding configuration variable? * Should this patch be scrapped and the feature should be done inside "git send-email" instead? Signed-off-by: Junio C Hamano <gitster@pobox.com> --- builtin/log.c | 2 ++ pretty.c | 2 ++ revision.h | 1 + t/t4014-format-patch.sh | 13 +++++++++++++ 4 files changed, 18 insertions(+) diff --git a/builtin/log.c b/builtin/log.c index 9b937d59b8..83b2d01b49 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -1897,6 +1897,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) N_("show changes against <refspec> in cover letter or single patch")), OPT_INTEGER(0, "creation-factor", &creation_factor, N_("percentage by which creation is weighted")), + OPT_BOOL(0, "force-inbody-from", &rev.force_inbody_from, + N_("Use in-body From: even for your own commit")), OPT_END() }; diff --git a/pretty.c b/pretty.c index 51e3fa5736..e266208c0b 100644 --- a/pretty.c +++ b/pretty.c @@ -483,6 +483,8 @@ static int use_inbody_from(const struct pretty_print_context *pp, const struct i return 0; if (ident_cmp(pp->from_ident, ident)) return 1; + if (pp->rev && pp->rev->force_inbody_from) + return 1; return 0; } diff --git a/revision.h b/revision.h index bb91e7ed91..a2d3813a21 100644 --- a/revision.h +++ b/revision.h @@ -208,6 +208,7 @@ struct rev_info { /* Format info */ int show_notes; + unsigned int force_inbody_from; unsigned int shown_one:1, shown_dashes:1, show_merge:1, diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh index fbec8ad2ef..a4ecd433e2 100755 --- a/t/t4014-format-patch.sh +++ b/t/t4014-format-patch.sh @@ -1400,6 +1400,19 @@ test_expect_success '--from omits redundant in-body header' ' test_cmp expect patch.head ' +test_expect_success 'with --force-inbody-from, --from keeps redundant in-body header' ' + git format-patch --force-inbody-from \ + -1 --stdout --from="A U Thor <author@example.com>" >patch && + cat >expect <<-\EOF && + From: A U Thor <author@example.com> + + From: A U Thor <author@example.com> + + EOF + sed -ne "/^From:/p; /^$/p; /^---$/q" patch >patch.head && + test_cmp expect patch.head +' + test_expect_success 'in-body headers trigger content encoding' ' test_env GIT_AUTHOR_NAME="éxötìc" test_commit exotic && test_when_finished "git reset --hard HEAD^" && -- 2.37.2-587-g47adba97a9 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] format-patch: allow forcing the use of in-body From: header 2022-08-26 21:32 ` [PATCH 2/2] format-patch: allow forcing the use of in-body From: header Junio C Hamano @ 2022-08-29 11:48 ` Johannes Schindelin 2022-08-29 17:41 ` Junio C Hamano 0 siblings, 1 reply; 13+ messages in thread From: Johannes Schindelin @ 2022-08-29 11:48 UTC (permalink / raw) To: Junio C Hamano; +Cc: git [-- Attachment #1: Type: text/plain, Size: 5108 bytes --] Hi Junio, On Fri, 26 Aug 2022, Junio C Hamano wrote: > Users may be authoring and committing their commits under the same > e-mail address they use to send their patches from, in which case > they shouldn't need to use the in-body From: line in their outgoing > e-mails. At the receiving end, "git am" will use the address on the > "From:" header of the incoming e-mail and all should be well. > > Some mailing lists, however, mangle the From: address from what the > original sender had; in such an unfortunate situation, the user may > want to add the in-body "From:" header even for their own patch. > > "git format-patch --[no-]force-inbody-from" was invented for such > users. > > Note. This is an uncooked early draft. Did you mean to mark the patch as [RFC], then? > Things to think about include (but not limited to, of course): > > * Should this rather be --use-inbody-from=yes,no,auto tristate, > that defaults to "auto", which is the current behaviour i.e. > "when --from is given, add it only when it does not match the > payload". "yes" would mean "always emit the --from address as > in-body From:" and "no" would mean ... what? "Ignore --from"? > Then why is the user giving --from in the first place? I would offer up the suggestion `--in-body-from={never,always,auto}` for consideration. > * Should it be "inbody" or "in-body"? The latter. > * Should it have a corresponding configuration variable? Probably. The commit message talks about mailing lists requiring different behavior from the default, which is likely to affect all patches generated from a corresponding local checkout. Having a config variable would lower the cognitive burden of having to remember this process detail. > * Should this patch be scrapped and the feature should be done > inside "git send-email" instead? Since it affects the `--pretty=email` mode, the current patch seems to aim for the correct layer. > diff --git a/builtin/log.c b/builtin/log.c > index 9b937d59b8..83b2d01b49 100644 > --- a/builtin/log.c > +++ b/builtin/log.c > @@ -1897,6 +1897,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) > N_("show changes against <refspec> in cover letter or single patch")), > OPT_INTEGER(0, "creation-factor", &creation_factor, > N_("percentage by which creation is weighted")), > + OPT_BOOL(0, "force-inbody-from", &rev.force_inbody_from, > + N_("Use in-body From: even for your own commit")), Please start the usage text in lower-case, to keep it consistent with the rest of the usage texts. Also, I would like to avoid the personal address "you" in that text, and also the verb "use". Maybe something like this: show in-body From: even if identical to the header > OPT_END() > }; > > diff --git a/pretty.c b/pretty.c > index 51e3fa5736..e266208c0b 100644 > --- a/pretty.c > +++ b/pretty.c > @@ -483,6 +483,8 @@ static int use_inbody_from(const struct pretty_print_context *pp, const struct i > return 0; > if (ident_cmp(pp->from_ident, ident)) > return 1; > + if (pp->rev && pp->rev->force_inbody_from) > + return 1; It would probably make sense to move this before `ident_cmp()`, to avoid unneeded calls ("is the ident the same? no? well, thank you for your answer but I'll insert the header anyway!"). > return 0; > } > > diff --git a/revision.h b/revision.h > index bb91e7ed91..a2d3813a21 100644 > --- a/revision.h > +++ b/revision.h > @@ -208,6 +208,7 @@ struct rev_info { > > /* Format info */ > int show_notes; > + unsigned int force_inbody_from; The reason why this isn't added to the `:1` bits below is probably the anticipation of the tri-state, but if that tri-state never materializes, adding it as a bit is still the right thing to do. > unsigned int shown_one:1, > shown_dashes:1, > show_merge:1, > diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh > index fbec8ad2ef..a4ecd433e2 100755 > --- a/t/t4014-format-patch.sh > +++ b/t/t4014-format-patch.sh > @@ -1400,6 +1400,19 @@ test_expect_success '--from omits redundant in-body header' ' > test_cmp expect patch.head > ' > > +test_expect_success 'with --force-inbody-from, --from keeps redundant in-body header' ' > + git format-patch --force-inbody-from \ > + -1 --stdout --from="A U Thor <author@example.com>" >patch && > + cat >expect <<-\EOF && > + From: A U Thor <author@example.com> > + > + From: A U Thor <author@example.com> > + > + EOF > + sed -ne "/^From:/p; /^$/p; /^---$/q" patch >patch.head && > + test_cmp expect patch.head > +' The test script starts to look a bit non-DRY with all those repetitions of `A U Thor <author@example.com>`, but that's hardly the responsibility of this here patch to address. Thank you, Dscho > + > test_expect_success 'in-body headers trigger content encoding' ' > test_env GIT_AUTHOR_NAME="éxötìc" test_commit exotic && > test_when_finished "git reset --hard HEAD^" && > -- > 2.37.2-587-g47adba97a9 > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] format-patch: allow forcing the use of in-body From: header 2022-08-29 11:48 ` Johannes Schindelin @ 2022-08-29 17:41 ` Junio C Hamano 0 siblings, 0 replies; 13+ messages in thread From: Junio C Hamano @ 2022-08-29 17:41 UTC (permalink / raw) To: Johannes Schindelin; +Cc: git Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> Note. This is an uncooked early draft. > > Did you mean to mark the patch as [RFC], then? It is mixture between that and WIP. >> Things to think about include (but not limited to, of course): >> >> * Should this rather be --use-inbody-from=yes,no,auto tristate, >> that defaults to "auto", which is the current behaviour i.e. >> "when --from is given, add it only when it does not match the >> payload". "yes" would mean "always emit the --from address as >> in-body From:" and "no" would mean ... what? "Ignore --from"? >> Then why is the user giving --from in the first place? > > I would offer up the suggestion `--in-body-from={never,always,auto}` for > consideration. That is essentially the same as the "Boolean plus auto" tristate, a very common pattern in our UI. The problem is that false-never-no does not make much sense in this case, because you do not need it. You can instead refrain from passing --from to achieve the same effect. >> * Should it be "inbody" or "in-body"? > > The latter. OK. This cascades up to 1/2 (there is a new helper function with the phrase in its name). >> * Should it have a corresponding configuration variable? > > Probably. The commit message talks about mailing lists requiring different > behavior from the default, which is likely to affect all patches generated > from a corresponding local checkout. Having a config variable would lower > the cognitive burden of having to remember this process detail. OK. >> diff --git a/builtin/log.c b/builtin/log.c >> index 9b937d59b8..83b2d01b49 100644 >> --- a/builtin/log.c >> +++ b/builtin/log.c >> @@ -1897,6 +1897,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) >> N_("show changes against <refspec> in cover letter or single patch")), >> OPT_INTEGER(0, "creation-factor", &creation_factor, >> N_("percentage by which creation is weighted")), >> + OPT_BOOL(0, "force-inbody-from", &rev.force_inbody_from, >> + N_("Use in-body From: even for your own commit")), > > Please start the usage text in lower-case, to keep it consistent with the > rest of the usage texts. Right. > Also, I would like to avoid the personal address "you" in that text, and > also the verb "use". Maybe something like this: > > show in-body From: even if identical to the header Much nicer. I'll take it. >> diff --git a/pretty.c b/pretty.c >> index 51e3fa5736..e266208c0b 100644 >> --- a/pretty.c >> +++ b/pretty.c >> @@ -483,6 +483,8 @@ static int use_inbody_from(const struct pretty_print_context *pp, const struct i >> return 0; >> if (ident_cmp(pp->from_ident, ident)) >> return 1; >> + if (pp->rev && pp->rev->force_inbody_from) >> + return 1; > > It would probably make sense to move this before `ident_cmp()`, to avoid > unneeded calls ("is the ident the same? no? well, thank you for your > answer but I'll insert the header anyway!"). I tend to prefer adding new things at the end when all things are equal, but in this case the new thing is an overriding condition, so it does make sense to have it before the call. >> diff --git a/revision.h b/revision.h >> index bb91e7ed91..a2d3813a21 100644 >> --- a/revision.h >> +++ b/revision.h >> @@ -208,6 +208,7 @@ struct rev_info { >> >> /* Format info */ >> int show_notes; >> + unsigned int force_inbody_from; > > The reason why this isn't added to the `:1` bits below is probably the > anticipation of the tri-state, but if that tri-state never materializes, > adding it as a bit is still the right thing to do. It might make sense to turn this into the common "Boolean plus auto" tristate, but the utility of "no" in this case is dubious, so I was not planning to go that route. This member is a full fledged word because the address of it given to OPT_BOOL(), and we cannot take an address of a bitfield member in a struct. Bit-pinching in this struct is not very useful. Even if we traverse a million commits in a single run, we will use a single "struct rev_info" instance. Thanks for reading it over. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 0/3] format-patch --force-in-body-from 2022-08-26 21:32 [PATCH 0/2] format-patch --force-inbody-from Junio C Hamano 2022-08-26 21:32 ` [PATCH 1/2] pretty: separate out the logic to decide the use of in-body from Junio C Hamano 2022-08-26 21:32 ` [PATCH 2/2] format-patch: allow forcing the use of in-body From: header Junio C Hamano @ 2022-08-29 21:38 ` Junio C Hamano 2022-08-29 21:38 ` [PATCH v2 1/3] pretty: separate out the logic to decide the use of in-body from Junio C Hamano ` (2 more replies) 2 siblings, 3 replies; 13+ messages in thread From: Junio C Hamano @ 2022-08-29 21:38 UTC (permalink / raw) To: git; +Cc: Konstantin Ryabitsev, Rasmus Villemoes, Johannes Schindelin Users may be authoring and committing their commits under the same e-mail address they use to send their patches from, in which case they shouldn't need to use the in-body From: line in their outgoing e-mails. At the receiving end, "git am" will use the address on the "From:" header of the incoming e-mail and all should be well. Some mailing lists, however, mangle the From: address from what the original sender had; in such an unfortunate situation, the user may want to add the in-body "From:" header even for their own patch. A new option `--force-in-body-from` from the command line of `git format-patch`, and a new configuration variable `format.forceInBodyFrom` that can be set per repository, are added to address this. Changes since the original submission are * The configuration variable support is new. * comes with documentation updates. * "in-body" is spelled as such, not "inbody". * the order of config and option parsing plus initializing the rev_info structure dictates a separate variable to be used for the former two and then copied to rev_info later. Junio C Hamano (3): pretty: separate out the logic to decide the use of in-body from format-patch: allow forcing the use of in-body From: header format-patch: learn format.forceInBodyFrom configuration variable Documentation/config/format.txt | 4 ++++ Documentation/git-format-patch.txt | 11 +++++++++ builtin/log.c | 9 ++++++++ pretty.c | 12 +++++++++- revision.h | 1 + t/t4014-format-patch.sh | 37 ++++++++++++++++++++++++++++++ 6 files changed, 73 insertions(+), 1 deletion(-) -- 2.37.2-621-gd3a800faf0 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 1/3] pretty: separate out the logic to decide the use of in-body from 2022-08-29 21:38 ` [PATCH v2 0/3] format-patch --force-in-body-from Junio C Hamano @ 2022-08-29 21:38 ` Junio C Hamano 2022-08-29 21:38 ` [PATCH v2 2/3] format-patch: allow forcing the use of in-body From: header Junio C Hamano 2022-08-29 21:38 ` [PATCH v2 3/3] format-patch: learn format.forceInBodyFrom configuration variable Junio C Hamano 2 siblings, 0 replies; 13+ messages in thread From: Junio C Hamano @ 2022-08-29 21:38 UTC (permalink / raw) To: git When pretty-printing the log message for a given commit in the e-mail format (e.g. "git format-patch"), we add an in-body "From:" header when the author identity of the commit is different from the identity of the person whose identity appears in the header of the e-mail (the latter is passed with them "--from" option). Split out the logic into a helper function, as we would want to extend the condition further. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- pretty.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/pretty.c b/pretty.c index 6d819103fb..cf418a6b20 100644 --- a/pretty.c +++ b/pretty.c @@ -477,6 +477,14 @@ static void append_line_with_color(struct strbuf *sb, struct grep_opt *opt, } } +static int use_in_body_from(const struct pretty_print_context *pp, + const struct ident_split *ident) +{ + 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 +511,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 (pp->from_ident && use_in_body_from(pp, &ident)) { struct strbuf buf = STRBUF_INIT; strbuf_addstr(&buf, "From: "); -- 2.37.2-621-gd3a800faf0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 2/3] format-patch: allow forcing the use of in-body From: header 2022-08-29 21:38 ` [PATCH v2 0/3] format-patch --force-in-body-from Junio C Hamano 2022-08-29 21:38 ` [PATCH v2 1/3] pretty: separate out the logic to decide the use of in-body from Junio C Hamano @ 2022-08-29 21:38 ` Junio C Hamano 2022-08-30 20:07 ` Jeff King 2022-08-29 21:38 ` [PATCH v2 3/3] format-patch: learn format.forceInBodyFrom configuration variable Junio C Hamano 2 siblings, 1 reply; 13+ messages in thread From: Junio C Hamano @ 2022-08-29 21:38 UTC (permalink / raw) To: git Users may be authoring and committing their commits under the same e-mail address they use to send their patches from, in which case they shouldn't need to use the in-body From: line in their outgoing e-mails. At the receiving end, "git am" will use the address on the "From:" header of the incoming e-mail and all should be well. Some mailing lists, however, mangle the From: address from what the original sender had; in such a situation, the user may want to add the in-body "From:" header even for their own patches. "git format-patch --[no-]force-in-body-from" was invented for such users. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- Documentation/git-format-patch.txt | 9 +++++++++ builtin/log.c | 5 +++++ pretty.c | 2 ++ revision.h | 1 + t/t4014-format-patch.sh | 13 +++++++++++++ 5 files changed, 30 insertions(+) diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt index be797d7a28..7c7f244e57 100644 --- a/Documentation/git-format-patch.txt +++ b/Documentation/git-format-patch.txt @@ -275,6 +275,15 @@ header). Note also that `git send-email` already handles this transformation for you, and this option should not be used if you are feeding the result to `git send-email`. +--[no-]force-in-body-from:: + With the e-mail sender specified via the `--from` option, by + default, an in-body "From:" to identify the real author of + the commit is added at the top of the commit log message if + the sender is different from the author. With this option, + the in-body "From:" is added even when the sender and the + author have the same name and address, which may help if the + mailing list software mangles the sender's identity. + --add-header=<header>:: Add an arbitrary header to the email headers. This is in addition to any configured headers, and may be used multiple times. diff --git a/builtin/log.c b/builtin/log.c index 9b937d59b8..78ccd37bd9 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -52,6 +52,7 @@ static int default_encode_email_headers = 1; static int decoration_style; static int decoration_given; static int use_mailmap_config = 1; +static unsigned int force_in_body_from; static const char *fmt_patch_subject_prefix = "PATCH"; static int fmt_patch_name_max = FORMAT_PATCH_NAME_MAX_DEFAULT; static const char *fmt_pretty; @@ -1897,6 +1898,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) N_("show changes against <refspec> in cover letter or single patch")), OPT_INTEGER(0, "creation-factor", &creation_factor, N_("percentage by which creation is weighted")), + OPT_BOOL(0, "force-in-body-from", &force_in_body_from, + N_("show in-body From: even if identical to the e-mail header")), OPT_END() }; @@ -1940,6 +1943,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN | PARSE_OPT_KEEP_DASHDASH); + rev.force_in_body_from = force_in_body_from; + /* Make sure "0000-$sub.patch" gives non-negative length for $sub */ if (fmt_patch_name_max <= strlen("0000-") + strlen(fmt_patch_suffix)) fmt_patch_name_max = strlen("0000-") + strlen(fmt_patch_suffix); diff --git a/pretty.c b/pretty.c index cf418a6b20..b7553e3fe0 100644 --- a/pretty.c +++ b/pretty.c @@ -480,6 +480,8 @@ static void append_line_with_color(struct strbuf *sb, struct grep_opt *opt, static int use_in_body_from(const struct pretty_print_context *pp, const struct ident_split *ident) { + if (pp->rev && pp->rev->force_in_body_from) + return 1; if (ident_cmp(pp->from_ident, ident)) return 1; return 0; diff --git a/revision.h b/revision.h index bb91e7ed91..6e346a60ab 100644 --- a/revision.h +++ b/revision.h @@ -221,6 +221,7 @@ struct rev_info { missing_newline:1, date_mode_explicit:1, preserve_subject:1, + force_in_body_from:1, encode_email_headers:1, include_header:1; unsigned int disable_stdin:1; diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh index fbec8ad2ef..347f7f7f35 100755 --- a/t/t4014-format-patch.sh +++ b/t/t4014-format-patch.sh @@ -1400,6 +1400,19 @@ test_expect_success '--from omits redundant in-body header' ' test_cmp expect patch.head ' +test_expect_success 'with --force-in-body-from, redundant in-body from is kept' ' + git format-patch --force-in-body-from \ + -1 --stdout --from="A U Thor <author@example.com>" >patch && + cat >expect <<-\EOF && + From: A U Thor <author@example.com> + + From: A U Thor <author@example.com> + + EOF + sed -ne "/^From:/p; /^$/p; /^---$/q" patch >patch.head && + test_cmp expect patch.head +' + test_expect_success 'in-body headers trigger content encoding' ' test_env GIT_AUTHOR_NAME="éxötìc" test_commit exotic && test_when_finished "git reset --hard HEAD^" && -- 2.37.2-621-gd3a800faf0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/3] format-patch: allow forcing the use of in-body From: header 2022-08-29 21:38 ` [PATCH v2 2/3] format-patch: allow forcing the use of in-body From: header Junio C Hamano @ 2022-08-30 20:07 ` Jeff King 2022-08-30 20:14 ` Jeff King 0 siblings, 1 reply; 13+ messages in thread From: Jeff King @ 2022-08-30 20:07 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Mon, Aug 29, 2022 at 02:38:36PM -0700, Junio C Hamano wrote: > +--[no-]force-in-body-from:: > + With the e-mail sender specified via the `--from` option, by > + default, an in-body "From:" to identify the real author of > + the commit is added at the top of the commit log message if > + the sender is different from the author. With this option, > + the in-body "From:" is added even when the sender and the > + author have the same name and address, which may help if the > + mailing list software mangles the sender's identity. I find it a little curious that this option can only be used with "--from". That makes sense in a way, because this is a special case of that feature, overriding the "are they the same" check. But given that the use case is not to send somebody else's patch, but to duplicate your _own_ ident in both spots, it feels funny that you must also say "by the way, I am the sender of the email". I.e., you have to say: git format-patch --from='Me <me@example.com>' --force-in-body-from I guess it is not too bad because just "--from" will do the equivalent thing (picking "me" from your committer ident). It just feels kind of clunky that: git format-patch --force-in-body-from will silently ignore the option. All that said, I don't care _too_ strongly about it. I do suspect the feature might be better placed in send-email (or possibly in addition). If you are using send-email, then I think you're not supposed to use "--from" with format-patch at all, and you have no way of accessing this feature. -Peff ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/3] format-patch: allow forcing the use of in-body From: header 2022-08-30 20:07 ` Jeff King @ 2022-08-30 20:14 ` Jeff King 0 siblings, 0 replies; 13+ messages in thread From: Jeff King @ 2022-08-30 20:14 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Tue, Aug 30, 2022 at 04:07:18PM -0400, Jeff King wrote: > All that said, I don't care _too_ strongly about it. I do suspect the > feature might be better placed in send-email (or possibly in addition). > If you are using send-email, then I think you're not supposed to use > "--from" with format-patch at all, and you have no way of accessing this > feature. This "not supposed to" came from me looking at the documentation. But this discussion implies that it's maybe not that big a deal: https://lore.kernel.org/git/xmqq8twlqwan.fsf@gitster.mtv.corp.google.com/ Apparently we were considering enabling "--from" by default, but I don't think that ever happened. -Peff ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 3/3] format-patch: learn format.forceInBodyFrom configuration variable 2022-08-29 21:38 ` [PATCH v2 0/3] format-patch --force-in-body-from Junio C Hamano 2022-08-29 21:38 ` [PATCH v2 1/3] pretty: separate out the logic to decide the use of in-body from Junio C Hamano 2022-08-29 21:38 ` [PATCH v2 2/3] format-patch: allow forcing the use of in-body From: header Junio C Hamano @ 2022-08-29 21:38 ` Junio C Hamano 2 siblings, 0 replies; 13+ messages in thread From: Junio C Hamano @ 2022-08-29 21:38 UTC (permalink / raw) To: git As the need to use the "--force-in-body-from" option primarily is tied to which mailing list the mails go to (and get their From: address mangled), it is likely that a user who needs to use this option once to interact with their upstream project needs to use it for all patches they send out. Add a configuration variable, suitable for setting in the local configuration file per repository, for this. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- Documentation/config/format.txt | 4 ++++ Documentation/git-format-patch.txt | 2 ++ builtin/log.c | 4 ++++ t/t4014-format-patch.sh | 24 ++++++++++++++++++++++++ 4 files changed, 34 insertions(+) diff --git a/Documentation/config/format.txt b/Documentation/config/format.txt index fdbc06a4d2..c7303d8d9f 100644 --- a/Documentation/config/format.txt +++ b/Documentation/config/format.txt @@ -15,6 +15,10 @@ format.from:: different. If set to a non-boolean value, format-patch uses that value instead of your committer identity. Defaults to false. +format.forceInBodyFrom:: + Provides the default value for the `--[no-]force-in-body-from` + option to format-patch. Defaults to false. + format.numbered:: A boolean which can enable or disable sequence numbers in patch subjects. It defaults to "auto" which enables it only if there diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt index 7c7f244e57..dfcc7da4c2 100644 --- a/Documentation/git-format-patch.txt +++ b/Documentation/git-format-patch.txt @@ -283,6 +283,8 @@ feeding the result to `git send-email`. the in-body "From:" is added even when the sender and the author have the same name and address, which may help if the mailing list software mangles the sender's identity. + Defaults to the value of the `format.forceInBodyFrom` + configuration variable. --add-header=<header>:: Add an arbitrary header to the email headers. This is in addition diff --git a/builtin/log.c b/builtin/log.c index 78ccd37bd9..776bc9afdb 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -1007,6 +1007,10 @@ static int git_format_config(const char *var, const char *value, void *cb) from = NULL; return 0; } + if (!strcmp(var, "format.forceinbodyfrom")) { + force_in_body_from = git_config_bool(var, value); + return 0; + } if (!strcmp(var, "format.notes")) { int b = git_parse_maybe_bool(value); if (b < 0) diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh index 347f7f7f35..ad5c029279 100755 --- a/t/t4014-format-patch.sh +++ b/t/t4014-format-patch.sh @@ -1413,6 +1413,30 @@ test_expect_success 'with --force-in-body-from, redundant in-body from is kept' test_cmp expect patch.head ' +test_expect_success 'format.forceInBodyFrom, equivalent to --force-in-body-from' ' + git -c format.forceInBodyFrom=yes format-patch \ + -1 --stdout --from="A U Thor <author@example.com>" >patch && + cat >expect <<-\EOF && + From: A U Thor <author@example.com> + + From: A U Thor <author@example.com> + + EOF + sed -ne "/^From:/p; /^$/p; /^---$/q" patch >patch.head && + test_cmp expect patch.head +' + +test_expect_success 'format.forceInBodyFrom, equivalent to --force-in-body-from' ' + git -c format.forceInBodyFrom=yes format-patch --no-force-in-body-from \ + -1 --stdout --from="A U Thor <author@example.com>" >patch && + cat >expect <<-\EOF && + From: A U Thor <author@example.com> + + EOF + sed -ne "/^From:/p; /^$/p; /^---$/q" patch >patch.head && + test_cmp expect patch.head +' + test_expect_success 'in-body headers trigger content encoding' ' test_env GIT_AUTHOR_NAME="éxötìc" test_commit exotic && test_when_finished "git reset --hard HEAD^" && -- 2.37.2-621-gd3a800faf0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-08-30 20:14 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-08-26 21:32 [PATCH 0/2] format-patch --force-inbody-from Junio C Hamano 2022-08-26 21:32 ` [PATCH 1/2] pretty: separate out the logic to decide the use of in-body from Junio C Hamano 2022-08-29 11:32 ` Johannes Schindelin 2022-08-29 17:29 ` Junio C Hamano 2022-08-26 21:32 ` [PATCH 2/2] format-patch: allow forcing the use of in-body From: header Junio C Hamano 2022-08-29 11:48 ` Johannes Schindelin 2022-08-29 17:41 ` Junio C Hamano 2022-08-29 21:38 ` [PATCH v2 0/3] format-patch --force-in-body-from Junio C Hamano 2022-08-29 21:38 ` [PATCH v2 1/3] pretty: separate out the logic to decide the use of in-body from Junio C Hamano 2022-08-29 21:38 ` [PATCH v2 2/3] format-patch: allow forcing the use of in-body From: header Junio C Hamano 2022-08-30 20:07 ` Jeff King 2022-08-30 20:14 ` Jeff King 2022-08-29 21:38 ` [PATCH v2 3/3] format-patch: learn format.forceInBodyFrom configuration variable Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).