git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Jani Nikula <jani.nikula@intel.com>
Cc: git@vger.kernel.org
Subject: Re: BUG: sendemail-validate hook is run too early
Date: Thu, 02 Jan 2020 12:26:10 -0800	[thread overview]
Message-ID: <xmqqk169ehb1.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <875zhut5yd.fsf@intel.com> (Jani Nikula's message of "Thu, 02 Jan 2020 14:10:02 +0200")

Jani Nikula <jani.nikula@intel.com> writes:

> I'm trying to use the sendemail-validate hook to validate the recipients
> of the patch email, among other things. Turns out the hook gets run
> immediately on the input patches, *not* on the "e-mail to be sent" as
> claimed by githooks(5).

I will make two suggestions, so please do not react before reading
both ;-)

The purpose of the validate hook, at least as it was originally
designed, was to vet the log message and patch contents, so what you
reported is not at all surprising.  After all, the sub that uses the
hook is called "validate_patch" ;-).

A low-hanging documentation fix (this is one suggestion) is to
phrase "e-mail to be sent" as "e-mail that has been submitted (to
git-send-email)" to avoid the confusion.

You do not want to use the sendemail-validate hook for checking for
the recipients, because the e-mail message is not a good source of
that information.

When a recipient is added, two things happen:

 * The recipient is added to the (internal) list of recipients on
   the underlying sendmail command line arguments.  This is the list
   of addresses that actually matter where the piece of email goes.

 * The recipient is added to the text of the message being sent, if
   s/he is being added to either To: or Cc: (this is purely for
   human consumption and does not affect where the piece of email
   goes).  A blind-carbon-copy recipient would not be added for
   obvious reasons.

If you truly want to validate where the message goes, you'd need to
vet the former list, not the latter one.  Otherwise, you'll miss BCC
recipients.

So the other suggestion is to have a separate hook to validate the
list of recipients.  This might be a bit more involved if we want to
execute cleanly, but should not be rocket science.  

The send_message() sub prepares @recipients list to form quite a bit
of processing at the beginning, and the uses the resulting list to
drive the sendmail by adding it to @sendmail_parameters().  The
contents of this @recipients list is what you want to vet before the
code talks to the sendmail program or daemon later in the function.

  reply	other threads:[~2020-01-02 20:26 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-02 12:10 BUG: sendemail-validate hook is run too early Jani Nikula
2020-01-02 20:26 ` Junio C Hamano [this message]
2020-01-03  9:46   ` Jani Nikula
2020-01-03 14:02     ` Jani Nikula

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=xmqqk169ehb1.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=jani.nikula@intel.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).