| Message ID | 20220826213203.3258022-3-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: > 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 > >
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.
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^" &&
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(+)