Re: [PATCH v3 17/34] mailinfo: move use_scissors and use_inbody_headers to struct mailinfo

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 21, 2015, 21:02 UTC
Message-ID: <xmqq8u6wue91.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAGZ79kZkms9SS3YpyEK4tmUPZ3TwXRF1M30X58GrS8dV13rJng@mail.gmail.com>

Stefan Beller <sbeller@google.com> writes:

Show 20 quoted lines

> On Mon, Oct 19, 2015 at 12:28 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>>  builtin/mailinfo.c | 23 +++++++++++++----------
>>  1 file changed, 13 insertions(+), 10 deletions(-)
>>
>> diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c
>> index c0522f2..2c8f249 100644
>> --- a/builtin/mailinfo.c
>> +++ b/builtin/mailinfo.c
>> @@ -20,6 +20,8 @@ struct mailinfo {
>>         int keep_subject;
>>         int keep_non_patch_brackets_in_subject;
>>         int add_message_id;
>> +       int use_scissors;
>> +       int use_inbody_headers; /* defaults to 1 */
>
> IMHO there is no need for the comment here, stating its default.
> That can be looked up in the init function, which is as convenient as
> reading globals in a file?

It was crucial to ensure correctness during conversion, that is, a conversion done in

 (1) Remove "static int use_inbody_headers = 1"
 (2) Add "int use_inbody_headers;"
 (3) Update setup_mailinfo()

sequence would have lost a clue as to what the right value to assign in step (3). Tweaking (2) to leave "/* this starts at 1 */" helped avoid such a mistake.

When reading the end result, the above three appear to have been done simultaneously, so I agree that the comment does not help.