Re: [PATCH v9 0/2] send-email: expose header information to git-send-email's sendemail-validate hook

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jan 23, 2023, 13:51 UTC
Message-ID: <230123.86wn5ds602.gmgdl@evledraar.gmail.com>
In-Reply-To: <20230120012459.920932-1-michael.strawbridge@amd.com>

On Thu, Jan 19 2023, Michael Strawbridge wrote:

Show 11 quoted lines

> Thanks to Ævar for an idea to simplify these patches further.
>
> Michael Strawbridge (2):
>   send-email: refactor header generation functions
>   send-email: expose header information to git-send-email's
>     sendemail-validate hook
>
>  Documentation/githooks.txt | 27 +++++++++--
>  git-send-email.perl        | 95 +++++++++++++++++++++++---------------
>  t/t9001-send-email.sh      | 27 ++++++++++-
>  3 files changed, 106 insertions(+), 43 deletions(-)

Thanks for the update. Aside from any quibbles, I still have some fundimental concerns about the implementation here:

 * Other hooks take stdin, not this sort of file argument.

   We discussed that ending in
   https://public-inbox.org/git/20230117215811.78313-1-michael.strawbridge@amd.com/;
   but I probably shouldn't have mentioned "git hook" at all.

   I do think though that we shouldn't expose a UX discrepancy like this
   forever, but the ways forward out of that would seem to be to either
   to revert a7555304546 (send-email: use 'git hook run' for
   'sendemail-validate', 2021-12-22) & move forward from there, or to
   wait for those patches (which I'm currentnly CI-ing).

 * Aside from that, shouldn't we have a new "validate-headers" or
   whatever hook, instead of assuming that we can add another argument
   to existing users?...

 * ...except can we do it safely? Now, it seems to me like you have
   potential correctness issues here. We call format_2822_time() to make
   the headers, but that's based on "$time", which we save away earlier.

   But for the rest (e.g. "Message-Id" are we sure that we're giving the
   hook the same headers as the one we actually end up sending?

   But regardless of that, something that would bypass this entire
   stdin/potential correctness etc. problem is if we just pass an offset
   to the the, i.e. currently we have a "validate" which gets the
   contents, if we had a "validate-raw" or whatever we could just pass:

	<headers>
	\n\n
	<content>

   Where the current "validate" just gets "content", no? We could then
   either pass the offset to the "\n\n", or just trust that such a hook
   knows to find the "\n\n".

   I also think that would be more generally usable, as the tiny
   addition of some exit code interpretation would allow us to say "I
   got this, and consider this sent", which would also satisfy some who
   have wanted e.g. a way to intrecept it before it invokes "sendmail"
   (I remember a recent thread about that in relation to using "mutt" to
   send it directly)