Re: [PATCH 2/2] send-email: add --header-cmd option

From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
Date: Apr 25, 2023, 18:53 UTC
Message-ID: <87leifpzco.fsf@gmail.com>
In-Reply-To: <xmqqcz3s3oz7.fsf@gitster.g>

Hi Junio,

Junio C Hamano <gitster@pobox.com> writes:

Show 11 quoted lines

> Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:
>
>> I'm not too familiar with the email format; but I presume an empty line
>> would signal the end of a message?  That'd be bad yes, but I think it
>> cannot currently happen given the 'last if $line =~ /^$/;' guard at in
>> execute_cmd around line 2023; it'd means headers following an empty line
>> would be discarded though.  The expected use case is indeed for a user's
>> script to produce RFC 2822 style headers to messages.
>
> Yes, silently discarding the end-user input is what I meant by a
> disaster.

In v3 just sent, empty blank lines are now detected and reported as an error.

Show 15 quoted lines

>> The former (a true default with no way to turn it off other than
>> redefining it), which I believe is the same behavior as for --cc-cmd or
>> --to-cmd.  There are no '--no-cc-cmd' or '--no-to-cmd' options, although
>> their result can be filtered via the '--no-cc' and '--no-to' options.
>
> Yup.
>
>> Looking in the source, options supporting '--no-' always appear to be
>> boolean toggles (on/off) though, so I'm not sure how a '--no-header-cmd'
>> that take a value can currently be implemented.  Perhaps it could be
>> added later if there is a need?
>
> Perhaps we can do without a configuration variable first, and
> perhaps the variable could be added later if there is a need and a
> proper way to turn it off per invocation basis.

I'd like to preserve the sendemail.headerCmd configuration variable; as it properly enables the use case that motivated this change :-). If it means I need to add some ad-hoc --no-header-cmd, I can do so; let me know!

>> I've extracted such postprocessing into fold_headers and applied
>> execute_cmd to it in new invoke_header_cmd subroutine.
>
> Sounds like a good approach (without looking the actual patch).

OK.  Make sure to look at the latest revision, v3.

Thanks again!

-- 
Maxim