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

From: Junio C Hamano <gitster@pobox.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org,
	Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: Re: [PATCH 10/11] relative_url(): fix incorrect condition
Date: Thu, 16 Jun 2022 10:55:57 -0700	[thread overview]
Message-ID: <xmqqr13otrv6.fsf@gitster.g> (raw)
In-Reply-To: <220616.86k09gwx6i.gmgdl@evledraar.gmail.com> (=?utf-8?B?IsOG?= =?utf-8?B?dmFyIEFybmZqw7Zyw7A=?= Bjarmason"'s message of "Thu, 16 Jun 2022 15:09:38 +0200")

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

>> -	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?