From: Michael Strawbridge <michael.strawbridge@amd.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v9 0/2] send-email: expose header information to git-send-email's sendemail-validate hook
Date: Mon, 23 Jan 2023 11:03:37 -0500 [thread overview]
Message-ID: <5758ffc7-eb8c-4c16-d226-dd882cb2406b@amd.com> (raw)
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.
> <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.
next prev parent reply other threads:[~2023-01-23 16:04 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-20 1:24 [PATCH v9 0/2] send-email: expose header information to git-send-email's sendemail-validate hook Michael Strawbridge
2023-01-20 1:24 ` [PATCH v9 1/2] send-email: refactor header generation functions Michael Strawbridge
2023-01-20 1:24 ` [PATCH v9 2/2] send-email: expose header information to git-send-email's sendemail-validate hook Michael Strawbridge
2023-01-20 13:07 ` Luben Tuikov
2023-01-20 14:25 ` Michael Strawbridge
2023-04-19 17:01 ` Junio C Hamano
2023-04-19 18:05 ` Luben Tuikov
2023-01-23 13:51 ` [PATCH v9 0/2] " Ævar Arnfjörð Bjarmason
2023-01-23 16:03 ` Michael Strawbridge [this message]
2023-01-30 10:40 ` Ævar Arnfjörð Bjarmason
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5758ffc7-eb8c-4c16-d226-dd882cb2406b@amd.com \
--to=michael.strawbridge@amd.com \
--cc=avarab@gmail.com \
--cc=git@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).