Use of strftime in movemail.ec

Use of strftime in movemail.ec

[Eli Zaretskii]

movemail.c uses strftime as follows:

  char fromline[100];
  if (! strftime (fromline, sizeof fromline,
		  "From movemail %a %b %e %T %Y\n", ltime))

The %e and %T formats are not supported by the MS-Windows runtime
version of strftime, and the result is that movemail produces mbox
files where Rmail doesn't see any valid email messages.

I believe these format specifiers were used under the assumption that
Gnulib's strftime will be used on systems that don't support the full
functionality.  However, Gnulib's strftime.c defines a function
my_strftime, which is renamed by config.h as nstrftime, and I don't
see anywhere any machinery to force movemail call nstrftime.  Am I
missing something?

For now, I pushed a change to emacs-25 (commit 7923112) to use only
portable specifiers there.  But if a simple and safe change can be
made that would cause nstrftime be called by movemail (without any
adverse effects on Emacs itself), let's do that instead.

Re: Use of strftime in movemail.ec

[Paul Eggert]

Eli Zaretskii wrote:
> I believe these format specifiers were used under the assumption that
> Gnulib's strftime will be used on systems that don't support the full
> functionality.

No, I merely assumed C99, which specifies the %e and %T format specifiers for 
strftime. I didn't know Microsoft lacks support for this. I didn't think Gnulib 
would be needed.

Using %d instead of %e changes the output format of movemail slightly, by using 
leading 0 rather than leading space for day of month. It's more conservative to 
stick with the same format, so I installed a further patch to try to do that. I 
can't easily test this further patch on Windows, but it clearly restores the old 
behavior elsewhere because now all the changes are inside "#ifdef WINDOWSNT".

Using Gnulib would be another possibility, but would be more intrusive for other 
reasons (we'd have to supply replacement implementations of emacs_getenv_TZ and 
emacs_setenv_TZ on all platforms), and I'd rather not do that so close to a release.

Re: Use of strftime in movemail.ec

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sat, 5 Mar 2016 11:39:07 -0800
> 
> Eli Zaretskii wrote:
> > I believe these format specifiers were used under the assumption that
> > Gnulib's strftime will be used on systems that don't support the full
> > functionality.
> 
> No, I merely assumed C99, which specifies the %e and %T format specifiers for 
> strftime. I didn't know Microsoft lacks support for this. I didn't think Gnulib 
> would be needed.

I see.

> Using %d instead of %e changes the output format of movemail slightly, by using 
> leading 0 rather than leading space for day of month. It's more conservative to 
> stick with the same format, so I installed a further patch to try to do that. I 
> can't easily test this further patch on Windows, but it clearly restores the old 
> behavior elsewhere because now all the changes are inside "#ifdef WINDOWSNT".

This change causes us have 2 slightly different format specifiers,
which subtly depend on each other for correct operation.  Wouldn't it
be better to pass the format string to movemail_strftime, and replace
%e and %T there?  Or maybe use %H:%M:%S instead of %T (it's just a
shorthand, right?)?  I'm a little bit nervous about having the format
duplicated.

> Using Gnulib would be another possibility, but would be more intrusive for other 
> reasons (we'd have to supply replacement implementations of emacs_getenv_TZ and 
> emacs_setenv_TZ on all platforms), and I'd rather not do that so close to a release.

I agree.

Thanks.

Re: Use of strftime in movemail.ec

[Eli Zaretskii]

> Date: Sat, 05 Mar 2016 21:58:58 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: emacs-devel@gnu.org
> 
> This change causes us have 2 slightly different format specifiers,
> which subtly depend on each other for correct operation.  Wouldn't it
> be better to pass the format string to movemail_strftime, and replace
> %e and %T there?  Or maybe use %H:%M:%S instead of %T (it's just a
> shorthand, right?)?  I'm a little bit nervous about having the format
> duplicated.

How about the follow-up below?

diff --git a/lib-src/movemail.c b/lib-src/movemail.c
index 873d85d..93d2ee3 100644
--- a/lib-src/movemail.c
+++ b/lib-src/movemail.c
@@ -807,7 +807,28 @@ static size_t
 movemail_strftime (char *s, size_t size, char const *format,
 		   struct tm const *tm)
 {
-  size_t n = strftime (s, size, "From movemail %a %b %d %H:%M:%S %Y\n", tm);
+  char fmt[size + 6], *q;
+  const char *p;
+
+  for (p = format, q = &fmt[0]; *p; )
+    {
+      if (*p == '%' && p[1] == 'e')
+	{
+	  strncpy (q, "%d", 2);
+	  q += 2;
+	  p += 2;
+	}
+      else if (*p == '%' && p[1] == 'T')
+	{
+	  strncpy (q, "%H:%M:%S", 8);
+	  q += 8;
+	  p += 2;
+	}
+      else
+	*q++ = *p++;
+    }
+
+  size_t n = strftime (s, size, fmt, tm);
   char *mday = s + sizeof "From movemail Sun Jan " - 1;
   if (*mday == '0')
     *mday = ' ';

Re: Use of strftime in movemail.ec

[Paul Eggert]

Eli Zaretskii wrote:
> Wouldn't it
> be better to pass the format string to movemail_strftime, and replace
> %e and %T there?

That would make movemail_strftime more general and less brittle, yes. Not sure 
it's worth the engineering effort.

 > Or maybe use %H:%M:%S instead of %T (it's just a
> shorthand, right?)?

That'd add a few bytes on non-MS-Windows platforms. Admittedly a small overhead, 
but if we're going to replace %e in the MS-Windows version we might as well 
replace %T too.

Re: Use of strftime in movemail.ec

[Paul Eggert]

Eli Zaretskii wrote:
> How about the follow-up below?

Yes, that's more general and shouldn't introduce bugs. It can mishandle %% in 
formats, if you care about full upward-compatibility. It should use memcpy 
instead of strncpy.

Personally I'd leave the code alone for simplicity's sake, but if you prefer the 
more-general version by all means....

Re: Use of strftime in movemail.ec

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sat, 5 Mar 2016 12:56:49 -0800
> 
> Eli Zaretskii wrote:
> > How about the follow-up below?
> 
> Yes, that's more general and shouldn't introduce bugs. It can mishandle %% in 
> formats, if you care about full upward-compatibility. It should use memcpy 
> instead of strncpy.
> 
> Personally I'd leave the code alone for simplicity's sake, but if you prefer the 
> more-general version by all means....

Thanks for the review and the comments.  I fixed the patch as you
suggested and pushed it.