It seems recent patch from Issue 10464 has introduced problems into
one line comment handling of netrc module.


Problem 1:

If there is a line starting with a comment sign the
following traceback is shown:

Traceback (most recent call last):
  File "/usr/lib/python2.7/netrc.py", line 32, in __init__
    self._parse(file, fp)
  File "/usr/lib/python2.7/netrc.py", line 90, in _parse
    file, lexer.lineno)
netrc.NetrcParseError: bad follower token '#' (./.netrc, line 3)


This can be fixed by modifying netrc.py like this:
71c71
<                 if (tt=='' or tt == 'machine' or
---
>                 if (tt=='' or tt == 'machine' or tt[0] == "#" or

Problem 2:

Now with the patch above it seems to work as long as there are
no comment lines like these:

a) with only pound sign:
#

b) with a pound sign followed by text without any space in between
#comment

If comment line like that is followed by netrc token that token
is consumed by fp.readline(). For example, if netrc file has
these contents:

<<<

machine host.domain.com login username password something

# comment1
machine host2.domain.com login username2 password something2

#comment2
machine host3.domain.com login username3 password something3

#
machine host4.domain.com login username4 password something4
>>>

netrc object will have entries only for host3 and host4.

Thanks for the report.  Would you like to submit a patch?  If so, guidelines are on http://docs.python.org/devguide

OK, finally got some time to make a patch. The fix is to seek the beginning of the comment before calling readline. That way the next line won't be deleted.

Also, provided a test case for this issue in the patch.

With these new additions, the test input is getting unwieldy.  If you have the time, I'd like to see the unit tests refactored to be more unit-testy.  That is, instead of a single test netrc file, have multiple inputs, one for each thing being tested, and turn setUp into a factory function that each test calls:

      def make_nrc (self, test_data):
        mode = 'w'
        if sys.platform not in ['cygwin']:
            mode += 't'
        fp = open(temp_filename, mode)
        fp.write(test_data)
        fp.close()
        return netrc.netrc(temp_filename)

You can also use textwrap.dedent to embed the test_string in the call to make_nrc in the test method in a pretty fashion:

      def test_default_login(self):
        nrc = self.make_nrc(textwrap.dedent("""\
            default login log2 password pass2
            """)
        self.assertEqual(self.nrc.hosts['default'], ('log2', None, 'pass2'))

If you don't have time to do this I'll do it at some point (not sure when).

I haven't looked at your fix in detail because the unit tests don't currently isolate the issues, but it looks to like it is the right approach.

I agree, the test input was becoming unmaintainable. So, I updated the patch with the refactored unit tests.

removed leftover debug code from patch

Uploading patch updated according to the review comments.

+1 to that last patch, modulo removal of an unnecessary docstring on one test method (IIRC the test runner would display it in verbose mode and that would not be useful output; the test speaks for itself, only a comment with this bug number is maybe missing).  Ruslan, you don’t have to edit the patch, David will do it before commit if he wants to.  Thanks for your contribution.

Thanks for helping with cleaning up the patch. I already removed that docstring too.

Here is an updated patch with the tests refactored even further.  The patch seems correct to me, and almost all the comment tests fail before the patch and pass after.

Benjamin, this patch fixes a regression relative to 2.7.1 and 3.1.3, so I'm setting it to release blocker so you can decide if it is worth putting in.  netrc is perhaps not a heavily used module, but the fact that we've had several bugs reports that have seen attention and fixes from users recently indicates it *is* used, and is probably worth fixing.  Up to you, though.  Sorry I didn't get this in before the RCs :(

The patch is for 2.7.

I'd like to see this go in.

+                # seek to the beginning of the comment, then skip the line.
+                pos = len(tt) + 1
+                lexer.instream.seek(-pos, 1)
+                lexer.instream.readline()

Can't you just lexer.instream.readline()?

+        if sys.platform != 'cygwin':
+            mode += 't'

Perhaps this deserves a comment.

+        self.assertEqual({'macro1': ['line1\n', 'line2\n'],
+                          'macro2': ['line3\n', 'line4\n']}, nrc.macros)

It's preferable that the literals are the second argument to assertEqual.

+        self.assertEqual(('bar', None, passwd), nrc.hosts['foo.domain.com'])
+        self.assertEqual(('foo', None, 'pass'), nrc.hosts['bar.domain.com'])
+

Here too.

lexer.instream.readline(): no, we can't just call that without the seek, because reading the token that started with # may have caused the line to be consumed already.  I've expanded the comment to explain this.

cygwin: I'd add a comment if I knew what that was for.  This was copied from the existing tests.

order of assertEqual args: this reversed style was the existing style in the test file, but since we are doing a full refactor anyway I've consistently reversed them in the updated patch.

Should I apply these to the current branches (including 3.1) or is it easier for you to do it in your release clones?  If you do it, note that the test patch doesn't apply cleanly to 3.x, but that's only because of a couple of PEP8ifications (a space was removed from between the name and the '(' in a couple function calls in 3.x), which should be easy enough to adjust by hand in order to apply the patch.

New changeset cb3a77b0f8dd by Benjamin Peterson in branch '2.7':
fix regression in netrc comment handling (closes #12009)
http://hg.python.org/cpython/rev/cb3a77b0f8dd

New changeset 6993910be426 by Benjamin Peterson in branch '2.7':
merge 2.7.2 release branch with fix for #12009
http://hg.python.org/cpython/rev/6993910be426

New changeset 8625ce7da152 by Benjamin Peterson in branch '3.1':
fix regression in netrc comment handling (closes #12009)
http://hg.python.org/cpython/rev/8625ce7da152

New changeset 6b93cbb69e49 by Benjamin Peterson in branch '3.2':
merge 3.1 (#12009)
http://hg.python.org/cpython/rev/6b93cbb69e49

New changeset 734831b62549 by Benjamin Peterson in branch 'default':
merge 3.2 (#12009)
http://hg.python.org/cpython/rev/734831b62549