| Message ID | 20230110211452.2568535-2-michael.strawbridge@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | send-email: expose header information to git-send-email's sendemail-validate hook | expand |
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... > + 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: > [...] > +} > + > +# 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"
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> +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... > ... >> +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: > ... Thanks as usual for a careful review, with concrete suggestions for improvements.
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!
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(); + 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) = @_; open my $fh, "<", $t or die sprintf(__("can't open file %s"), $t); @@ -1893,9 +1897,9 @@ sub process_file { } close $fh; - push @to, recipients_cmd("to-cmd", "to", $to_cmd, $t) + push @to, recipients_cmd("to-cmd", "to", $to_cmd, $t, $quiet) if defined $to_cmd; - push @cc, recipients_cmd("cc-cmd", "cc", $cc_cmd, $t) + push @cc, recipients_cmd("cc-cmd", "cc", $cc_cmd, $t, $quiet) if defined $cc_cmd && !$suppress_cc{'cccmd'}; if ($broken_encoding{$t} && !$has_content_type) { @@ -1954,6 +1958,15 @@ sub process_file { @initial_to = @to; } } +} + +# 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"
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(-)