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@xxxxxxx/; > 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. > <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.
