[PATCH 2/3] Maildir relative paths change was not complete

[Vladimir Marek]

> > In commit e023f190b0eed84fefc10e28bfe5e4e0467ff257 we missed one spot
> > 
> > Signed-off-by: Vladimir Marek <vlmarek at volny.cz>
> > ---
> >  offlineimap/folder/Maildir.py |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/offlineimap/folder/Maildir.py b/offlineimap/folder/Maildir.py
> > index 3b53156..2646205 100644
> > --- a/offlineimap/folder/Maildir.py
> > +++ b/offlineimap/folder/Maildir.py
> > @@ -128,7 +128,7 @@ class MaildirFolder(BaseFolder):
> >          folderstr = ',FMD5=' + foldermd5
> >          for dirannex in ['new', 'cur']:
> >              fulldirname = os.path.join(self.getfullname(), dirannex)
> > -            files.extend(os.path.join(fulldirname, filename) for
> > +            files.extend(os.path.join(dirannex, filename) for
> >                           filename in os.listdir(fulldirname))
> >          for file in files:
> >              messagename = os.path.basename(file)
> 
> Stop! You are right that something is not correct here, but we need to
> be careful. Part of the confusion is because it is never documented what
> the messagelist[UI]={filename: XXX} is supposed to contain. It used to
> be absolute file paths, and I made it to be only the shorter version, to
> save memory.

Yes that were my thoughts also, what is filename supposed to contain?
Ten I thought it's path relative to self.getfullname(). Like

tmp/...
cur/...


> 1) I think we should explicitly document what format the filename field
> is supposed to take (I know you are not guilty of that omission, but we
> should fix it while we are at it).

+1


> 2) Now:
> 
> fulldirname = <absolute_folderroot>/<dirannex>
> and "files" is now a list of absolute file names such as
> /home/user/mail/INBOX/cur/1231231_FMD5=1_UID=123:2,S
> 
> You are making it a list of dirannex+filename:
> cur/1231231_FMD5=1_UID=123:2,S
> 
> which is file for the value to stuff in the messagelist construct, but
> the "file" variable is also used in this function:
>   os.path.getsize(file)
> 
> and that would fail if it were in your new form, rather than the old
> one. So we need to to use:
>  os.path.getsize(os.path.join(self.getfullname(), file))
> 
> Does this make sense?

Absolutely. It didn't explode to me since I have no maxsize set.


> I'll send an updated patch based on your patch. Please let me know if
> you are fine with that change.

Sure. I made it less broken, you'll make it correct :)


> P.S. Cool that you are actually using a test suite. Can you tell how to
> use it and where to get it? Do you have a separate repo for that?

I have six test cases so far, but the infrastructure should be quite
ready. I have it as a branch in my local repo, I do not know how to
publish it. I can send tar.gz of the directory to list, but I guess
that's too old-fashioned :)

Anyway I would like to finish some documentation and comments first to
make it shiny. Also I still haven't read the thread Nicolas pointed me
to ...

-- 
	Vlad