git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Lars Schneider <larsxschneider@gmail.com>
Date
Jan 21, 2017, 13:32 UTC
Message-ID
<A7425283-9C32-4AE8-A442-11B7CFEAB4E8@gmail.com>
In-Reply-To
<01020159c0e82598-e373cf0d-2bad-41bb-b455-6896ad183e22-000000@eu-west-1.amazonses.com>
Show 30 quoted lines
> 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...

Show 7 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. Therefore, I would vote to bump the minimum version. But that's just my opinion :-)

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

Previous: George VanburghNext: George Vanburgh
Message 2 of 9 in “git-p4: Fix git-p4.mapUser on Windows”
  1. git-p4: Fix git-p4.mapUser on WindowsGeorge Vanburgh, Jan 21, 2017
  2. Lars SchneiderJan 21, 2017
  3. George VanburghJan 21, 2017
  4. Luke DiamandJan 22, 2017
  5. George VanburghJan 22, 2017
  6. git-p4: Fix git-p4.mapUser on WindowsGeorge Vanburgh, Jan 25, 2017
  7. Junio C HamanoJan 27, 2017
  8. Luke DiamandJan 29, 2017
  9. Junio C HamanoJan 30, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.