From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: "Strawbridge, Michael" <Michael.Strawbridge@amd.com>
Cc: "git@vger.kernel.org" <git@vger.kernel.org>,
"Tuikov, Luben" <Luben.Tuikov@amd.com>,
Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH v5 2/2] send-email: expose header information to git-send-email's sendemail-validate hook
Date: Tue, 17 Jan 2023 14:23:50 +0100 [thread overview]
Message-ID: <230117.86sfg9xp98.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <20230110211452.2568535-3-michael.strawbridge@amd.com>
On Tue, Jan 10 2023, Strawbridge, Michael wrote:
> To allow further flexibility in the git hook, the SMTP header
> information of the email that git-send-email intends to send, is now
> passed as a 2nd argument to the sendemail-validate hook.
>
> As an example, this can be useful for acting upon keywords in the
> subject or specific email addresses.
Okey, but...
> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> index a16e62bc8c..2b5c6640cc 100644
> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -583,10 +583,19 @@ processed by rebase.
> sendemail-validate
> ~~~~~~~~~~~~~~~~~~
>
> -This hook is invoked by linkgit:git-send-email[1]. It takes a single parameter,
> -the name of the file that holds the e-mail to be sent. Exiting with a
> -non-zero status causes `git send-email` to abort before sending any
> -e-mails.
> +This hook is invoked by linkgit:git-send-email[1].
> +
> +It takes these command line arguments:
> +1. the name of the file that holds the e-mail to be sent.
> +2. the name of the file that holds the SMTP headers to be used.
> +
> +The hook doesn't need to support multiple header names (for example only Cc
> +is passed). However, it does need to understand that lines beginning with
> +whitespace belong to the previous header. The header information follows
> +the same format as the confirmation given at the end of send-email.
> +
> +Exiting with a non-zero status causes `git send-email` to abort
> +before sending any e-mails.
>
> fsmonitor-watchman
> ~~~~~~~~~~~~~~~~~~
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 810dd1f1ce..b2adca515e 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -787,14 +787,6 @@ sub is_format_patch_arg {
>
> @files = handle_backup_files(@files);
>
> -if ($validate) {
> - foreach my $f (@files) {
> - unless (-p $f) {
> - validate_patch($f, $target_xfer_encoding);
> - }
> - }
> -}
> -
> if (@files) {
> unless ($quiet) {
> print $_,"\n" for (@files);
> @@ -1738,6 +1730,16 @@ sub send_message {
> return 1;
> }
>
> +if ($validate) {
> + foreach my $f (@files) {
> + unless (-p $f) {
> + pre_process_file($f, 1);
> +
> + validate_patch($f, $target_xfer_encoding);
> + }
> + }
> +}
...here we have the seemingly unrelated change of first doing the
validation before this, and if we pass it we'll print the names of the
files we're sending unless --quiet.
Now we'll do it the other way around, maybe that's good, or maybe not,
but your updated docs don't say.
Also (and I didn't look at this all that carefully), why are you moving
the control logic to between the later function declarations?
Perl isn't a language where you need to arrange your source in that way
(unless a bareword is involved, or if this happens at BEGIN time or
whatever). The current structure is:
<use & imports>
<basic setup (getopts etc)>
<main logic>
<helper function>
Here you're moving part of the main logic to in-between two helper
function, why?
> $in_reply_to = $initial_in_reply_to;
> $references = $initial_in_reply_to || '';
> $message_num = 0;
> @@ -2101,11 +2103,20 @@ sub validate_patch {
> chdir($repo->wc_path() or $repo->repo_path())
> or die("chdir: $!");
> local $ENV{"GIT_DIR"} = $repo->repo_path();
> +
> + my ($recipients_ref, $to, $date, $gitversion, $cc, $ccline, $header) = gen_header();
> +
> + require File::Temp;
> + my ($header_filehandle, $header_filename) = File::Temp::tempfile(
> + ".gitsendemail.header.XXXXXX", DIR => $repo->repo_path());
> + print $header_filehandle $header;
> +
> my @cmd = ("git", "hook", "run", "--ignore-missing",
> $hook_name, "--");
> - my @cmd_msg = (@cmd, "<patch>");
> - my @cmd_run = (@cmd, $target);
> + my @cmd_msg = (@cmd, "<patch>", "<header>");
> + my @cmd_run = (@cmd, $target, $header_filename);
> $hook_error = system_or_msg(\@cmd_run, undef, "@cmd_msg");
> + unlink($header_filehandle);
> chdir($cwd_save) or die("chdir: $!");
I know "git hook run" doesn't support input on stdin yet, but isn't this
just working around it not supporting that? That seems like a much
better & natural interface than what we're doing here.
I have out-of-tree patches for that (or rather, a re-roll of Emily's
patches to do that), if that landed in-tree could this use that
interface, do you think?
I'd rather that we didn't forever codify a strange interface here due to
a temporary limitation in "git hook" and the hook API...
next prev parent reply other threads:[~2023-01-17 13:30 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-10 21:16 [PATCH v5 0/2] send-email: expose header information to git-send-email's sendemail-validate hook Strawbridge, Michael
2023-01-10 21:16 ` [PATCH v5 1/2] send-email: refactor header generation functions Strawbridge, Michael
2023-01-17 13:20 ` Ævar Arnfjörð Bjarmason
2023-01-17 15:13 ` Junio C Hamano
2023-01-17 21:36 ` Strawbridge, Michael
2023-01-10 21:16 ` [PATCH v5 2/2] send-email: expose header information to git-send-email's sendemail-validate hook Strawbridge, Michael
2023-01-14 1:17 ` Junio C Hamano
2023-01-14 16:03 ` Junio C Hamano
2023-01-14 16:06 ` Junio C Hamano
2023-01-15 3:34 ` Junio C Hamano
2023-01-17 4:09 ` Luben Tuikov
2023-01-17 4:29 ` Junio C Hamano
2023-01-17 4:56 ` Luben Tuikov
2023-01-17 13:23 ` Ævar Arnfjörð Bjarmason [this message]
2023-01-17 21:58 ` Strawbridge, Michael
2023-01-17 1:49 ` [PATCH v5 0/2] " Strawbridge, Michael
2023-01-17 1:37 Strawbridge, Michael
2023-01-17 1:37 ` [PATCH v5 2/2] " Strawbridge, Michael
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=230117.86sfg9xp98.gmgdl@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=Luben.Tuikov@amd.com \
--cc=Michael.Strawbridge@amd.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).