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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 16, 2022, 17:55 UTC
Message-ID: <xmqqr13otrv6.fsf@gitster.g>
In-Reply-To: <220616.86k09gwx6i.gmgdl@evledraar.gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Show 40 quoted lines

>> -	while (url) {
>> +	while (*url) {
>>  		if (starts_with_dot_dot_slash_native(url)) {
>>  			url += 3;
>>  			colonsep |= chop_last_dir(&remoteurl, is_relative);
>
> Which I tested with this:
> 	
> 	diff --git a/remote.c b/remote.c
> 	index 9b9bbfe80ec..e049bbb791c 100644
> 	--- a/remote.c
> 	+++ b/remote.c
> 	@@ -2846,14 +2846,17 @@ 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 (1) {
> 	 		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))
> 	+		} else if (starts_with_dot_slash_native(url)) {
> 	 			url += 2;
> 	-		else
> 	-			break;
> 	+		} else if (!*url) {
> 	+			BUG("ran off the end of our url?");
> 	+		} else {
> 	+			break; 
> 	+		}
> 	 	}
> 	 	strbuf_reset(&sb);
> 	 	strbuf_addf(&sb, "%s%s%s", remoteurl, colonsep ? ":" : "/", url);
> 	
> Which will fail e.g. on this test in t3420-rebase-autostash.sh:
>
> 	+ git submodule add ./ sub
> 	BUG: remote.c:2856: ran off the end of our url?
> 	Aborted

Sorry, but I am confused. I do not see the point of that test before the BUG.

I agree that this loop over url is "we want to loop forever, and stop immediately when url does not start with ../ or ./" infinite loop, and it is better written with "while(1)". If you do not make any other changes that confuses me, we'll get this:

-	while (*url) {
+	while (1) {
		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;
	}

We know url upon entry is not NULL. starts_with_dot_*() ends up calling dir.c:path_match_flags() and the first thing it does is to return if the byte is not '.'. So the difference is whether you leave the loop with the "while" condition, or you fail the two starts_with_dot_*() calls and hit the "break" at the end in the loop.

IOW, running of the end of the URL is not a BUG, is it?