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

From: Michael Strawbridge <michael.strawbridge@amd.com>
Date: Jan 23, 2023, 16:03 UTC
Message-ID: <5758ffc7-eb8c-4c16-d226-dd882cb2406b@amd.com>
In-Reply-To: <230123.86wn5ds602.gmgdl@evledraar.gmail.com>

On 2023-01-23 08:51, Ævar Arnfjörð Bjarmason wrote:

Show 27 quoted lines

> On Thu, Jan 19 2023, Michael Strawbridge wrote:
>
>> 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).

Ok. If we are at the point where the change is just trying to pass CI but the main logic is there I am willing to wait some time.

>
>  * 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?...

While it's true we could (and I don't have a super strong opinion here), I suppose I was foreseeing the potential that a user may want to have logic that requires both the email headers and contents. For example, only checking contents for a specific mailing list. If we split the hooks, a user would then need to figure out how to have them coordinate.

Show 12 quoted lines

>
>  * ...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:

I think there might be a part missing here: "problem is if we just pass an offset to the ___." So there's a chance I may not fully grasp your suggestion.

Show 16 quoted lines

> 	<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)
>
>

Are you suggesting to simply add the header to the current sendemail-validate hook?

I appreciate the feedback.