◐ Shell
reader mode source ↗
Skip to content

Fix IndexError in GitConfigParser When a Quoted Config Value Contains a Trailing New Line#1908

Merged
Byron merged 3 commits into
gitpython-developers:mainfrom
DaveLak:fix-issue-1887
Apr 26, 2024
Merged

Fix IndexError in GitConfigParser When a Quoted Config Value Contains a Trailing New Line#1908
Byron merged 3 commits into
gitpython-developers:mainfrom
DaveLak:fix-issue-1887

Conversation

@DaveLak

@DaveLak DaveLak commented Apr 22, 2024

Copy link
Copy Markdown
Contributor

Fixes: #1887

Improve the guarding if check in GitConfigParser's string_decode function to safely handle empty strings and prevent IndexErrors when accessing string elements.

This resolves an IndexError in the GitConfigParser'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 when
accessing string elements.

This resolves an IndexError in the `GitConfigParser`'s `.read()`
method when the config file contains a quoted value containing a
trailing new line.

Fixes:
gitpython-developers#1887

@Byron Byron left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

Thanks a lot for this improvement!

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

@EliahKagan EliahKagan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

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 are two (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.

@Byron

Byron commented Apr 26, 2024

Copy link
Copy Markdown
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 are two (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 the gitoxide 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.

@Byron Byron merged commit 82bb3bb into gitpython-developers:main Apr 26, 2024
@DaveLak DaveLak deleted the fix-issue-1887 branch April 29, 2024 00:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Unhandled IndexError when calling .read() on a malformed config file

3 participants