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

[Junio C Hamano <gitster@pobox.com>]

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.