From: "George Vanburgh" <george@vanburgh.me>
To: "'Lars Schneider'" <larsxschneider@gmail.com>
Cc: "'Git mailing list'" <git@vger.kernel.org>,
"'Luke Diamand'" <luke@diamand.org>
Subject: RE: [PATCH] git-p4: Fix git-p4.mapUser on Windows
Date: Sat, 21 Jan 2017 15:21:20 -0000 [thread overview]
Message-ID: <000f01d273fa$05358ee0$0fa0aca0$@vanburgh.me> (raw)
In-Reply-To: <A7425283-9C32-4AE8-A442-11B7CFEAB4E8@gmail.com>
> 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?
>
>
> > 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
next prev parent reply other threads:[~2017-01-21 15:21 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-01-21 12:02 [PATCH] git-p4: Fix git-p4.mapUser on Windows George Vanburgh
2017-01-21 13:32 ` Lars Schneider
2017-01-21 15:21 ` George Vanburgh [this message]
2017-01-22 13:04 ` Luke Diamand
2017-01-22 14:14 ` George Vanburgh
2017-01-25 9:17 ` [PATCH v2] " George Vanburgh
2017-01-27 17:33 ` Junio C Hamano
2017-01-29 17:10 ` Luke Diamand
2017-01-30 17:07 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='000f01d273fa$05358ee0$0fa0aca0$@vanburgh.me' \
--to=george@vanburgh.me \
--cc=git@vger.kernel.org \
--cc=larsxschneider@gmail.com \
--cc=luke@diamand.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).