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

 ('anonymous', '', 'pass'))


+deftest_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_modulereturn _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

+importsys
+importtextwrap
+importunittest
+fromtest.supportimportos_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

 forlineinself.macros[macro]:
 rep+=line
 rep+="\n"
 returnrep


Revert deleting the CLI entry point logic:
Suggested change
returnrep
returnrep
if__name__=='__main__':
print(netrc())

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

Lib/netrc.py

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


looks like an irrelevant formatting change
Suggested change
raiseNetrcParseError(
"missing %r name"%tt, file, lexer.lineno)
raiseNetrcParseError("missing %r name"%tt, file, lexer.lineno)

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

Lib/netrc.py

-iflexer.lineno==saved_linenoandlen(tt) ==1:
+# For top level tokens, we skip line if the # is followed
+# by a space / newline. Otherwise, we only skip the token.
+iftt=='#'andnotlexer.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
iftt=='#'andnotlexer.dontskip:
iflen(tt)==1andnotlexer.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

+ifchinself.whitespaceandnotenquoted:
+iftoken=="":
+continue
+ifch=='\n':


What about \r? \r\n?

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

Lib/netrc.py

-forchinfiter:
-ifchinself.whitespace:
+enquoted=False
+whilech:=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

+importos
+importstat


Let's undo formatting to make sure only relevant changes are visible in the diff.
Suggested change
importos
importstat
importos, stat

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

Lib/netrc.py

-returnself.hosts['default']
-else:
-returnNone
+returnself.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.

webknjaz · 2025-07-31T15:10:48Z

@sleiderr would you improve the change note with now this affects the end-users in practice? No low-level details, just relatable high-level effects.

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' intopythongh-104306-netrc-comments
dce8305

blurb-itbotand 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 resolvedHide resolved

sleiderrand others added 4 commits May 15, 2023 20:08

Link to module
8c243db
Co-authored-by: Oleg Iarygin <oleg@arhadthedev.net>

Merge branch 'main' intopythongh-104306-netrc-comments
3284bbf

Merge branch 'main' intopythongh-104306-netrc-comments
98d0df9

Merge branch 'main' intopythongh-104306-netrc-comments
3e1a021

ssbarnea approved these changes Jan 4, 2025
View reviewed changes

bedevere-appbot 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' intopythongh-104306-netrc-comments
47399ef

webknjaz reviewed Jan 6, 2025
View reviewed changes

Lib/test/test_netrc.py Outdated
('anonymous', '', 'pass'))

deftest_main():
Copy link
Member
webknjazJan 6, 2025
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed?

webknjaz reviewed Jan 6, 2025
View reviewed changes

webknjaz requested changes Jan 6, 2025
View reviewed changes

Lib/test/test_netrc.py Outdated Show resolvedHide resolved
Lib/test/test_netrc.py Outdated Show resolvedHide resolved
Lib/test/test_netrc.py Outdated Show resolvedHide resolved
Lib/netrc.py Outdated Show resolvedHide resolved

sleiderrand 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.pyShow resolvedHide resolved

remove extra line
9e63185

sleiderr commented Jan 6, 2025
View reviewed changes

Lib/netrc.pyShow resolvedHide resolved

remove extra blank line
dc72ad1

webknjaz reviewed Jan 6, 2025
View reviewed changes

Lib/netrc.py Outdated
ifchinself.whitespaceandnotenquoted:
iftoken=="":
continue
ifch=='\n':
Copy link
Member
webknjazJan 6, 2025
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about \r? \r\n?

webknjaz reviewed Jan 6, 2025
View reviewed changes

fix: skip any trailing new line when lexing
5ed10a7

Merge branch 'main' intopythongh-104306-netrc-comments
e32c360

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

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

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

	iftt=='#'andnotlexer.dontskip:
	iflen(tt)==1andnotlexer.dontskip:

Uh oh!

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

Uh oh!

Conversation

sleiderr commented May 15, 2023

Uh oh!

bedevere-bot commented May 15, 2023

Uh oh!

ghost commented May 15, 2023• edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-bot commented May 15, 2023

Uh oh!

Uh oh!

ssbarnea commented Jan 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webknjaz commented Jan 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webknjaz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

webknjaz commented Jan 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sleiderr commented Jan 7, 2025

Uh oh!

webknjaz commented Jul 31, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

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