Please try Pmail

Please try Pmail

[Richard M Stallman]

Pmail is basically working now.  I have used it for all my mail
reading for weeks, and I have also gone through all the code making
sure all the commands work.  So please try it.

I see no reason not to include it in the next release.
It should not cause any delay.

Re: Please try Pmail

[Chong Yidong]

Richard M Stallman <rms@gnu.org> writes:

> Pmail is basically working now.  I have used it for all my mail
> reading for weeks, and I have also gone through all the code making
> sure all the commands work.  So please try it.
>
> I see no reason not to include it in the next release.
> It should not cause any delay.

Thanks.  First, the swapping code in pmail should be tweaked to use
write-region-annotate, like in Stefan's 2008-12-30 change to
tar-mode.el.

Also, I've been wondering about the following code in pmail.el, which is
used to perform babyl-to-mbox conversion.  It uses temp-files to
transfer the data.  Would this lead to a security hole via symlink
attack?  Could someone suggest a solution?  (Maybe a simple fix is to
make the temp files in .emacs.d?)


(defun pmail-convert-babyl-to-mbox ()
  "Convert the mail file from Babyl version 5 to mbox.
This function also reinitializes local variables used by Pmail."
  (let ((old-file (make-temp-file "pmail"))
        (new-file (make-temp-file "pmail")))
    (unwind-protect
      (progn
        (kill-all-local-variables)
        (write-region (point-min) (point-max) old-file)
        (unrmail old-file new-file)
        (message "Replacing BABYL format with mbox format...")
        (let ((inhibit-read-only t))
           (erase-buffer)
           (insert-file-contents-literally new-file)
           ...

Re: Please try Pmail

[Alfred M. Szmidt]

What about creating the temporary file in the users home directory?
Say, rebinding temporary-file-directory.

Re: Please try Pmail

[Stefan Monnier]

>> Pmail is basically working now.  I have used it for all my mail
>> reading for weeks, and I have also gone through all the code making
>> sure all the commands work.  So please try it.
>> 
>> I see no reason not to include it in the next release.
>> It should not cause any delay.

Great!  How 'bout moving pmail*.el over rmail*.el, then?

> Thanks.  First, the swapping code in pmail should be tweaked to use
> write-region-annotate, like in Stefan's 2008-12-30 change to
> tar-mode.el.

Sounds right.

> Also, I've been wondering about the following code in pmail.el, which is
> used to perform babyl-to-mbox conversion.  It uses temp-files to
> transfer the data.  Would this lead to a security hole via symlink
> attack?  Could someone suggest a solution?  (Maybe a simple fix is to
> make the temp files in .emacs.d?)

It uses `make-temp-file' whose main purpose is to encpasulate the code
needed to create a temp file safely.  If make-temp-file is not safe,
then we should fix it.


        Stefan

Re: Please try Pmail

[Chong Yidong]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> It uses `make-temp-file' whose main purpose is to encpasulate the code
> needed to create a temp file safely.  If make-temp-file is not safe,
> then we should fix it.

I just double-checked, and actually the situation looks OK:
make-temp-file makes the temp-file with restrictive permissions (400),
and assures that the created file is new.

Re: Please try Pmail

[Glenn Morris]

Trivia:

You moved pmail-last-label and pmail-last-multi-labels from pmail to
pmailkwd, and pmail-message-labels-p from pmail to pmailsum, on
20090105.

AFAICS this does nothing but create compiler warnings (from pmailsum
and pmailsort). pmailkwd and pmailsum both require pmail. Mind if I
move the definitions back to pmail.el?

Re: Please try Pmail

[Chong Yidong]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Thanks.  First, the swapping code in pmail should be tweaked to use
>> write-region-annotate, like in Stefan's 2008-12-30 change to
>> tar-mode.el.
>
> Sounds right.

Actually, there seems to be a problem with doing this.  The
straightforward approach for using write-region-annotate is:

(defun pmail-write-region-annotate (start end)
  (when (pmail-buffers-swapped-p)
    (set-buffer pmail-view-buffer)
    (widen)
    nil))

The `widen' is necessary because pmail-view-buffer, which contains the
swapped-out buffer contents, is narrowed to the current message.
Without it, only the narrowed part of pmail-view-buffer is written out,
which is wrong.  However, we need to re-narrow pmail-view-buffer after
performing write-region, otherwise pmail screws up.  It's not clear how
to do so.

So we must either (i) put buffer-swapped-with back in and use that, or
(ii) modify annotations to somehow automatically widen and save
restrictions when the current buffer is switched.

Any suggestions?

Re: Please try Pmail

[Richard M Stallman]

The purpose of make-temp-file is to prevent the symlink attack.
However, I see no reason why this needs to output into a temporary file.
Why not construct the mbox data in a buffer and leave it there?

Re: Please try Pmail

[Chong Yidong]

Richard M Stallman <rms@gnu.org> writes:

> However, I see no reason why this needs to output into a temporary file.
> Why not construct the mbox data in a buffer and leave it there?

I think because it calls the function unrmail, which was already in
existence, and unrmail operates on files.  It should be possible to
rewrite unrmail to operate on buffers; the only tricky spot would be
handling the coding-system conversion correctly.

Re: Please try Pmail

[Richard M Stallman]

    You moved pmail-last-label and pmail-last-multi-labels from pmail to
    pmailkwd, and pmail-message-labels-p from pmail to pmailsum, on
    20090105.

Yes, I thought they were used only in those files so I moved them there.

    AFAICS this does nothing but create compiler warnings (from pmailsum
    and pmailsort). pmailkwd and pmailsum both require pmail. Mind if I
    move the definitions back to pmail.el?

Ok.

Re: Please try Pmail

[Richard M Stallman]

    Great!  How 'bout moving pmail*.el over rmail*.el, then?

The symbols need to be renamed, too.

Also, the file pmailhdr.el should be deleted.
It is not really used.

Re: Please try Pmail

[Stefan Monnier]

>>> Thanks.  First, the swapping code in pmail should be tweaked to use
>>> write-region-annotate, like in Stefan's 2008-12-30 change to
>>> tar-mode.el.
>> Sounds right.

> Actually, there seems to be a problem with doing this.  The
> straightforward approach for using write-region-annotate is:

> (defun pmail-write-region-annotate (start end)
>   (when (pmail-buffers-swapped-p)
>     (set-buffer pmail-view-buffer)
>     (widen)
>     nil))

Ah, indeed, that's a problem.

> The `widen' is necessary because pmail-view-buffer, which contains the
> swapped-out buffer contents, is narrowed to the current message.

So Pmail displays the messages directly from the raw mbox buffer?
How does it deal with character encodings, then?

> So we must either (i) put buffer-swapped-with back in and use that, or
> (ii) modify annotations to somehow automatically widen and save
> restrictions when the current buffer is switched.

Or use write-contents-functions, or change the code so the raw mbox
buffer is never narrowed (i.e. never shown to the user).


        Stefan

Re: Please try Pmail

[Chong Yidong]

After further investigation, I tracked down the problem with using
write-region-annotate-functions in pmail.  Actually, calling `widen' in
the annotation function causes the file to be saved correctly; the
reason pmail is unable to recover its state after saving is not that the
restrictions in the swapped buffer are wrong.

The problem is that swapping to a different buffer in an annotation
function causes that buffer to be killed!

The docstring to write-region-annotate-functions does not say that
swapping to a different buffer causes that buffer to be killed
afterwards.  What is the purpose of this?  Here is the relevant code
from build_annotations_unwind (fileio.c:4256):


/* If build_annotations switched buffers, switch back to BUF.
   Kill the temporary buffer that was selected in the meantime.

   Since this kill only the last temporary buffer, some buffers remain
   not killed if build_annotations switched buffers more than once.
   -- K.Handa */

static Lisp_Object
build_annotations_unwind (buf)
     Lisp_Object buf;
{
  Lisp_Object tembuf;

  if (XBUFFER (buf) == current_buffer)
    return Qnil;
  tembuf = Fcurrent_buffer ();
  Fset_buffer (buf);
  Fkill_buffer (tembuf);
  return Qnil;
}

Re: Please try Pmail

[Richard M Stallman]

    > The `widen' is necessary because pmail-view-buffer, which contains the
    > swapped-out buffer contents, is narrowed to the current message.

    So Pmail displays the messages directly from the raw mbox buffer?

No it doesn't.  (If it did, it would need only one buffer.)
But it does, apparently, do narrowing in the buffer that holds
the file.

Re: Please try Pmail

[Stefan Monnier]

> /* If build_annotations switched buffers, switch back to BUF.
>    Kill the temporary buffer that was selected in the meantime.

>    Since this kill only the last temporary buffer, some buffers remain
>    not killed if build_annotations switched buffers more than once.
>    -- K.Handa */

> static Lisp_Object
> build_annotations_unwind (buf)
>      Lisp_Object buf;
> {
>   Lisp_Object tembuf;

>   if (XBUFFER (buf) == current_buffer)
>     return Qnil;
>   tembuf = Fcurrent_buffer ();
>   Fset_buffer (buf);
>   Fkill_buffer (tembuf);
>   return Qnil;
> }

Duh! I tested saving, and checked that the file's contents was right,
but never noticed this.  It turns out that this code is used by
format-annotate-function, so there's a good reason for it to be there.

We should try to find a way to get both cases working right (and even
the case where build_annotations switched buffers more than once,
ideally).

The cleanest solution is to let build_annotations_unwind run some
buffer-local hook function (e.g. write-region-post-annotate-function),
which can either run kill-buffer, and/or re-narrow the buffer, and/or
kill previous buffers.  The current code already allows it via
kill-buffer-hook, but using that is ugly and will lead to
other surprises.


        Stefan

Re: Please try Pmail

[Chong Yidong]

"Stefan Monnier" <monnier@iro.umontreal.ca> writes:

> We should try to find a way to get both cases working right (and even
> the case where build_annotations switched buffers more than once,
> ideally).
>
> The cleanest solution is to let build_annotations_unwind run some
> buffer-local hook function (e.g. write-region-post-annotate-function),
> which can either run kill-buffer, and/or re-narrow the buffer, and/or
> kill previous buffers.  The current code already allows it via
> kill-buffer-hook, but using that is ugly and will lead to
> other surprises.

The annotation functions would need to add functions to that hook when
they are run.  Maybe this is a good solution, but I'm not sure.

Another idea: allow a new type of return value for annotation functions,
and use this to keep track of buffers to be killed.  For example, allow
annotation functions to return (FUN1 . FUN2), where FUN1 and FUN2 are
lambda functions.  Then FUN1 is called during annotation, and FUN2 is
called after other annotations have taken place.

Re: Please try Pmail

[Stefan Monnier]

>> The cleanest solution is to let build_annotations_unwind run some
>> buffer-local hook function (e.g. write-region-post-annotate-function),
>> which can either run kill-buffer, and/or re-narrow the buffer, and/or
>> kill previous buffers.  The current code already allows it via
>> kill-buffer-hook, but using that is ugly and will lead to
>> other surprises.
> The annotation functions would need to add functions to that hook when
> they are run.

Not really.  When they switch to another buffer, they may need to adjust
that hook in that buffer.  The default value could be `kill-buffer', so
as to preserve current behavior.

> Another idea: allow a new type of return value for annotation functions,
> and use this to keep track of buffers to be killed.  For example, allow
> annotation functions to return (FUN1 . FUN2), where FUN1 and FUN2 are
> lambda functions.  Then FUN1 is called during annotation, and FUN2 is
> called after other annotations have taken place.

Since they return a buffer already, we may as well store the FUN2
inside that buffer (as a buffer-local var).


        Stefan

Re: Please try Pmail

[Eli Zaretskii]

> From: Richard M Stallman <rms@gnu.org>
> Date: Tue, 20 Jan 2009 18:00:52 -0500
> Cc: cyd@stupidchicken.com, emacs-devel@gnu.org
> Reply-To: rms@gnu.org
> 
>     So Pmail displays the messages directly from the raw mbox buffer?
> 
> No it doesn't.  (If it did, it would need only one buffer.)
> But it does, apparently, do narrowing in the buffer that holds
> the file.

What for?  If display is from another buffer, why do we need to
narrow?  That's Rmail legacy, isn't it.

Re: Please try Pmail

[Chong Yidong]

"Stefan Monnier" <monnier@iro.umontreal.ca> writes:

>> The annotation functions would need to add functions to that hook when
>> they are run.
>
> Not really.  When they switch to another buffer, they may need to adjust
> that hook in that buffer.  The default value could be `kill-buffer', so
> as to preserve current behavior.
>
>> Another idea: allow a new type of return value for annotation functions,
>> and use this to keep track of buffers to be killed.  For example, allow
>> annotation functions to return (FUN1 . FUN2), where FUN1 and FUN2 are
>> lambda functions.  Then FUN1 is called during annotation, and FUN2 is
>> called after other annotations have taken place.
>
> Since they return a buffer already, we may as well store the FUN2
> inside that buffer (as a buffer-local var).

They don't return a buffer.  They switch to a different buffer, and
return nil.

Here's one reason against adding a new hook to post-process annotations:
to do this particular job right, we need to run all the hook functions
in all the buffers that have been swapped to (so as to pick up their
local values).  This violates the usual behavior of run-hooks, which
only runs the buffer-local value of the hook, followed by the global
value of the hook.  The lambda function idea tries to avoid this by
making the post-processing part of the annotations process.

Re: Please try Pmail

[Richard M Stallman]

    The annotation functions would need to add functions to that hook when
    they are run.  Maybe this is a good solution, but I'm not sure.

I think it would be simpler to implement buffer-swapped-with.

Re: Please try Pmail

[Stefan Monnier]

>> Since they return a buffer already, we may as well store the FUN2
>> inside that buffer (as a buffer-local var).
> They don't return a buffer.  They switch to a different buffer, and
> return nil.

Yes, that's what I meant by "return a new buffer".  They aren't forced
to return nil, BTW (although in practice, it seems they always do).

> Here's one reason against adding a new hook to post-process annotations:
> to do this particular job right, we need to run all the hook functions
> in all the buffers that have been swapped to (so as to pick up their
> local values).  This violates the usual behavior of run-hooks, which
> only runs the buffer-local value of the hook, followed by the global
> value of the hook.

I don't propose a hook, but a buffer-local *-function (as opposed to
*-functions or *-hook).  I.e. a single function, just like your
proposal, except it's stored in the final "returned" buffer, rather than
returned explicitly.


        Stefan

Re: Please try Pmail

[Chong Yidong]

Richard M Stallman <rms@gnu.org> writes:

>     The annotation functions would need to add functions to that hook
>     when they are run.  Maybe this is a good solution, but I'm not
>     sure.
>
> I think it would be simpler to implement buffer-swapped-with.

We have to fix the write-region-annotate-functions problem anyway (i.e.,
that swapping buffers more than once currently does the wrong thing).

Re: Please try Pmail

[Chong Yidong]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> I don't propose a hook, but a buffer-local *-function (as opposed to
> *-functions or *-hook).  I.e. a single function, just like your
> proposal, except it's stored in the final "returned" buffer, rather than
> returned explicitly.

You mean something like this?

  DEFVAR_LISP ("write-region-annotate-functions", &Vwrite_region_annotate_functions,
         doc: /* A list of functions to be called at the start of `write-region'.
Each is passed two arguments, START and END as for `write-region'.
These are usually two numbers but not always; see the documentation
for `write-region'.  The function should return a list of pairs
of the form (POSITION . STRING), consisting of strings to be effectively
inserted at the specified positions of the file being written (1 means to
insert before the first byte written).  The POSITIONs must be sorted into
increasing order.

If there are several annotation functions, the lists returned by the
annotation functions are merged destructively.  As each annotation
function runs, the variable `write-region-annotations-so-far' contains
a list of all annotations returned by previous annotation functions.

An annotation function can return with a different buffer current.
Doing so removes the annotations returned by previous functions.

After `write-region' completes, Emacs calls the function stored in
`write-region-post-annotation-function', once for each buffer that was
current when building the annotations (i.e., at least once if
`write-region-annotate-functions' is non-nil).  This function is called
with no arguments, and with that buffer current.  */);

Re: Please try Pmail

[Stefan Monnier]

>> I don't propose a hook, but a buffer-local *-function (as opposed to
>> *-functions or *-hook).  I.e. a single function, just like your
>> proposal, except it's stored in the final "returned" buffer, rather than
>> returned explicitly.
> You mean something like this?
[...]
> After `write-region' completes, Emacs calls the function stored in
> `write-region-post-annotation-function', once for each buffer that was
> current when building the annotations (i.e., at least once if
> `write-region-annotate-functions' is non-nil).  This function is called
> with no arguments, and with that buffer current.  */);

Well, that's a possibility.  You could also just say "run the
write-region-post-annotation-function of the last buffer" and let the
annotation code do the work of accumulating the intermediate
write-region-post-annotation-functions in the last buffer's
write-region-post-annotation-function.

After all, this corner case never happens AFAIK, so there's no point
spending much time on it.


        Stefan

Re: Please try Pmail

[Chong Yidong]

"Stefan Monnier" <monnier@iro.umontreal.ca> writes:

> You could also just say "run the write-region-post-annotation-function
> of the last buffer" and let the annotation code do the work of
> accumulating the intermediate write-region-post-annotation-functions
> in the last buffer's write-region-post-annotation-function.
>
> After all, this corner case never happens AFAIK, so there's no point
> spending much time on it.

No reason not to try to DTRT, I think.  Anyway, I've checked in the
changes, including those to pmail.

Re: Please try Pmail

[Richard M Stallman]

    No reason not to try to DTRT, I think.  Anyway, I've checked in the
    changes, including those to pmail.

Not marking pmail-buffer-swapped as a permanent local reintroduces a
disastrous bug I fixed a few weeks ago.  I hesitate to do a CVS update
now, so please fix this.

Re: Please try Pmail

[Chong Yidong]

Richard M Stallman <rms@gnu.org> writes:

>     No reason not to try to DTRT, I think.  Anyway, I've checked in the
>     changes, including those to pmail.
>
> Not marking pmail-buffer-swapped as a permanent local reintroduces a
> disastrous bug I fixed a few weeks ago.  I hesitate to do a CVS update
> now, so please fix this.

Done.