From: Michael Strawbridge Date: Mon, 23 Jan 2023 16:03:37 GMT Subject: Re: [PATCH v9 0/2] send-email: expose header information to git-send-email's sendemail-validate hook 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: > 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. > > * ...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. > > \n\n > > > 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.