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