Re: [PATCH 2/2] format-patch: allow forcing the use of in-body From: header

Johannes Schindelin [off-list ref] writes:

quoted

Note.  This is an uncooked early draft.

Did you mean to mark the patch as [RFC], then?

It is mixture between that and WIP.

quoted

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.

quoted

 * 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).

quoted

 * 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.

quoted

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.

quoted

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.

quoted

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.