git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] submodule: Port resolve_relative_url from shell to C

From
Johannes Sixt <j6t@kdbg.org>
Date
Jan 14, 2016, 20:57 UTC
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:
Show 28 quoted lines
> 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.

Show 10 quoted lines
>
> 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...
Show 16 quoted lines
>> +	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
Previous: Junio C HamanoNext: Stefan Beller
Message 9 of 12 in “submodule: Port resolve_relative_url from shell to C”
  1. submodule: Port resolve_relative_url from shell to CStefan Beller, Jan 13, 2016
  2. Junio C HamanoJan 13, 2016
  3. Stefan BellerJan 13, 2016
  4. Jens LehmannJan 14, 2016
  5. Stefan BellerJan 14, 2016
  6. Junio C HamanoJan 15, 2016
  7. Stefan BellerJan 15, 2016
  8. Junio C HamanoJan 15, 2016
  9. Johannes SixtJan 14, 2016
  10. Stefan BellerJan 14, 2016
  11. Eric SunshineJan 13, 2016
  12. Stefan BellerJan 13, 2016

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.