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

From: George Vanburgh <george@vanburgh.me>
Date: Jan 21, 2017, 15:21 UTC
Message-ID: <000f01d273fa$05358ee0$0fa0aca0$@vanburgh.me>
In-Reply-To: <A7425283-9C32-4AE8-A442-11B7CFEAB4E8@gmail.com>

Show 8 quoted lines

> On 21 Jan 2017, at 13:33, 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.

Show 27 quoted lines

> >
> > 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...

It appears I was mistaken - the successful tests I ran were actually under the Ubuntu subsystem for Windows, which (obviously) passed.

Just did a quick experiment:

Git Bash (MinGW): georg@TEMPEST MINGW64 ~ $ python -c "import os print(repr(os.linesep))" '\r\n'

Powershell: PS C:\Users\georg> python -c "import os

>> print(repr(os.linesep))"

'\r\n'

Ubuntu subsystem for Windows: george@TEMPEST:~$ python -c "import os print(repr(os.linesep))" '\n'

So this issue applies to git-p4 running under both PowerShell and MinGW.

Show 11 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.

Now I feel old...

> Therefore, I would vote to
> bump the minimum version. But that's just my opinion :-)

I feel like splitlines is the better/safer fix - but figured bumping the minimum Python version was probably part of a wider discussion. If it's something people are comfortable with - I'd be happy to rework the fix to use splitlines. Luke - do you have any thoughts on this?

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)

Show 10 quoted lines

> > -        _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