From: Johannes Sixt <j6t@kdbg.org>
Date: Thu, 14 Jan 2016 20:57:44 GMT
Subject: Re: [PATCH] submodule: Port resolve_relative_url from shell to C
Message-ID: <56980BC8.90506@kdbg.org>
In-Reply-To: <xmqq4mehm92b.fsf@gitster.mtv.corp.google.com>

Am 13.01.2016 um 23:03 schrieb Junio C Hamano:
> Stefan Beller <sbeller@google.com> writes:
>> +	while (url) {
>> +		if (starts_with_dot_dot_slash(url)) {
>> +			char *rfind;
>> +			url += 3;
>> +
>> +			rfind = last_dir_separator(remoteurl);
>> +			if (rfind)
>> +				*rfind = '\0';
>> +			else {
>> +				rfind = strrchr(remoteurl, ':');
>> +				if (rfind) {
>> +					*rfind = '\0';
>> +					colonsep = 1;
>> +				} else {
>> +					if (is_relative || !strcmp(".", remoteurl))
>> +						die(_("cannot strip one component off url '%s'"), remoteurl);
>> +					else
>> +						remoteurl = xstrdup(".");
>> +				}
>> +			}
>
> It is somewhat hard to see how this avoids stripping one (or both)
> slashes just after "http:" in remoteurl="http://site/path/", leaving
> just "http:/" (or "http:").
>
> This codepath has overly deep nesting levels.  Is this the simplest
> we can do?

The code as written is quite easy to follow when compared to the 
original shell code. I think that is a reasonable goal, and improvements 
can into separate patches.

>
> The final else { if .. else } can be made into else if .. else to
> dedent the overlong die() by one level, but I am wondering if the
> deep nesting is just a symptom of logic being unnecessarily complex.
>
>> +		} else if (starts_with_dot_slash(url)) {
>> +			url += 2;
>> +		} else
>> +			break;
>> +	}

For example, the section that begins here...

>> +	strbuf_reset(&sb);
>> +	strbuf_addf(&sb, "%s%s%s", remoteurl, colonsep ? ":" : "/", url);
>> +
>> +	if (starts_with_dot_slash(sb.buf))
>> +		out = xstrdup(sb.buf + 2);
>> +	else
>> +		out = xstrdup(sb.buf);
>> +	strbuf_reset(&sb);
>> +
>> +	free(remoteurl);
>> +	if (!up_path || !is_relative)
>> +		return out;
>> +
>> +	strbuf_addf(&sb, "%s%s", up_path, out);
>> +	free(out);
>> +	return strbuf_detach(&sb, NULL);


... and ends here can easily be rewritten to become a single 
strbuf_addf() without the xstrdup()s and without the early exit (at the 
cost of some additional ?: conditionals in the arguments).

-- Hannes