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

On 22 Jan 2017, at 13:05, Luke Diamand [off-list ref]

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 [off-list ref]
wrote:

quoted

quoted

On 21 Jan 2017, at 13:33, Lars Schneider [off-list ref]

quoted

On 21 Jan 2017, at 13:02, George Vanburgh [off-list ref]

wrote:

quoted

From: George Vanburgh <redacted>

When running git-p4 on Windows, with multiple git-p4.mapUser
entries in git config - no user mappings are applied to the
generated

repository.

quoted

quoted

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

quoted

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

quoted

quoted

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.

quoted

quoted

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.

quoted

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']

quoted

quoted

Well, that also means if we ever use splitlines() then your fix below

would

quoted

brake the code, right?

Python 2.7 was released 7 years ago in 2010.

Now I feel old...

:-)

quoted

quoted

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