René's URL Explorer Experiment

Title: Don't remove quotes if `\` or `"` are present inside by EliahKagan · Pull Request #2048 · gitpython-developers/GitPython · GitHub

Open Graph Title: Don't remove quotes if `\` or `"` are present inside by EliahKagan · Pull Request #2048 · gitpython-developers/GitPython

X Title: Don't remove quotes if `\` or `"` are present inside by EliahKagan · Pull Request #2048 · gitpython-developers/GitPython

Description: Background #2035 fixed issue #1923, where the ConfigParser would not remove the quotes around single-line values. As discussed in comments there: That improved the common cases where quote removal is all that is needed, in particular where no escape sequences are present. When there are escape sequences within a single line quoted value, that was already not handled correctly--since no single-line quoted values (other than a quoted empty string) had been handled correctly before #2035, in that quote removal was never performed for them. Nonetheless, #2035 may have made the situation worse for some applications if they handled the quoted values themselves, or if the quoted values were more readily recognized to mean parsing had not succeeded. Let's take the best of both worlds (so far) This PR keeps the changes from #2035 in the case that they work because the text contained strictly between the beginning and ending " characters contains neither any \ nor any other ". This both: Preserves the benefit of #2035 for #1923. (The only exception is cases where a \ is meant to be preserved rather than treated as an escape character. This is presumably rare--if it ever happens--since that's not the syntax of double-quoted values in Git config files.) Keeps the old, potentially safer behavior of doing no transformations, not even quote removal, in cases where quote removal alone would clearly not produce the correct result. Changes c8e4aa0 refactors to prepare for the other changes. 7bcea08 adds a test for the behavior described above. f2b8041 makes those tests pass. But it can get better than this This is not intended as a long-term alternative to parsing escape sequences. The idea in #2035 (comment) of handling them is good, and this is not meant to discourage or interfere with that. The new test fixture and test can be modified accordingly. See the docstring and comments in test_config_with_quotes_containing_escapes. For review It seems to me that the idea here is sound, since it restores the main branch to a state where no changes are expected to produce problems for programs and libraries that use GitPython, if a patch release were to be made. But even if I am right to think that, there are a few reasons it may be useful to have a review here before merging: I'm less familiar with the config parser than most of the rest of the code of GitPython, so there could be design subtleties I am missing. There are multiple reasonable ways to make this change, and you may have input, stylistically or otherwise. It's easy to introduce bugs when making nontrivial (even if small) changes to parsing logic. (This follows #2046 and #2047, which followed #2035 and #2036.)

Open Graph Description: Background #2035 fixed issue #1923, where the ConfigParser would not remove the quotes around single-line values. As discussed in comments there: That improved the common cases where quote removal...

X Description: Background #2035 fixed issue #1923, where the ConfigParser would not remove the quotes around single-line values. As discussed in comments there: That improved the common cases where quote removal...

Opengraph URL: https://github.com/gitpython-developers/GitPython/pull/2048

X: @github

direct link

Domain: github.com

Links:

Viewport: width=device-width