Repository navigation
Reject comments after implicit boolean config keys - #2239
Merged
Byron merged 1 commit intoSep 13, 2026
Merged
Byron merged 1 commit into
Byron merged 1 commit into
Conversation
Copy link
Copy Markdown
Member
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The reviewed changes and regression coverage address the stated parsing issue.
Pull request overviewFixes parsing and round-tripping of implicit boolean keys followed by # or ; comments.
Changes:
- Stops option matching at comment markers.
- Adds regression coverage for parsing, boolean access, and write-back.
| File | Summary |
|---|---|
| test/test_config.py | Adds comment-handling regression tests. |
| git/config.py | Updates assignment and valueless-key regex handling. |
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Sorry, something went wrong.
Byron
force-pushed
the
2238-follow-up
branch
from
42566c6 to
0087d89
Compare
September 13, 2026 17:12
Copy link
Copy Markdown
Member
Author
|
Actually these have to be rejected, just like in Git. |
Sorry, something went wrong.
Note that this is just a fixup, on a huge hack which is the native git-config parsing. Let's just hope this holds up until v4. <!-- agent --> GitConfigParser accepted entries such as "enabled # comment" and "enabled ; comment" even though Git rejects them. The comment became part of the option name, and an equals sign or colon inside the comment could make the entry look like an assignment. Silently stripping the comment would also accept configuration that Git considers invalid. Exclude both comment markers from the shared option-name expression and require a full-line match for valueless options. The assignment pattern cannot cross a comment marker, and the valueless fallback cannot accept just the valid-looking prefix. Such lines now raise the existing ParsingError during reading or an attempted edit. Ordinary bare keys retain their implicit true value and round-trip behavior. Add six regression cases covering both markers, spaces, tabs, adjacent comments, and assignment delimiters inside comments. Compare rejection with git config, check both getboolean and an unrelated edit raise ParsingError, and verify that the failed edit leaves the original bytes untouched. All six cases failed before full-line matching was added. Git reference: checkout 1630431f326e15fcde608827b5ff38422528eb59, config.c:get_value. Without an assignment, that parser requires the line to end after the key and optional whitespace. Runtime comparisons used Git 2.50.1 (Apple Git-155), which rejected all six inputs with exit status 128. Validation on Python 3.12.14: 42 configuration tests and six regression subtests passed, with two existing skips. Ruff lint and formatting and git diff --check passed. Assisted-by: GPT 6.0 Co-authored-by: GPT 6.0 <codex@openai.com>
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The parser and regression tests do not implement or verify the intended comment-handling behavior.
Get a fresh assessment by requesting another Copilot review.
Review details- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
Sorry, something went wrong.
Byron
force-pushed
the
2238-follow-up
branch
from
0087d89 to
2d4f068
Compare
September 13, 2026 17:22
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Tasks
This section is for Byron only. Models continuing this PR must not add, remove, check, uncheck, rename, or reorder checkboxes here.
Everything below this line was generated by Codex GPT-6.
Created by Codex on behalf of Byron. Byron will review before this is ready to merge.
Git rejects bare config keys followed by # or ; comments. GitPython previously accepted those lines with the comment in the option name, and an = or : inside the comment could make the line look like an assignment.
Exclude comment markers from the shared option-name expression and require a full-line match for valueless options. Reading or editing these entries now raises ParsingError. Ordinary bare keys retain their implicit true value.
Validation
Git reference
Inspected Git checkout 1630431f326e15fcde608827b5ff38422528eb59, particularly config.c:get_value. Without an assignment, Git requires the line to end after the key and optional whitespace. Git 2.50.1 (Apple Git-155) rejects all six regression inputs with exit status 128.
Reported issue
The compatibility requirement was clarified to match Git's rejection of these entries. Original report:
Follow-up to PR 2238:
This matches the valueless-option regex against the unprocessed line, so a valid Git entry such as enabled # comment (or enabled ; comment) is stored with the comment text in the option name instead of as enabled. A write-back then emits that text as part of the key, and getboolean("flag", "enabled") still cannot find the option; strip the unquoted comment before extracting the name and add a regression for it.