a wee code review (pamg.bugs/re-fix-eol)

[pamg...@gmail.com]

I'd like you to do a code review. To review this change, run

gvn review --project https://gvn.googlecode.com/svn pamg.bugs/re-fix-eol@981

Alternatively, to review the latest snapshot of this change
branch, run

gvn --project https://gvn.googlecode.com/svn review pamg.bugs/re-fix-eol

to review the following change:

*pamg.bugs/re-fix-eol@981 | pamg.bugs | 2008-07-15 15:28:17 -0800 (Tue, 15 Jul 2008)

Description:

Restore old fix for line endings in gvn mail on Windows, since the new attempt
didn't work in all cases. (Out of two users who tried it, one saw line-endings
doubled, and one -- me -- saw them removed entirely.)

* lib/gvn/subcommands/mail.py
(Handle_GvnMail): Reinstate old hack to force all EOLs to Unix (LF) before
handing them to the editor. This was added (with slightly
different comments) in r547, then removed in r800. The
recurrence of the problem wasn't noticed until now because
a new version of gvn hadn't been released.

Affected Paths:
M //trunk/lib/gvn/subcommands/mail.py

This is a semiautomated message from "gvn mail". See
<http://code.google.com/p/gvn/> to learn more.

Index: lib/gvn/subcommands/mail.py
===================================================================
--- lib/gvn/subcommands/mail.py (^/trunk/lib/gvn/subcommands/mail.py@979)

+++ lib/gvn/subcommands/mail.py (^/changes/pamg.bugs/re-fix-eol/trunk/lib/gvn/subcommands/mail.py@981)

@@ -181,6 +181,17 @@ def Handle_GvnMail(ctx):

project.mail_template % template_dict,
])

+ # On Windows, the interaction among Python and Cygwin or the cmd shell
+ # can be a mess, even after the endings on the diff text have been normalized
+ # (see changebranch.Diff). The message is left with mixed line endings that
+ # confuse some editors and either drop or double endings in the final mail.
+ # We're left with this unfortunate hack to convert them all to Unix endings
+ # (\n) here, and let Python and/or Cygwin handle converting that to the OS's
+ # native ending when they write the file out.
+ # Assume endings are some combination of \r, \n, and \r\n.
+ if sys.platform == 'win32':
+ message = message.replace('\r\n', '\n').replace('\r', '\n')
+
editor = None
try:
if ctx.options.non_interactive:

[David Glasser]

LGTM. I can't easily test on Windows, but I trust that you have.

--dave

--
David Glasser | gla...@davidglasser.net | http://www.davidglasser.net/

[Pam Greene]

I haven't run unit tests, but none of them should be affected by this. (They weren't when it was removed.)  I have tried it out locally, and while I can't guarantee that this fixes all cases, evidence indicates that it's an improvement.

- Pam