From: Johannes Sixt <j6t@kdbg.org>
To: Stefan Beller <sbeller@google.com>
Cc: git@vger.kernel.org, gitster@pobox.com, jens.lehmann@web.de
Subject: Re: [PATCHv2] submodule: Port resolve_relative_url from shell to C
Date: Thu, 17 Dec 2015 19:55:43 +0100 [thread overview]
Message-ID: <5673052F.7050000@kdbg.org> (raw)
In-Reply-To: <1450311999-3992-2-git-send-email-sbeller@google.com>
Am 17.12.2015 um 01:26 schrieb Stefan Beller:
> This reimplements the helper function `resolve_relative_url` in shell
> in C. This functionality is needed in C for introducing the groups
> feature later on. When using groups, the user should not need to run
> `git submodule init`, but it should be implicit at all appropriate places,
> which are all in C code. As the we would not just call out to `git
> submodule init`, but do a more fine grained structure there, we actually
> need all the init functionality in C before attempting the groups
> feature. To get the init functionality in C, rewriting the
> resolve_relative_url subfunction is a major step.
>
> This also improves the performance:
> (Best out of 3) time ./t7400-submodule-basic.sh
> Before:
> real 0m9.575s
> user 0m2.683s
> sys 0m6.773s
> After:
> real 0m9.293s
> user 0m2.691s
> sys 0m6.549s
>
> That's about 3%.
I appreciate this effort as it should help us on Windows. Although the
numbers (and my own timings) suggest that this is only a small step
forward. That's not surprising as the patch removes only two forks.
As to the implementation, find a patch below that removes the ifdefs
and a few other suggestions. It is a mechanical conversion without
understanding what relative_url() does. I have the gut feeling that the
two strbuf_addf towards the end of the function can be contracted and
the temporarily allocate copy in 'out' can be removed.
If there were a few examples in the comment above the function, it
would be much simpler to understand.
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index b925bed..8ec0975 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -11,6 +11,7 @@
#include "run-command.h"
#include "remote.h"
#include "refs.h"
+#include "connect.h"
static const char *get_default_remote(void)
{
@@ -31,34 +32,23 @@ static const char *get_default_remote(void)
return xstrdup(dest);
}
-static int has_same_dir_prefix(const char *str, const char **out)
+static int starts_with_dot_slash(const char *str)
{
-#ifdef GIT_WINDOWS_NATIVE
- return skip_prefix(str, "./", out)
- || skip_prefix(str, ".\\", out);
-#else
- return skip_prefix(str, "./", out);
-#endif
+ return str[0] == '.' && is_dir_sep(str[1]);
}
-static int has_upper_dir_prefix(const char *str, const char **out)
+static int starts_with_dot_dot_slash(const char *str)
{
-#ifdef GIT_WINDOWS_NATIVE
- return skip_prefix(str, "../", out)
- || skip_prefix(str, "..\\", out);
-#else
- return skip_prefix(str, "../", out);
-#endif
+ return str[0] == '.' && str[1] == '.' && is_dir_sep(str[2]);
}
-static char *last_dir_separator(const char *str)
+static char *last_dir_separator(char *str)
{
-#ifdef GIT_WINDOWS_NATIVE
- return strrchr(str, "/")
- || strrchr(str, "\\");
-#else
- return strrchr(str, '/');
-#endif
+ char* p = str + strlen(str);
+ while (p-- != str)
+ if (is_dir_sep(*p))
+ return p;
+ return NULL;
}
/*
@@ -85,9 +75,10 @@ static const char *relative_url(const char *url, const char *up_path)
size_t len;
char *remoteurl = NULL;
char *sep = "/";
- const char *out;
+ char *out;
struct strbuf sb = STRBUF_INIT;
const char *remote = get_default_remote();
+
strbuf_addf(&sb, "remote.%s.url", remote);
if (git_config_get_string(sb.buf, &remoteurl))
@@ -98,22 +89,23 @@ static const char *relative_url(const char *url, const char *up_path)
if (is_dir_sep(remoteurl[len]))
remoteurl[len] = '\0';
- if (strchr(remoteurl, ':') || is_dir_sep(*remoteurl))
+ if (!url_is_local_not_ssh(remoteurl) || is_absolute_path(remoteurl))
is_relative = 0;
- else if (has_same_dir_prefix(remoteurl, &out) ||
- has_upper_dir_prefix(remoteurl, &out))
+ else if (starts_with_dot_slash(remoteurl) ||
+ starts_with_dot_dot_slash(remoteurl))
is_relative = 1;
else {
is_relative = 1;
strbuf_reset(&sb);
strbuf_addf(&sb, "./%s", remoteurl);
+ free(remoteurl);
remoteurl = strbuf_detach(&sb, NULL);
}
while (url) {
- if (has_upper_dir_prefix(url, &out)) {
+ if (starts_with_dot_dot_slash(url)) {
char *rfind;
- url = out;
+ url += 3;
rfind = last_dir_separator(remoteurl);
if (rfind)
@@ -130,22 +122,23 @@ static const char *relative_url(const char *url, const char *up_path)
remoteurl = ".";
}
}
- } else if (has_same_dir_prefix(url, &out))
- url = out;
- else
+ } else if (starts_with_dot_slash(url)) {
+ url += 2;
+ } else
break;
}
strbuf_reset(&sb);
strbuf_addf(&sb, "%s%s%s", remoteurl, sep, url);
- if (!has_same_dir_prefix(sb.buf, &out))
- out = sb.buf;
- out = xstrdup(out);
+ if (starts_with_dot_slash(sb.buf))
+ out = strdup(sb.buf + 2);
+ else
+ out = xstrdup(sb.buf);
strbuf_reset(&sb);
strbuf_addf(&sb, "%s%s", is_relative && up_path ? up_path : "", out);
- free((char*)out);
+ free(out);
return strbuf_detach(&sb, NULL);
}
next prev parent reply other threads:[~2015-12-17 18:55 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-12-10 1:07 [PATCH] submodule: Port resolve_relative_url from shell to C Stefan Beller
2015-12-10 6:48 ` Johannes Sixt
2015-12-16 22:36 ` Stefan Beller
2015-12-17 0:26 ` [PATCHv2] Porting " Stefan Beller
2015-12-17 7:47 ` Torsten Bögershausen
2015-12-17 0:26 ` [PATCHv2] submodule: Port " Stefan Beller
2015-12-17 8:17 ` Eric Sunshine
2015-12-17 18:55 ` Johannes Sixt [this message]
2015-12-17 19:17 ` Johannes Sixt
2015-12-18 3:27 ` Jeff King
2016-01-11 20:17 ` Stefan Beller
-- strict thread matches above, loose matches on Subject: below --
2016-01-12 23:35 Stefan Beller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5673052F.7050000@kdbg.org \
--to=j6t@kdbg.org \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jens.lehmann@web.de \
--cc=sbeller@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).