Re: [PATCH 10/11] relative_url(): fix incorrect condition

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 16, 2022, 16:41 UTC
Message-ID: <xmqqwndgv9vv.fsf@gitster.g>
In-Reply-To: <0bf70e65d2c9e187203a77088ff0f7d18510caca.1655336146.git.gitgitgadget@gmail.com>

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 29 quoted lines

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> In 63e95beb085c (submodule: port resolve_relative_url from shell to C,
> 2016-04-15), we added a loop over `url` where we are looking for `../`
> or `./` components.
>
> The loop condition we used is the pointer `url` itself, which is clearly
> not what we wanted.
>
> Pointed out by Coverity.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  remote.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/remote.c b/remote.c
> index 9b9bbfe80ec..bded6acbfe8 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -2846,7 +2846,7 @@ char *relative_url(const char *remote_url, const char *url,
>  	 * When the url starts with '../', remove that and the
>  	 * last directory in remoteurl.
>  	 */
> -	while (url) {
> +	while (*url) {
>  		if (starts_with_dot_dot_slash_native(url)) {
>  			url += 3;
>  			colonsep |= chop_last_dir(&remoteurl, is_relative);

		} else if (starts_with_dot_slash_native(url))
			url += 2;
		else
			break;
	}

Looking at the "problematic" code again, the original is bad in the sense that it is utterly misleading, but not quite "incorrect", I would think.

In other words, what the original wanted to write is an infinite loop that uses an explicit "break" to terminate the iteration, so we should have written "while (1)" in the first place.

We know that earlier part of the function already depends on url being not NULL, and nothing in the loop makes it NULL. *url that is NUL would make the two starts_with_frotz() tests fail and causes the loop to terminate by hitting "break", so while it is correct to check *url, it is wasting cycles.

But this patch is not wrong per-se, so I still have it in my tree ;-)

Thanks.