From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS6315 166.70.0.0/16 X-Spam-Status: No, score=-3.7 required=3.0 tests=AWL,BAYES_00, RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from out03.mta.xmission.com (out03.mta.xmission.com [166.70.13.233]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dcvr.yhbt.net (Postfix) with ESMTPS id 5A4C41F461; Sat, 18 May 2019 15:10:16 +0000 (UTC) Received: from in01.mta.xmission.com ([166.70.13.51]) by out03.mta.xmission.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.87) (envelope-from ) id 1hS0yU-0002Bp-NK; Sat, 18 May 2019 09:10:14 -0600 Received: from ip72-206-97-68.om.om.cox.net ([72.206.97.68] helo=x220.xmission.com) by in01.mta.xmission.com with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.87) (envelope-from ) id 1hS0yI-0004Z3-K1; Sat, 18 May 2019 09:10:14 -0600 From: ebiederm@xmission.com (Eric W. Biederman) To: Eric Wong Cc: meta@public-inbox.org References: <87h89tzvp5.fsf@xmission.com> <20190518080300.q2klxia2uymnoxyi@dcvr> Date: Sat, 18 May 2019 10:09:38 -0500 In-Reply-To: <20190518080300.q2klxia2uymnoxyi@dcvr> (Eric Wong's message of "Sat, 18 May 2019 08:03:00 +0000") Message-ID: <87blzzu7d9.fsf@xmission.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-XM-SPF: eid=1hS0yI-0004Z3-K1;;;mid=<87blzzu7d9.fsf@xmission.com>;;;hst=in01.mta.xmission.com;;;ip=72.206.97.68;;;frm=ebiederm@xmission.com;;;spf=neutral X-XM-AID: U2FsdGVkX1+y67S/rNcRyhT5Ig7gD6bXpXHvMSwshcw= X-SA-Exim-Connect-IP: 72.206.97.68 X-SA-Exim-Mail-From: ebiederm@xmission.com Subject: Re: [PATCH] PublicInbox::Import Extend add with a optional raw message parameter X-SA-Exim-Version: 4.2.1 (built Thu, 05 May 2016 13:38:54 -0600) X-SA-Exim-Scanned: Yes (on in01.mta.xmission.com) List-Id: Eric Wong writes: > "Eric W. Biederman" wrote: >> >> I don't trust the MIME type to not munge my email messages in horrible >> ways upon occasion. Therefore allow for passing in the raw message >> value instead of trusting the mime object to preserve it. >> >> Signed-off-by: "Eric W. Biederman" > > I've had the same concern in the past about Email::MIME and > Email::Simple. But after reading the code for Email::MIME, > Email::Simple and Email::{MIME,Simple}::Header, I don't think > the implementation of Email::MIME->as_string and all methods it > calls does anything unreasonable. > > The only notable munging they seem to do is make irrelevant > whitespace changes in headers and maybe fix quoting in headers. > No body changes AFAIK. I was hoping I could skip the Email::MIME stuff entirely but the headers do need a bit of parsing to derive the git commit information. I will take a second look at the code. I did not make it all of the way through last time. I just know the bazillions of warnings I could not easily track down with old emails did not make me comfortable with that code base as anything other than a suggestion. >> The context here is because the only copy of messages that I save >> I save in public-inbox I don't want to have to worry about losing >> information. So I just pass the raw email_str to add. >> >> I expect if I were to export these lists public I would want to do >> some more but for now I am just putting them in public-inbox >> so that I can read and archive the lists locally. > > I worry about public archives getting badly munged, too. > >> lib/PublicInbox/Import.pm | 10 +++++----- >> lib/PublicInbox/V2Writable.pm | 8 ++++---- >> 2 files changed, 9 insertions(+), 9 deletions(-) > > Did you have plans to modify -mda/-watch or another script to > use this? I have been using this for a while with my own imap fetcher script. As for the others I certain could. I don't use -mda or -watch so they have not been a priority. >> diff --git a/lib/PublicInbox/Import.pm b/lib/PublicInbox/Import.pm >> index 81a38fb6987d..0a63784414f2 100644 >> --- a/lib/PublicInbox/Import.pm >> +++ b/lib/PublicInbox/Import.pm >> @@ -359,7 +359,7 @@ sub clean_tree_v2 ($$$) { >> # returns undef on duplicate >> # returns the :MARK of the most recent commit >> sub add { >> - my ($self, $mime, $check_cb) = @_; # mime = Email::MIME >> + my ($self, $mime, $check_cb, $email_str) = @_; # mime = Email::MIME > > I usually place callback args at the end of the arg list so > it's easy to write: > > $im->add($mime, sub { > # ... > }); > > So having a parameter after the sub{} is a bit ugly... > If I had to support this, I think I'd accept $mime being > a plain hashref: > > if (ref($mime) eq 'HASH') { > $raw = $mime->{raw}; > $mime = $mime->{mime}; > } else { > $raw = $mime->as_string; > } > > But, I'm still on the fence about the idea... > > Side note: I'm also taking the opportunity to use "$raw" instead > of "$str", because I've been bitten by the difference header_raw > vs header_str in the Email::MIME API, so consistency with > that API would be good, here. Yes the distinction about "$raw" is fair, and the placement in the argument list makes sense. As for the hashref. Perhaps what I should do is modify PublicInbox::MIME to at least conditionally keep the original raw email around. Then the logic to get the raw email could be kept in PublicInbox::MIME as well. Which let's me think the general solution is to have a configuration option somewhere that says we want to archive the raw email. We update PublicInbox::MIME to always keep the original raw email. If add is not configured to drop any headers we use the raw original email str. If add drops any headers we do what we do today. I think perhaps we could move all of the scrubbing into PublicInbox::Filter::Base.pm::scrub. Instead of having a hard coded drop_unwanted_headers in PublicInbox::Import. That would make it very straight forward to just make this a knob that the user controls for how they want their email received/imported. If that general idea sounds palatable I will investigate to see if I can move the caching of the raw email into PublicInbox::MIME, see about moving the dropping of headers into an appropriate config knob. Eric