Bug#812108: [PATCH] problem upgrading from offlineimap 6.3.4 to 6.6.1
On Sun, Mar 06, 2016 at 10:11:54PM +0100, Nicolas Sebrecht wrote: > Comments follow. I think the current logic is uselessly complex > > -- %< -- > > Signed-off-by: Nicolas Sebrecht > --- > offlineimap/folder/Maildir.py | 7 +++ > 1 file changed, 7 insertions(+) > > diff --git a/offlineimap/folder/Maildir.py b/offlineimap/folder/Maildir.py > index 77a774b..1546a2f 100644 > --- a/offlineimap/folder/Maildir.py > +++ b/offlineimap/folder/Maildir.py > @@ -129,6 +129,13 @@ class MaildirFolder(BaseFolder): > foldermatch = folderstr in filename > # If there was no folder MD5 specified, or if it mismatches, > # assume it is a foreign (new) message and ret: uid, fmd5 = None, > None > + > +# XXX: This is wrong behaviour: if FMD5 is missing or mismatches, > assume > +# the mail is new and **fix UID to None** to avoid any conflict. > + > +# XXX: If UID is missing, I have no idea what FMD5 can do. Should be > +# fixed to None in this case, too. The more I think about those 15 lines of code, the more I think this _parse_filename() method is critical to avoid data loss and UID conflicts. The name of this method is a bit lying around. It's from there it's later decided if the file is considered new mail or not (whether UID is None). > + > if foldermatch: > uidmatch = re_uidmatch.search(filename) > if uidmatch: -- Nicolas Sebrecht
Bug#812108: [PATCH] problem upgrading from offlineimap 6.3.4 to 6.6.1
On Sun, Mar 06, 2016 at 09:32:47PM +0200, Ilias Tsitsimpis wrote: > I tried to find a way for this to automatically happen when OfflineIMAP > detects that the user is upgrading from an older version (just like the > LocalStatus cache is upgraded) but the only think that came into my mind > is to accept both FMD5 values as valid. > > If anyone has any better idea, I would be happy to try and implement it. Comments follow. I think the current logic is uselessly complex -- %< -- Signed-off-by: Nicolas Sebrecht --- offlineimap/folder/Maildir.py | 7 +++ 1 file changed, 7 insertions(+) diff --git a/offlineimap/folder/Maildir.py b/offlineimap/folder/Maildir.py index 77a774b..1546a2f 100644 --- a/offlineimap/folder/Maildir.py +++ b/offlineimap/folder/Maildir.py @@ -129,6 +129,13 @@ class MaildirFolder(BaseFolder): foldermatch = folderstr in filename # If there was no folder MD5 specified, or if it mismatches, # assume it is a foreign (new) message and ret: uid, fmd5 = None, None + +# XXX: This is wrong behaviour: if FMD5 is missing or mismatches, assume +# the mail is new and **fix UID to None** to avoid any conflict. + +# XXX: If UID is missing, I have no idea what FMD5 can do. Should be +# fixed to None in this case, too. + if foldermatch: uidmatch = re_uidmatch.search(filename) if uidmatch: -- 2.6.0
Bug#812108: problem upgrading from offlineimap 6.3.4 to 6.6.1
On Sun, Mar 06, 2016 at 09:32:47PM +0200, Ilias Tsitsimpis wrote: > I sent the pull request[1]. > I successfully did a migration from version 6.3.4 to 6.6.1. > Please review and/or test. Reviewed and applied. Thank you. -- Nicolas Sebrecht
Bug#812108: problem upgrading from offlineimap 6.3.4 to 6.6.1
On Fri, Mar 04, 2016 at 01:49:29PM +0200, Ilias Tsitsimpis wrote: > IIRC the old algo was using the local folder name in order to compute > the FMD5. So, it is relatively easy to check if the current FMD5 matches > that of the local folder name and only then change it to the new one. > This will indeed provide extra insurance against potential UID > conflicts, but at the same time limit the patch to only those cases > where one needs to upgrade from a version older than v6.3.5. I still > cannot find another usage for the above patch (i.e., other cases where > FMD5 has been modified and needs fixing), so, if you like, we may go > that way (and change the name of the flag to something like > --migrate-fmd5-using-nametrans). That's fine for me. If any new use case is discovered, updating your feature or introduce yet another one should be easy, BTW. > Indeed, the edge-case will occur if the user happens to move a message > since the previous sync. But as rare as this may seems, the results are > very bad (UID conflicts) and users are very sensitive with their emails. > I understand that there is no algorithm that will work for everyone, > hence, we will make sure that they have read the blog-post and made a > backup. Good! > I will update the patch and send a pull request (hopefully by the weekend). Thank you for working on this Ilias! -- Nicolas Sebrecht
Bug#812108: problem upgrading from offlineimap 6.3.4 to 6.6.1
On Fri, Mar 04, 2016 at 09:50:16AM +0100, franc...@avalenn.eu wrote: > On Sun, Feb 28, 2016 at 07:29:02PM +0100, Nicolas Sebrecht wrote: > > I guess this might be confusing or make bad things (including data loss) > > if applied on mails that has moved from a folder to another one without > > a full filename change because this might lead to UID conflicts. > > Indeed at first glance patch seems sound but this potential UID > conflicts annoys me. Is it possible to retrieve the older FMD5 (before > the change of using nametrans) and replace only those matching the old > one ? Theorically, I believe that's possible to re-introduce the old algo to re-hash the old FMD5. However, I'm not much worried. The conflicts would only appear in the very edge-case when the user runs the fix while unexpected filemane changes happened since the previous sync. I'd say this edge-case would not worth the trouble in the code. If you worry about that, it's enough to add a warning before fixing the hashes. Something like: "Ensure to dispose a proper backup of both the cache and the Maildir before running this fix. Be aware that if you made unexpected changes to filenames in the maildir (behind OfflineIMAP's back) since your previous sync, bad things might occur. Continue? (y/n)" Either way would still be much better than the current alternatives where users have to find the proper fix by themselves and possibly do worse things. Also, I may update the blog post written by François at http://www.offlineimap.org/configuration/2016/02/12/debian-upgrade-from-jessie-to-stretch.html if none of you intend to do it. -- Nicolas Sebrecht
Bug#812108: problem upgrading from offlineimap 6.3.4 to 6.6.1
On Sun, Feb 28, 2016 at 06:00:58PM +0200, Ilias Tsitsimpis wrote: > Control: tags -1 + patch > > Hi François, Nicolas, Hi, > I have created a patch that hopefully will help here[1]. My plan is to > include this to the next OfflineIMAP version in Debian, and state at the > NEWS file that everyone who is upgrading from v6.3.4 should run > > $ offlineimap --verify-fmd5 || offlineimap --fix-fmd5 > > before the first use of the newer version. > > Could you review this, and/or verify that this is working correctly? > > [1] https://github.com/iliastsi/offlineimap/commit/8d07ec308e83665 >From a quick review, this looks fine for a merge. I didn't check the validity of the algo, though. > @nicolas: Do you believe this should be included in the mainline > OfflineIMAP repository (i.e., do you believe this has any other use > other than upgrading from v6.3.4)? I'm in favor to get this merged ASAP, yes. I think distribution maintainers should backport it to versions from v6.3.4 to v6.7.0-rc2 (the upcoming v6.7.0-rc3 should include the patches). I guess this might be confusing or make bad things (including data loss) if applied on mails that has moved from a folder to another one without a full filename change because this might lead to UID conflicts. -- Nicolas Sebrecht
Bug#670120: [PATCH] Re: Make SIGHUP singal handler equivalent to SIGTERM and SIGINT.
On Sat, Jun 09, 2012 at 01:10:32PM +0200, Nicolas Sebrecht wrote: > On Tue, Jun 05, 2012 at 04:04:10AM +0100, Dmitrijs Ledkovs wrote: > > > offlineimap has several frontends that encourage running it from a > > terminal under an X session. When X session closes for a system > > shutdown, the terminals exit, after sending SIGHUP to their children. > > > > Previously SIGHUP was treated to be equivalent to SIGUSR1, i.e. wake > > up and sync all accounts. This causes delays during shutdown. > > > > According to Wikipedia [0], SIGHUP has been repurposed from a > > historical meaning to one of: > > * re-read configuration files, or reinitialize (e.g. Apache, sendmail) > > * controlling pseudo or virtual terminal has been closed > > > > I believe second meaning is more appropriate for offlineimap, and > > hence this patch makes SIGHUP to be handled in the same way SIGTERM > > and SIGINT are handled. > > > > This patch also adds documentation of the current handling of > > SIGTERM/SIGINT/SIGHUP signals. > > > > [0] http://en.wikipedia.org/wiki/SIGHUP > > Looks ok to me except that users might rely on this signal from script. > I think it's worth to warn about this change in the changelog. Taken to pu, anyway. Thank you. -- Nicolas Sebrecht -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Bug#670120: [PATCH] Re: Make SIGHUP singal handler equivalent to SIGTERM and SIGINT.
On Tue, Jun 05, 2012 at 04:04:10AM +0100, Dmitrijs Ledkovs wrote: > offlineimap has several frontends that encourage running it from a > terminal under an X session. When X session closes for a system > shutdown, the terminals exit, after sending SIGHUP to their children. > > Previously SIGHUP was treated to be equivalent to SIGUSR1, i.e. wake > up and sync all accounts. This causes delays during shutdown. > > According to Wikipedia [0], SIGHUP has been repurposed from a > historical meaning to one of: > * re-read configuration files, or reinitialize (e.g. Apache, sendmail) > * controlling pseudo or virtual terminal has been closed > > I believe second meaning is more appropriate for offlineimap, and > hence this patch makes SIGHUP to be handled in the same way SIGTERM > and SIGINT are handled. > > This patch also adds documentation of the current handling of > SIGTERM/SIGINT/SIGHUP signals. > > [0] http://en.wikipedia.org/wiki/SIGHUP Looks ok to me except that users might rely on this signal from script. I think it's worth to warn about this change in the changelog. Thanks for contribution! ,-) -- Nicolas Sebrecht -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Bug#610685: [PATCH] Re: Limit msg body length in debug output
On Sat, May 07, 2011 at 06:34:31PM +0200, Sebastian Spaeth wrote: > On Sat, 7 May 2011 13:56:29 +0200, Nicolas Sebrecht wrote: > > I inclined to merge this patch if it's still needed (thinking about the > > very last imaplib2 improvements around log). Should I? > > Difficult call :-). If we turn off the extremely verbose imaplib2 debug, > we don't get the full messages in the debug log any longer. But I think > we rarely need the full messages in the debug log. > > It might make people feel uncomfortable to send debug logs to others, so > I am inclined to have this one merged. We can tweak it later if we find > that we are missing out information that we need. Ok. Could you rebase this patch against current next, please? -- Nicolas Sebrecht -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Bug#610685: [PATCH] Re: Limit msg body length in debug output
On Thu, May 05, 2011 at 11:56:28PM +0200, Sebastian Spaeth wrote: > On Thu, 5 May 2011 20:18:24 +0200, Nicolas Sebrecht > wrote: > I agree that it would be nicer. > But that would entail some content analysis of each and every email > that we handle. I don't want to think about the performance impact of > that. Just outputting the first 100 and last 100 bytes was a nice > tradeoff that doesn't cost perf and let's us identify which mail is > being transferred. > > We currently still get the full body in the imaplib2 debug log, so that > each message is now (fully) twice in our debug logs. I am not sure what > the correct solution would be. Perhaps do the mail content analysis only > in debug mode and output only the headers then...? Still, this patch > would be an improvement until then. But it's your call, I won't be sad > if this is not applied. :) I inclined to merge this patch if it's still needed (thinking about the very last imaplib2 improvements around log). Should I? > > Why should we care of the last byte of the message? > > To see if a message was fully transmitted or cut off somewhere. Right, thanks. -- Nicolas Sebrecht -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Bug#610685: [PATCH] Re: Limit msg body length in debug output
On Wed, May 04, 2011 at 06:00:01PM +0200, Sebastian Spaeth wrote: > > We were outputting full message bodies to the debug log (often stderr), > and then again (as they go over the imaplib2 wire, imaplib logs > everything too). Not only is quite a privacy issue when sending in debug > logs but it can also freeze a console for quite some time. Plus it > bloats debug logs A LOT. Good. > Only output the first and last 100 bytes of each message body to the > debug log (we still get the full body from imaplib2 logging). This > limits privacy issues when handing the log to someone else, but usually > still contains all the interesting bits that we want to see in a log. I don't think it's a good strategy. We may have errors due to unexpected headers or so. I would rather keep the whole headers (maybe trimed from the "Received:" content, though). Why should we care of the last byte of the message? -- Nicolas Sebrecht -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
