Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork938
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this 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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this 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.
Uh oh!
There was an error while loading.Please reload this page.
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 |
82bb3bb
intogitpython-developers:mainUh oh!
There was an error while loading.Please reload this page.
@@ -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.
This comment was marked as resolved.
Sorry, something went wrong.
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Fixes:#1887
Improve the guarding
if
check inGitConfigParser
'sstring_decode
function to safely handle empty strings and preventIndexError
s 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.