Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

FixIndexError inGitConfigParser When a Quoted Config Value Contains a Trailing New Line#1908

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
Byron merged 3 commits intogitpython-developers:mainfromDaveLak:fix-issue-1887
Apr 26, 2024

Conversation

DaveLak
Copy link
Contributor

Fixes:#1887

Improve the guardingif check inGitConfigParser'sstring_decode function to safely handle empty strings and preventIndexErrors when accessing string elements.

This resolves an IndexError in theGitConfigParser's.read() method when the config file contains a quoted value ending with a new line.

Improve the guarding `if` check in `GitConfigParser`'s `string_decode`function to safely handle empty strings and prevent `IndexError`s whenaccessing string elements.This resolves an IndexError in the `GitConfigParser`'s `.read()`method when the config file contains a quoted value containing atrailing new line.Fixes:gitpython-developers#1887
Copy link
Member

@ByronByron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thanks a lot for this improvement!

I will merge right after CI is green after applying my tiny patch.

DaveLak reacted with thumbs up emoji
Copy link
Member

@EliahKaganEliahKagan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I've suggested a small change that I think improves clarity.

Separately from the changes here but relevant to the logic they are a part of, I'm a bit worried about the case where there aretwo (or more) trailing backslashes, so that removing one of them still leaves a trailing backslash. However, that was present before, and this PR definitely need not be expanded to address it! I'm also not certain I'm right to be worried about that; maybe it is less important to cover, or a part of a separate class of malformed configuration file inputs that are intended to trigger decoding errors.

DaveLak reacted with thumbs up emoji
@Byron
Copy link
Member

I've suggested a small change that I think improves clarity.

Separately from the changes here but relevant to the logic they are a part of, I'm a bit worried about the case where there aretwo (or more) trailing backslashes, so that removing one of them still leaves a trailing backslash. However, that was present before, and this PR definitely need not be expanded to address it! I'm also not certain I'm right to be worried about that; maybe it is less important to cover, or a part of a separate class of malformed configuration file inputs that are intended to trigger decoding errors.

Thanks for taking another look - admittedly I forgot to merge this PR. Cygwin is the bane of this CI as it takes unnatural amounts of time :/.

Having worked on thegitoxide version of this I can say that this implementation here is more hole than cheese (if that makes sense :)), but it seems to work well enough for the common cases, still. Since GitPython is in maintenance mode, I don't think there is any need to try to improve it beyond fixing bugs.

DaveLak reacted with thumbs up emoji

@ByronByron merged commit82bb3bb intogitpython-developers:mainApr 26, 2024
26 checks passed
@@ -452,7 +452,7 @@ def _read(self, fp: Union[BufferedReader, IO[bytes]], fpname: str) -> None:
e = None # None, or an exception.

def string_decode(v: str) -> str:
if v[-1] =="\\":
if v and v.endswith("\\"):

This comment was marked as resolved.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Sorry, for some reason I didn't seethat comment! That addresses this proactively and my above comment can be disregarded entirely.

If the code is modified further in the future then thestr annotation, which is inconsistent with aNone value, could be examined and potentially changed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

By the way, I tried to enable branch protections which should enable auto-merge, and thus resolve the issue with long-running CI sometimes preventing a merge in addition to me forgetting to do it later. It didn't yet work for this PR, but I will see for the next one.

EliahKagan and DaveLak reacted with thumbs up emoji
@DaveLakDaveLak deleted the fix-issue-1887 branchApril 29, 2024 00:34
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@ByronByronByron approved these changes

@EliahKaganEliahKaganEliahKagan left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unhandled IndexError when calling .read() on a malformed config file
3 participants
@DaveLak@Byron@EliahKagan

[8]ページ先頭

©2009-2025 Movatter.jp