Re: [PATCH v3 2/2] Windows: do not treat a path with backslashes as a remote's nick name

From: Junio C Hamano <gitster@pobox.com>
Date: May 25, 2017, 23:16 UTC
Message-ID: <xmqq37bseddy.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <10ee6b91-cd54-d43a-4cfb-d3baa2af7e7a@kdbg.org>

Johannes Sixt <j6t@kdbg.org> writes:

Show 8 quoted lines

>>> So in short:
>>>
>>>   (1) Hannes's patches are good, but they solve a problem that is
>>>       different from what their log messages say; the log message
>>>       needs to be updated;
>
> I do not resend patch 1/2 as it is unchanged in all regards. This 2/2
> changes the justification; patch text is unchanged.

Thanks. I think this is explained better. Complaints against fopen() warnings sounded as if we should avoid attempting to open a file that may result in _any_ failure, which I felt was misleading, but it is not a huge issue.

So how do we want to proceed on the point (2), i.e. updating the "warn on _unexpected_ errors from fopen" series to make it aware of the EINVAL we can expect on Windows? My primary question is if all EINVAL we could ever see on Windows after open/fopen returns an error is because the pathname the caller gave is not liked by the filesystem (hence we also know that the path does not exist).

If that is the case, then the "workaround" patch I sent would be an OK approach (even though I do not know what to write after #ifdef and I suspect that is not "WINDOWS". We would want to cover the one you use, the one Dscho releases and possibly the cygwin build).

If we can see EINVAL after open/fopen error that is _not_ expected and indicates a failure that is worth reporting to the user (just like we want to report e.g. I/O or permission errors), I think Windows folks are in a better position than I am to decide between that approach and a patch at lower level (e.g. teach open/fopen not to give EINVAL and instead give ENOENT when appropriate).

Show 21 quoted lines

>  remote.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/remote.c b/remote.c
> index ad6c5424ed..1949882c10 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -645,7 +645,12 @@ static int valid_remote_nick(const char *name)
>  {
>  	if (!name[0] || is_dot_or_dotdot(name))
>  		return 0;
> -	return !strchr(name, '/'); /* no slash */
> +
> +	/* remote nicknames cannot contain slashes */
> +	while (*name)
> +		if (is_dir_sep(*name++))
> +			return 0;
> +	return 1;
>  }
>  
>  const char *remote_for_branch(struct branch *branch, int *explicit)