gh-104306: Fix incorrect comment handling in the `netrc` module, minor refactor #104511

sleiderr · 2023-05-15T17:17:29Z

Issue: netrc emits syntax errors for comments after blank lines #104306

…oken()` method

bedevere-bot · 2023-05-15T17:17:35Z

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

ghost · 2023-05-15T17:17:55Z

All commit authors signed the Contributor License Agreement.

bedevere-bot · 2023-05-15T17:17:56Z

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

Double backticks

Misc/NEWS.d/next/Library/2023-05-15-17-22-53.gh-issue-104306.YMiegg.rst

Co-authored-by: Oleg Iarygin <oleg@arhadthedev.net>

ssbarnea · 2025-01-04T15:27:48Z

Any chance we can revive this? I already spend few hours trying to identify why I was getting 404 errors from github.com, just to discover that it was a commented line in my ~/.netrc, one that was ignored correctly by curl.

webknjaz · 2025-01-06T14:10:03Z

Misc/NEWS.d/next/Library/2023-05-15-17-22-53.gh-issue-104306.YMiegg.rst

@@ -0,0 +1 @@
+Fix incorrect comment parsing in the :mod:`netrc` module.


Could you expand on this and include context that would give an arbitrary changelog reader an idea of how the change might impact them?

webknjaz · 2025-01-06T14:13:16Z

Lib/test/test_netrc.py

@@ -309,5 +329,9 @@ def test_security(self):
                             ('anonymous', '', 'pass'))


+def test_main():


Why is this needed?

webknjaz · 2025-01-06T14:16:07Z

@sleiderr the CI is failing with this PR. Here I've extracted the relevant part of the log from one of the jobs:

1 test failed:
    test_netrc

435 tests OK.

0:12:34 load avg: 5.15 Re-running 1 failed tests in verbose mode in subprocesses
0:12:34 load avg: 5.15 Run 1 test in parallel using 1 worker process (timeout: 10 min, worker timeout: 15 min)
0:12:34 load avg: 5.15 [1/1/1] test_netrc failed (uncaught exception)
Re-running test_netrc in verbose mode
test test_netrc crashed -- Traceback (most recent call last):
  File "D:\a\cpython\cpython\Lib\test\libregrtest\single.py", line 184, in _runtest_env_changed_exc
    _load_run_test(result, runtests)
    ~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^
  File "D:\a\cpython\cpython\Lib\test\libregrtest\single.py", line 129, in _load_run_test
    test_mod = importlib.import_module(module_name)
  File "D:\a\cpython\cpython\Lib\importlib\__init__.py", line 88, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
           ~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<frozen importlib._bootstrap>", line 1386, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1359, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1330, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 935, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 756, in exec_module
  File "<frozen importlib._bootstrap>", line 488, in _call_with_frames_removed
  File "D:\a\cpython\cpython\Lib\test\test_netrc.py", line 6, in <module>
    from test.support import os_helper, run_unittest
ImportError: cannot import name 'run_unittest' from 'test.support' (D:\a\cpython\cpython\Lib\test\support\__init__.py)
1 test failed again:
    test_netrc

== Tests result: FAILURE then FAILURE ==

(https://github.com/python/cpython/actions/runs/12611674426/job/35147581981?pr=104511#step:6:631)

webknjaz · 2025-01-06T14:17:05Z

Lib/test/test_netrc.py

+import sys
+import textwrap
+import unittest
+from test.support import os_helper, run_unittest


This causes a ImportError: cannot import name 'run_unittest' from 'test.support' (#104511 (comment)).

webknjaz

I think that dropping the unrelated changes may fix #104511 (comment).

Lib/test/test_netrc.py

Lib/netrc.py

Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>

Lib/netrc.py

webknjaz · 2025-01-06T22:11:53Z

0:00:07 load avg: 9.38 [ 59/482] test_netrc passed

webknjaz · 2025-01-06T22:13:44Z

Lib/netrc.py

@@ -187,6 +187,3 @@ def __repr__(self):
                rep += line
            rep += "\n"
        return rep


Revert deleting the CLI entry point logic:

Suggested change

return rep

return rep

if __name__ == '__main__':

print(netrc())

webknjaz · 2025-01-06T22:14:25Z

Lib/netrc.py

+                raise NetrcParseError(
+                    "missing %r name" % tt, file, lexer.lineno)


looks like an irrelevant formatting change

Suggested change

raise NetrcParseError(

"missing %r name" % tt, file, lexer.lineno)

raise NetrcParseError("missing %r name" % tt, file, lexer.lineno)

webknjaz · 2025-01-06T22:19:20Z

Lib/netrc.py

-                if lexer.lineno == saved_lineno and len(tt) == 1:
+                # For top level tokens, we skip line if the # is followed
+                # by a space / newline. Otherwise, we only skip the token.
+                if tt == '#' and not lexer.dontskip:


I'm not sure if I understand why the entirety of t is being compared to a '#'. Is this semantically “the entire token consists of just #”?
This could be

Suggested change

if tt == '#' and not lexer.dontskip:

if len(tt) == 1 and not lexer.dontskip:

but then, why does it matter whether it's # thing vs #thing? Typically, comment parsers just disregard whatever's after the hash and don't interpret that in any way…

webknjaz · 2025-01-06T22:20:54Z

Lib/netrc.py

+            if ch in self.whitespace and not enquoted:
+                if token == "":
+                    continue
+                if ch == '\n':


What about \r? \r\n?

webknjaz · 2025-01-06T22:21:51Z

Lib/netrc.py

-        for ch in fiter:
-            if ch in self.whitespace:
+        enquoted = False
+        while ch := self._read_char():


Was this refactoring necessary to fix the bug? It's rather difficult to read the diff with so many lines reshuffled. If I were to guess, this might be the reason people are postponing doing reviews on this PR.

webknjaz · 2025-01-06T22:22:34Z

Lib/netrc.py

+import os
+import stat


Let's undo formatting to make sure only relevant changes are visible in the diff.

Suggested change

import os

import stat

import os, stat

webknjaz · 2025-01-06T22:24:05Z

Lib/netrc.py

-            return self.hosts['default']
-        else:
-            return None
+        return self.hosts.get(host, self.hosts.get('default'))


I have a feeling that this might not be relevant to the fix and would be better in a separate refactoring PR.

sleiderr · 2025-01-07T23:02:01Z

Hi @webknjaz - thanks for reviewing this pull request.

I agree that most of the changes were not relevant to the actual bug fix and were more confusing than anything else (even for myself, one year later).

I've reverted to the upstream version of the module, and fixed the bug in (hopefully) a clearer manner.
Trailing new lines were messing up a check that I assume was supposed to determine whether a token was the the last one on a line, to then ensure that we do not skip line twice when parsing comment. But the way this check is implemented (checking if the current line number increased after calling the lever) is not quite correct, especially with extra blank lines that increase the current line number, but do not necessarily mean that the token was the last on its line.

Removing extra new lines before storing the line count fixes that issue, I've added a few test cases based on the original bug report.

sleiderr added 3 commits May 15, 2023 02:01

Fixed netrc comments handling

52ea05f

Implementation that passes all test cases. Rework on the `lexer.get_t…

2810677

…oken()` method

Removed debug remnants

68c946d

bedevere-bot added the awaiting review label May 15, 2023

bedevere-bot mentioned this pull request May 15, 2023

netrc emits syntax errors for comments after blank lines #104306

Open

Merge branch 'main' into pythongh-104306-netrc-comments

dce8305

blurb-it bot and others added 3 commits May 15, 2023 17:22

📜🤖 Added by blurb_it.

a542e84

Missing whitespace

27b83e9

Update 2023-05-15-17-22-53.gh-issue-104306.YMiegg.rst

1920abe

Double backticks

arhadthedev reviewed May 15, 2023

View reviewed changes

Misc/NEWS.d/next/Library/2023-05-15-17-22-53.gh-issue-104306.YMiegg.rst Outdated Show resolved Hide resolved

sleiderr and others added 4 commits May 15, 2023 20:08

Link to module

8c243db

Co-authored-by: Oleg Iarygin <oleg@arhadthedev.net>

Merge branch 'main' into pythongh-104306-netrc-comments

3284bbf

Merge branch 'main' into pythongh-104306-netrc-comments

98d0df9

Merge branch 'main' into pythongh-104306-netrc-comments

3e1a021

ssbarnea approved these changes Jan 4, 2025

View reviewed changes

bedevere-app bot added awaiting core review and removed awaiting review labels Jan 4, 2025

ssbarnea mentioned this pull request Jan 4, 2025

[ERROR]: failed to download the file: HTTP Error 404: Not Found (when file does exist) ansible/galaxy#2668

Open

Merge branch 'main' into pythongh-104306-netrc-comments

47399ef

webknjaz reviewed Jan 6, 2025

View reviewed changes

webknjaz suggested changes Jan 6, 2025

View reviewed changes

Lib/test/test_netrc.py Outdated Show resolved Hide resolved

Lib/test/test_netrc.py Outdated Show resolved Hide resolved

Lib/test/test_netrc.py Outdated Show resolved Hide resolved

Lib/netrc.py Outdated Show resolved Hide resolved

sleiderr and others added 2 commits January 6, 2025 21:37

fix netrc unit tests

e835819

Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>

remove extra blank line

7687836

Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>

remove extra blank line

7f64f23

Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>

sleiderr commented Jan 6, 2025

View reviewed changes

Lib/netrc.py Outdated Show resolved Hide resolved

remove extra line

9e63185

sleiderr commented Jan 6, 2025

View reviewed changes

Lib/netrc.py Outdated Show resolved Hide resolved

remove extra blank line

dc72ad1

webknjaz reviewed Jan 6, 2025

View reviewed changes

fix: skip any trailing new line when lexing

5ed10a7

Merge branch 'main' into pythongh-104306-netrc-comments

e32c360

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-104306: Fix incorrect comment handling in the `netrc` module, minor refactor #104511

gh-104306: Fix incorrect comment handling in the `netrc` module, minor refactor #104511

sleiderr commented May 15, 2023

bedevere-bot commented May 15, 2023

ghost commented May 15, 2023 •

edited by ghost

Loading

bedevere-bot commented May 15, 2023

ssbarnea commented Jan 4, 2025

webknjaz Jan 6, 2025

webknjaz Jan 6, 2025

webknjaz commented Jan 6, 2025

webknjaz Jan 6, 2025

webknjaz left a comment

webknjaz commented Jan 6, 2025

webknjaz Jan 6, 2025

webknjaz Jan 6, 2025

webknjaz Jan 6, 2025

webknjaz Jan 6, 2025

webknjaz Jan 6, 2025

webknjaz Jan 6, 2025

webknjaz Jan 6, 2025

sleiderr commented Jan 7, 2025

		@@ -0,0 +1 @@
		Fix incorrect comment parsing in the :mod:`netrc` module.

		@@ -309,5 +329,9 @@ def test_security(self):
		('anonymous', '', 'pass'))


		def test_main():

		raise NetrcParseError(
		"missing %r name" % tt, file, lexer.lineno)

	raise NetrcParseError(
	"missing %r name" % tt, file, lexer.lineno)
	raise NetrcParseError("missing %r name" % tt, file, lexer.lineno)

	if tt == '#' and not lexer.dontskip:
	if len(tt) == 1 and not lexer.dontskip:

gh-104306: Fix incorrect comment handling in the netrc module, minor refactor #104511

Are you sure you want to change the base?

gh-104306: Fix incorrect comment handling in the netrc module, minor refactor #104511

Conversation

sleiderr commented May 15, 2023

bedevere-bot commented May 15, 2023

ghost commented May 15, 2023 • edited by ghost Loading

bedevere-bot commented May 15, 2023

ssbarnea commented Jan 4, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webknjaz commented Jan 6, 2025

Choose a reason for hiding this comment

webknjaz left a comment

Choose a reason for hiding this comment

webknjaz commented Jan 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sleiderr commented Jan 7, 2025

gh-104306: Fix incorrect comment handling in the `netrc` module, minor refactor #104511

gh-104306: Fix incorrect comment handling in the `netrc` module, minor refactor #104511

ghost commented May 15, 2023 •

edited by ghost

Loading