From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Mon, 22 May 2017 11:56:55 GMT
Subject: Re: [PATCH 2/2] Windows: do not treat a path with backslashes as a remote's nick name
Message-ID: <alpine.DEB.2.21.1.1705221353570.3610@virtualbox>
In-Reply-To: <6347731f26396b1256070e02376bf3623bfb7102.1495261020.git.j6t@kdbg.org>

Hi Hannes,

On Sat, 20 May 2017, Johannes Sixt wrote:

> Fetching from a remote using a native Windows path produces these warnings:
> 
> C:\Temp\gittest>git fetch C:\Temp\gittest
> warning: unable to access '.git/remotes/C:\Temp\gittest': Invalid argument
> warning: unable to access '.git/branches/C:\Temp\gittest': Invalid argument
> From C:\Temp\gittest
>  * branch            HEAD       -> FETCH_HEAD
> 
> The functions that read the branches and remotes files are protected by
> a valid_remote_nick() check. Its implementation does not take Windows
> paths into account, so that the caller simply concatenates the paths,
> leading to the error seen above.
> 
> Use is_dir_sep() to check for both slashes and backslashes on Windows.
> 
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>

I like this explanation.

> diff --git a/remote.c b/remote.c
> index 9584af366d..9501357c06 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -649,7 +649,10 @@ static int valid_remote_nick(const char *name)
>  {
>  	if (!name[0] || is_dot_or_dotdot(name))
>  		return 0;
> -	return !strchr(name, '/'); /* no slash */
> +	while (*name)
> +		if (is_dir_sep(*name++))	/* no slash */

I understand that you simply kept the comment, yet I have to admit that I
find it confusing in the post image. Something like

	/* remote nicknames cannot contain slashes */

(and putting it *above* the `if` line instead of appending it separated by
a <tab>) would have made it easier on me.

> +			return 0;
> +	return 1;
>  }
>  

With my suggested change or without it, ACK.

Ciao,
Dscho