[PATCH v5 1/2] send-email: refactor header generation functions

["Strawbridge, Michael" <Michael.Strawbridge@amd.com>]

On Tue, Jan 17 2023, Ævar Arnfjörð Bjarmason wrote:
>On Tue, Jan 10 2023, Strawbridge, Michael wrote:
>
>> Split process_file and send_message into easier to use functions.
>> Making SMTP header information more widely available.
>>
>> Cc: Luben Tuikov <luben.tuikov@amd.com>
>> Cc: Junio C Hamano <gitster@pobox.com>
>> Signed-off-by: Michael Strawbridge <michael.strawbridge@amd.com>
>> ---
>>  git-send-email.perl | 49 ++++++++++++++++++++++++++++-----------------
>>  1 file changed, 31 insertions(+), 18 deletions(-)
>>
>> diff --git a/git-send-email.perl b/git-send-email.perl
>> index 5861e99a6e..810dd1f1ce 100755
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -1495,16 +1495,7 @@ sub file_name_is_absolute {
>>  	return File::Spec::Functions::file_name_is_absolute($path);
>>  }
>>  
>> -# Prepares the email, then asks the user what to do.
>> -#
>> -# If the user chooses to send the email, it's sent and 1 is returned.
>> -# If the user chooses not to send the email, 0 is returned.
>> -# If the user decides they want to make further edits, -1 is returned and the
>> -# caller is expected to call send_message again after the edits are performed.
>> -#
>> -# If an error occurs sending the email, this just dies.
>> -
>> -sub send_message {
>> +sub gen_header {
>>  	my @recipients = unique_email_list(@to);
>>  	@cc = (grep { my $cc = extract_valid_address_or_die($_);
>>  		      not grep { $cc eq $_ || $_ =~ /<\Q${cc}\E>$/ } @recipients
>> @@ -1546,6 +1537,22 @@ sub send_message {
>>  	if (@xh) {
>>  		$header .= join("\n", @xh) . "\n";
>>  	}
>> +	my $recipients_ref = \@recipients;
>> +	return ($recipients_ref, $to, $date, $gitversion, $cc, $ccline, $header);
>> +}
>> +
>> +# Prepares the email, then asks the user what to do.
>> +#
>> +# If the user chooses to send the email, it's sent and 1 is returned.
>> +# If the user chooses not to send the email, 0 is returned.
>> +# If the user decides they want to make further edits, -1 is returned and the
>> +# caller is expected to call send_message again after the edits are performed.
>> +#
>> +# If an error occurs sending the email, this just dies.
>> +
>> +sub send_message {
>> +	my ($recipients_ref, $to, $date, $gitversion, $cc, $ccline, $header) = gen_header();
>
>This makes the diff smaller, but if we're refactoring these functions to
>return arguments it's probably better to return a hash reference rather
>than remembering all the parameter names.
>
>But in this case it's probably fine...
>
I hadn't known about passing a hash reference on perl.  Although after
looking into it, I'm not sure I will go that way this time.

>> +	my @recipients = @$recipients_ref;
>>  
>>  	my @sendmail_parameters = ('-i', @recipients);
>>  	my $raw_from = $sender;
>> @@ -1735,11 +1742,8 @@ sub send_message {
>>  $references = $initial_in_reply_to || '';
>>  $message_num = 0;
>>  
>> -# Prepares the email, prompts the user, sends it out
>> -# Returns 0 if an edit was done and the function should be called again, or 1
>> -# otherwise.
>> -sub process_file {
>> -	my ($t) = @_;
>> +sub pre_process_file {
>> +	my ($t, $quiet) = @_;
>
>This, I think, is an anti-pattern in this file. We can just read the
>"$quiet" and other things that we set up when we parse the parameters as
>globals, we don't need to pass them as named parameters.
>
>It doesn't help readability to shadow that variable with another lexical
>here below:
>
I am open to guidance here.  The reason I included quiet as an argument
was because I wanted to override the global quiet value for the call of
pre_process_file from the validation section (line 1736).  This is needed
otherwise the user gets spammed with double print statements.  Once in
the validation loop and another time in the actual sending message part.

The paths forward that I can currently see are:
1) a) remain as it is
   b) rename quiet in pre_process_file but keep the logic the same
2) in the validation section, temporarily save the quiet value, alter it to
indicate quieting of the print statements, and then put the quiet value back
3) separate out the print statements inside pre_prcoess_file into a different
function and only call that function in the send message code.

>> [...]
>> +}
>> +
>> +# Prepares the email, prompts the user, sends it out
>> +# Returns 0 if an edit was done and the function should be called again, or 1
>> +# otherwise.
>> +sub process_file {
>> +	my ($t) = @_;
>> +
>> +        pre_process_file($t, $quiet);
>>  
>>  	my $message_was_sent = send_message();
>>  	if ($message_was_sent == -1) {
>> @@ -2002,7 +2015,7 @@ sub process_file {
>>  # Execute a command (e.g. $to_cmd) to get a list of email addresses
>>  # and return a results array
>>  sub recipients_cmd {
>> -	my ($prefix, $what, $cmd, $file) = @_;
>> +	my ($prefix, $what, $cmd, $file, $quiet) = @_;
>>  
>>  	my @addresses = ();
>>  	open my $fh, "-|", "$cmd \Q$file\E"

I appreciate your feedback!

-- 
2.34.1