Re: [PATCH] git-p4: Fix git-p4.mapUser on Windows
- From
Lars Schneider <larsxschneider@gmail.com>
- Date
- Jan 21, 2017, 13:32 UTC
- Message-ID
- <A7425283-9C32-4AE8-A442-11B7CFEAB4E8@gmail.com>
- In-Reply-To
- <01020159c0e82598-e373cf0d-2bad-41bb-b455-6896ad183e22-000000@eu-west-1.amazonses.com>
Show 30 quoted lines
> 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...
Show 7 quoted lines
> 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 :-)
Show 15 quoted lines
> 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