patchgit-p4: Fix git-p4.mapUser on Windows

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

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

> 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")

> 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

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

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

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

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

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

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

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

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

> 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.
> ...
> Using splitlines solves this issue, by splitting config on all
> typical delimiters ('\n', '\r\n' etc.)

> 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..b66f68b 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().splitlines()
>          if _gitConfig[key] == ['']:
>              _gitConfig[key] = []
>      return _gitConfig[key]
>
> --
> https://github.com/git/git/pull/319

> George Vanburgh <george@vanburgh.me> writes:
>
>> 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.
>> ...
>> Using splitlines solves this issue, by splitting config on all
>> typical delimiters ('\n', '\r\n' etc.)
>
> Luke, Lars, this version seems to be in line with the conclusion of
> your earlier reviews, e.g.
>
> <CAE5ih7_+Vc9oqKdjo8h2vgZPup4pto9wd=sBb=W6hCs4tuW2Jg@mail.gmail.com>
>
> Even though it looks OK to my eyes, I'll wait for Acks or further
> refinement suggestions from either of you two before acting on this
> patch.

>
> Thanks.
>
>> 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..b66f68b 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().splitlines()
>>          if _gitConfig[key] == ['']:
>>              _gitConfig[key] = []
>>      return _gitConfig[key]
>>
>> --
>> https://github.com/git/git/pull/319

> On 27 January 2017 at 17:33, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Luke, Lars, this version seems to be in line with the conclusion of
>> your earlier reviews, e.g.
>>
>> <CAE5ih7_+Vc9oqKdjo8h2vgZPup4pto9wd=sBb=W6hCs4tuW2Jg@mail.gmail.com>
>>
>> Even though it looks OK to my eyes, I'll wait for Acks or further
>> refinement suggestions from either of you two before acting on this
>> patch.
>
> It looks good to me. The tests all pass, and the change looks correct.