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

["George Vanburgh" <george@vanburgh.me>]

> On 22 Jan 2017, at 13:05, Luke Diamand <luke@diamand.org>
> 
> I'm confused....see below.

That now makes two of us! 
I think I've figured out where I messed up, see below.

> 
> On 21 January 2017 at 15:21, George Vanburgh <george@vanburgh.me>
> wrote:
> >> 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.
> >> >
> >> > 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.
> >
> >>
> >>
> >> > 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.
> 
> Have you tried a 2.6 Python with splitlines? I just tried this code and it seems
> fine.
> 
> > val = s.strip().splitlines()
> > print("splitlines:", val)
> 
> Tried with 2.7 and 2.6, and bot print out an array of strings returned from
> read_pipe().
> 
> And 'grep -r splitlines' on the Python2.6 source has lots of hits.

Ah - it appears I was looking in the wrong part of the Python 
documentation - which is what lead me to think this. 
From the Python 2.4 documentation:

https://docs.python.org/release/2.4/lib/string-methods.html

Splitlines is a built-in method, not part of the string class.

> george@TEMPEST:~# /home/george/.pyenv/versions/2.4.1/bin/python -c 'import string
> print("test1\ntest2\r\ntest3".splitlines())'
> ['test1', 'test2', 'test3']

> 
> >>
> >> 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?
> 
> I agree that we have to stop supporting 2.6 at some point (and start
> supporting 3.x, but that's another world of hurt). But does 2.6 really not have
> splitlines?

The minimum supported version is currently Python 2.4, no?

> 
> If you send a patch with splitlines I can try it out (although I guess it could be
> broken under Windows).

Given that there doesn't _actually_ seem to be an issue with using splitlines in
2.4 (sorry for the confusion!) - I'll test out a patch using splitlines on 
Windows, and resubmit =]

> 
> Luke