{"thread":{"id":"52550","subject":"BUG: sendemail-validate hook is run too early","startedAt":"2020-01-02T12:10:08Z","lastAt":"2020-01-03T14:02:19Z","messageCount":4,"participants":["Jani Nikula","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"389162","messageId":"875zhut5yd.fsf@intel.com","threadId":"52550","inReplyTo":null,"subject":"BUG: sendemail-validate hook is run too early","fromName":"Jani Nikula","fromEmail":"jani.nikula@intel.com","sentAt":"2020-01-02T12:10:02Z","receivedAt":"2020-01-02T12:10:08Z","isPatch":false,"sender":{"key":"jani.nikula@intel.com","avatar":null},"body":"\nI'm trying to use the sendemail-validate hook to validate the recipients\nof the patch email, among other things. Turns out the hook gets run\nimmediately on the input patches, *not* on the \"e-mail to be sent\" as\nclaimed by githooks(5).\n\nThis means the recipients added by git send-email automatically or on\nthe git send-email command-line, or any changes done by the user with\n--annotate will not be validated.\n\nThis is easy to demonstrate in a git repo with e.g.\n\n$ ln -s /bin/cat .git/hooks/sendemail-validate\n$ git send-email --dry-run -1 --to bypass-validation@example.com\n\nThe file passed to the validate hook does not have the address.\n\nIf changing the location of the current validation hook seems too risky,\nas apparently it's been like this for more than a decade, I suggest\nadding another hook on the actual email to be sent.\n\n\nBR,\nJani.\n\n-- \nJani Nikula, Intel Open Source Graphics Center\n"},{"id":"389177","messageId":"xmqqk169ehb1.fsf@gitster-ct.c.googlers.com","threadId":"52550","inReplyTo":"875zhut5yd.fsf@intel.com","subject":"Re: BUG: sendemail-validate hook is run too early","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-02T20:26:10Z","receivedAt":"2020-01-02T20:26:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jani Nikula writes:\n\n> I'm trying to use the sendemail-validate hook to validate the recipients\n> of the patch email, among other things. Turns out the hook gets run\n> immediately on the input patches, *not* on the \"e-mail to be sent\" as\n> claimed by githooks(5).\n\nI will make two suggestions, so please do not react before reading\nboth ;-)\n\nThe purpose of the validate hook, at least as it was originally\ndesigned, was to vet the log message and patch contents, so what you\nreported is not at all surprising. After all, the sub that uses the\nhook is called \"validate_patch\" ;-).\n\nA low-hanging documentation fix (this is one suggestion) is to\nphrase \"e-mail to be sent\" as \"e-mail that has been submitted (to\ngit-send-email)\" to avoid the confusion.\n\nYou do not want to use the sendemail-validate hook for checking for\nthe recipients, because the e-mail message is not a good source of\nthat information.\n\nWhen a recipient is added, two things happen:\n\n * The recipient is added to the (internal) list of recipients on\n the underlying sendmail command line arguments. This is the list\n of addresses that actually matter where the piece of email goes.\n\n * The recipient is added to the text of the message being sent, if\n s/he is being added to either To: or Cc: (this is purely for\n human consumption and does not affect where the piece of email\n goes). A blind-carbon-copy recipient would not be added for\n obvious reasons.\n\nIf you truly want to validate where the message goes, you'd need to\nvet the former list, not the latter one. Otherwise, you'll miss BCC\nrecipients.\n\nSo the other suggestion is to have a separate hook to validate the\nlist of recipients. This might be a bit more involved if we want to\nexecute cleanly, but should not be rocket science. \n\nThe send_message() sub prepares @recipients list to form quite a bit\nof processing at the beginning, and the uses the resulting list to\ndrive the sendmail by adding it to @sendmail_parameters(). The\ncontents of this @recipients list is what you want to vet before the\ncode talks to the sendmail program or daemon later in the function.\n"},{"id":"389190","messageId":"87sgkwswhs.fsf@intel.com","threadId":"52550","inReplyTo":"xmqqk169ehb1.fsf@gitster-ct.c.googlers.com","subject":"Re: BUG: sendemail-validate hook is run too early","fromName":"Jani Nikula","fromEmail":"jani.nikula@intel.com","sentAt":"2020-01-03T09:46:39Z","receivedAt":"2020-01-03T09:46:45Z","isPatch":false,"sender":{"key":"jani.nikula@intel.com","avatar":null},"body":"On Thu, 02 Jan 2020, Junio C Hamano wrote:\n> Jani Nikula writes:\n>\n>> I'm trying to use the sendemail-validate hook to validate the recipients\n>> of the patch email, among other things. Turns out the hook gets run\n>> immediately on the input patches, *not* on the \"e-mail to be sent\" as\n>> claimed by githooks(5).\n>\n> I will make two suggestions, so please do not react before reading\n> both ;-)\n>\n> The purpose of the validate hook, at least as it was originally\n> designed, was to vet the log message and patch contents, so what you\n> reported is not at all surprising. After all, the sub that uses the\n> hook is called \"validate_patch\" ;-).\n>\n> A low-hanging documentation fix (this is one suggestion) is to\n> phrase \"e-mail to be sent\" as \"e-mail that has been submitted (to\n> git-send-email)\" to avoid the confusion.\n\nAgreed; I think this should be done no matter what.\n\n> You do not want to use the sendemail-validate hook for checking for\n> the recipients, because the e-mail message is not a good source of\n> that information.\n>\n> When a recipient is added, two things happen:\n>\n> * The recipient is added to the (internal) list of recipients on\n> the underlying sendmail command line arguments. This is the list\n> of addresses that actually matter where the piece of email goes.\n>\n> * The recipient is added to the text of the message being sent, if\n> s/he is being added to either To: or Cc: (this is purely for\n> human consumption and does not affect where the piece of email\n> goes). A blind-carbon-copy recipient would not be added for\n> obvious reasons.\n>\n> If you truly want to validate where the message goes, you'd need to\n> vet the former list, not the latter one. Otherwise, you'll miss BCC\n> recipients.\n\nUnderstood.\n\n> So the other suggestion is to have a separate hook to validate the\n> list of recipients. This might be a bit more involved if we want to\n> execute cleanly, but should not be rocket science.\n>\n> The send_message() sub prepares @recipients list to form quite a bit\n> of processing at the beginning, and the uses the resulting list to\n> drive the sendmail by adding it to @sendmail_parameters(). The\n> contents of this @recipients list is what you want to vet before the\n> code talks to the sendmail program or daemon later in the function.\n\nOne key point here is using the patch as input to the recipient\nvalidation. For example, requiring specific recipients when certain\nfiles are changed (a bit like get_maintainers.pl in Linux kernel). To\nmake it easier for the hook writer, both the patch and the recipients\nshould be passed to the hook at the same time.\n\nI think one possible alternative to adding a completely new hook would\nbe postponing the sendemail-validate hook, passing the same patch on the\ncommand-line as before (to ensure current users are unchanged), and\nadditionally passing in the recipients. Perhaps write the recipient list\nin a temp file, and pass the filename on the command-line or via the\nenvironment to the hook.\n\nAlas implementing this in perl *is* rocket science to me, so I'm pretty\nmuch dependent on the git community's help here.\n\nBR,\nJani.\n\n-- \nJani Nikula, Intel Open Source Graphics Center\n"},{"id":"389194","messageId":"87png0sknv.fsf@intel.com","threadId":"52550","inReplyTo":"87sgkwswhs.fsf@intel.com","subject":"Re: BUG: sendemail-validate hook is run too early","fromName":"Jani Nikula","fromEmail":"jani.nikula@intel.com","sentAt":"2020-01-03T14:02:12Z","receivedAt":"2020-01-03T14:02:19Z","isPatch":false,"sender":{"key":"jani.nikula@intel.com","avatar":null},"body":"On Fri, 03 Jan 2020, Jani Nikula wrote:\n> I think one possible alternative to adding a completely new hook would\n> be postponing the sendemail-validate hook, passing the same patch on the\n> command-line as before (to ensure current users are unchanged), and\n> additionally passing in the recipients.\n\nI realize the validation is done on *all* patches in a series before any\nfurther processing, so the suggestion to postpone the current validation\nhook is a bad idea. We'd need a new hook. The other points about having\nboth the patch contents and the recipients available to the hook remain.\n\nBR,\nJani.\n\n\n-- \nJani Nikula, Intel Open Source Graphics Center\n"}]}