Re: [PATCH] git-p4: Fix git-p4.mapUser on Windows

[Lars Schneider <larsxschneider@gmail.com>]

> On 21 Jan 2017, at 13:02, George Vanburgh <george@vanburgh.me> wrote:
> 
> From: George Vanburgh <gvanburgh@bloomberg.net>
> 
> When running git-p4 on Windows, with multiple git-p4.mapUser entries in
> git config - no user mappings are applied to the generated repository.
> 
> Reproduction Steps:
> 
> 1. Add multiple git-p4.mapUser entries to git config on a Windows
>   machine
> 2. Attempt to clone a p4 repository
> 
> None of the user mappings will be applied.
> 
> This issue is caused by the fact that gitConfigList,
> uses split(os.linesep) to convert the output of
> git config --get-all into a list.
> 
> On Windows, os.linesep is equal to '\r\n' - however git.exe
> returns configuration with a line seperator of '\n'. This leads to
> the list returned by gitConfigList containing only one element - which
> contains the full output of git config --get-all in string form. This
> causes problems for the code introduced to getUserMapFromPerforceServer
> in 10d08a1.
> 
> This issue should be caught by the test introduced in 10d08a1, and
> would require running on Windows to reproduce. When running inside
> MinGW/Cygwin, however, os.linesep correctly returns '\n', and everything
> works as expected.

This surprises me. I would expect `\r\n` in a MinGW env...
Nevertheless, I wouldn't have caught that as I don't run the git-p4 tests
on Windows...


> The simplest fix for this issue would be to convert the line split logic
> inside gitConfigList to use splitlines(), which splits on any standard
> line delimiter. However, this function was only introduced in Python
> 2.7, and would mean a bump in the minimum required version of Python
> required to run git-p4. The alternative fix, implemented here, is to use
> '\n' as a delimiter, which git.exe appears to output consistently on
> Windows anyway.

Well, that also means if we ever use splitlines() then your fix below
would brake the code, right?

Python 2.7 was released 7 years ago in 2010. Therefore, I would vote to
bump the minimum version. But that's just my opinion :-)


> Signed-off-by: George Vanburgh <gvanburgh@bloomberg.net>
> ---
> git-p4.py | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/git-p4.py b/git-p4.py
> index f427bf6..c134a58 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -656,7 +656,7 @@ def gitConfigInt(key):
> def gitConfigList(key):
>     if not _gitConfig.has_key(key):
>         s = read_pipe(["git", "config", "--get-all", key], ignore_error=True)
> -        _gitConfig[key] = s.strip().split(os.linesep)
> +        _gitConfig[key] = s.strip().split("\n")

I can't easily reproduce this as I don't have a running git-p4 setup on Windows.
However, your explanation and your fix make sense to me. If we don't want to bump
the version then this looks good to me.

Cheers,
Lars