"Johannes Schindelin via GitGitGadget" <gitgitgadget@xxxxxxxxx>
writes:
> From: Johannes Schindelin <johannes.schindelin@xxxxxx>
>
> 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@xxxxxx>
> ---
> 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.
