Re: [PATCH] submodule--helper: normalize funny urls

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 18, 2016, 19:49 UTC
Message-ID: <xmqq8ttlphd7.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20161018175247.28326-1-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 24 quoted lines

> Some users may rely on this by always cloning with '/.' and having
> an additional '../' in the relative path for the submodule, and this
> patch breaks them. So why introduce this patch?
>
> The fix in c12922024 (submodule: ignore trailing slash on superproject
> URL, 2016-10-10) and prior discussion revealed, that Git and Git
> for Windows treat URLs differently, as currently Git for Windows
> strips off a trailing dot from paths when calling a Git binary
> unlike when running a shell. Which means Git for Windows is already
> doing the right thing for the case mentioned above, but it would fail
> our current tests, that test for the broken behavior and it would
> confuse users working across platforms. So we'd rather fix it
> in Git to ignore any of these trailing no ops in the path properly.
>
> We never produce the URLs with a trailing '/.' in Git ourselves,
> they come to us, because the user used it as the URL for cloning
> a superproject. Normalize these paths.
>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
>  * reworded the commit message, taken from Junio, but added more explanation
>    why we want to introduce this patch.

The additional explanation is very good.

Show 11 quoted lines

> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 260f46f..ac03cb3 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -76,6 +76,29 @@ static int chop_last_dir(char **remoteurl, int is_relative)
>  	return 0;
>  }
>  
> +static void strip_url_ending(char *url, size_t *_len)
> +{
> +	size_t len = _len ? *_len : strlen(url);

Stare at our codebase and you'd notice that we avoid using names with leading underscore deliberately and use trailing one instead when we name a throw-away name like this. Let's do the same here. I.e.

	static void strip_url_ending(char *url, size_t *len_)
	{
		size_t len = len_ ? *len_ : strlen(url);

> +	for (;;) {
> +		if (len > 1 && is_dir_sep(url[len-2]) && url[len-1] == '.') {
> +			url[len-2] = '\0';

"len-1" and "len-2" are usually spelled with SP on both sides of binary operators.

Show 6 quoted lines

> +			len -= 2;
> +			continue;
> +		}
> +		if (len > 0 && is_dir_sep(url[len-1])) {
> +			url[len-1] = '\0';
> +			len --;

And post-decrement sticks to whatever it is decrementing without SP.

> +			continue;
> +		}