Re: [PATCH 00/12] Hard coded string length cleanup

["René Scharfe" <l.s.r@web.de>]

Am 18.12.2013 15:53, schrieb Nguyễn Thái Ngọc Duy:
> I reimplemented skip_prefix() again just to realize this function
> already exists. Which reminds me there are a bunch of places that
> could benefit from this function, the same reason that I wanted to
> reimplement it.
> 
> So this is series to make it more popular (so hopefully I'll see it
> used somewhere and know that it exists) and the code cleaner. The
> pattern "compare a string, then skip the compared part by a hard coded
> string length" is almost killed. I left a few in places for those who
> want to contribute :)

Good idea.

Seeing that skip_prefix_defval is mostly used in the form
skip_prefix_defval(foo, prefix, foo) I wonder if it makes sense to
first change skip_prefix to return the full string instead of NULL
if the prefix is not matched.  Would the resulting function cover
most use cases?  And would it still be easily usable?

---
 advice.c                   |  2 ++
 builtin/branch.c           |  6 +++---
 builtin/clone.c            |  6 ++++--
 builtin/commit.c           |  6 ++----
 builtin/fmt-merge-msg.c    |  6 +++---
 builtin/push.c             |  2 --
 builtin/remote.c           | 13 +++----------
 column.c                   |  2 +-
 config.c                   |  2 +-
 credential-cache--daemon.c |  4 ++--
 credential.c               |  2 +-
 git-compat-util.h          |  7 +------
 parse-options.c            | 11 ++++++-----
 strbuf.c                   | 10 ++++++++++
 transport.c                |  6 +++++-
 urlmatch.c                 |  2 +-
 16 files changed, 45 insertions(+), 42 deletions(-)

diff --git a/advice.c b/advice.c
index 3eca9f5..1f85338 100644
--- a/advice.c
+++ b/advice.c
@@ -66,6 +66,8 @@ int git_default_advice_config(const char *var, const char *value)
 	const char *k = skip_prefix(var, "advice.");
 	int i;
 
+	if (k == var)
+		return 0;
 	for (i = 0; i < ARRAY_SIZE(advice_config); i++) {
 		if (strcmp(k, advice_config[i].name))
 			continue;
diff --git a/builtin/branch.c b/builtin/branch.c
index b4d7716..d3694d0 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -294,13 +294,13 @@ static char *resolve_symref(const char *src, const char *prefix)
 {
 	unsigned char sha1[20];
 	int flag;
-	const char *dst, *cp;
+	const char *dst;
 
 	dst = resolve_ref_unsafe(src, sha1, 0, &flag);
 	if (!(dst && (flag & REF_ISSYMREF)))
 		return NULL;
-	if (prefix && (cp = skip_prefix(dst, prefix)))
-		dst = cp;
+	if (prefix)
+		dst = skip_prefix(dst, prefix);
 	return xstrdup(dst);
 }
 
diff --git a/builtin/clone.c b/builtin/clone.c
index f98f529..79f24cd 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -578,11 +578,13 @@ static void update_remote_refs(const struct ref *refs,
 static void update_head(const struct ref *our, const struct ref *remote,
 			const char *msg)
 {
-	if (our && starts_with(our->name, "refs/heads/")) {
+	const char *head;
+
+	if (our &&
+	    ((head = skip_prefix(our->name, "refs/heads/")) != our->name)) {
 		/* Local default branch link */
 		create_symref("HEAD", our->name, NULL);
 		if (!option_bare) {
-			const char *head = skip_prefix(our->name, "refs/heads/");
 			update_ref(msg, "HEAD", our->old_sha1, NULL, 0, DIE_ON_ERR);
 			install_branch_config(0, head, option_origin, our->name);
 		}
diff --git a/builtin/commit.c b/builtin/commit.c
index 3767478..c18a77d 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -934,7 +934,7 @@ static int message_is_empty(struct strbuf *sb)
 static int template_untouched(struct strbuf *sb)
 {
 	struct strbuf tmpl = STRBUF_INIT;
-	char *start;
+	const char *start;
 
 	if (cleanup_mode == CLEANUP_NONE && sb->len)
 		return 0;
@@ -943,9 +943,7 @@ static int template_untouched(struct strbuf *sb)
 		return 0;
 
 	stripspace(&tmpl, cleanup_mode == CLEANUP_ALL);
-	start = (char *)skip_prefix(sb->buf, tmpl.buf);
-	if (!start)
-		start = sb->buf;
+	start = skip_prefix(sb->buf, tmpl.buf);
 	strbuf_release(&tmpl);
 	return rest_is_empty(sb, start - sb->buf);
 }
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 3906eda..ff34c62 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -284,7 +284,7 @@ static void credit_people(struct strbuf *out,
 			  int kind)
 {
 	const char *label;
-	const char *me;
+	const char *me, *p;
 
 	if (kind == 'a') {
 		label = "By";
@@ -297,8 +297,8 @@ static void credit_people(struct strbuf *out,
 	if (!them->nr ||
 	    (them->nr == 1 &&
 	     me &&
-	     (me = skip_prefix(me, them->items->string)) != NULL &&
-	     skip_prefix(me, " <")))
+	     (p = skip_prefix(me, them->items->string)) != me &&
+	     starts_with(p, " <")))
 		return;
 	strbuf_addf(out, "\n%c %s ", comment_line_char, label);
 	add_people_count(out, them);
diff --git a/builtin/push.c b/builtin/push.c
index a73982a..2852a46 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -91,8 +91,6 @@ static NORETURN int die_push_simple(struct branch *branch, struct remote *remote
 	const char *short_upstream =
 		skip_prefix(branch->merge[0]->src, "refs/heads/");
 
-	if (!short_upstream)
-		short_upstream = branch->merge[0]->src;
 	/*
 	 * Don't show advice for people who explicitly set
 	 * push.default.
diff --git a/builtin/remote.c b/builtin/remote.c
index b3ab4cf..1f5dfbe 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -248,14 +248,7 @@ struct branch_info {
 
 static struct string_list branch_list;
 
-static const char *abbrev_ref(const char *name, const char *prefix)
-{
-	const char *abbrev = skip_prefix(name, prefix);
-	if (abbrev)
-		return abbrev;
-	return name;
-}
-#define abbrev_branch(name) abbrev_ref((name), "refs/heads/")
+#define abbrev_branch(name) skip_prefix((name), "refs/heads/")
 
 static int config_read_branches(const char *key, const char *value, void *cb)
 {
@@ -1326,10 +1319,10 @@ static int prune_remote(const char *remote, int dry_run)
 
 		if (dry_run)
 			printf_ln(_(" * [would prune] %s"),
-			       abbrev_ref(refname, "refs/remotes/"));
+			       skip_prefix(refname, "refs/remotes/"));
 		else
 			printf_ln(_(" * [pruned] %s"),
-			       abbrev_ref(refname, "refs/remotes/"));
+			       skip_prefix(refname, "refs/remotes/"));
 		warn_dangling_symref(stdout, dangling_msg, refname);
 	}
 
diff --git a/column.c b/column.c
index 9367ba5..7de051d 100644
--- a/column.c
+++ b/column.c
@@ -337,7 +337,7 @@ int git_column_config(const char *var, const char *value,
 		      const char *command, unsigned int *colopts)
 {
 	const char *it = skip_prefix(var, "column.");
-	if (!it)
+	if (it == var)
 		return 0;
 
 	if (!strcmp(it, "ui"))
diff --git a/config.c b/config.c
index d969a5a..b787f8d 100644
--- a/config.c
+++ b/config.c
@@ -134,7 +134,7 @@ int git_config_include(const char *var, const char *value, void *data)
 		return ret;
 
 	type = skip_prefix(var, "include.");
-	if (!type)
+	if (type == var)
 		return ret;
 
 	if (!strcmp(type, "path"))
diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c
index 390f194..21aad75 100644
--- a/credential-cache--daemon.c
+++ b/credential-cache--daemon.c
@@ -110,13 +110,13 @@ static int read_request(FILE *fh, struct credential *c,
 
 	strbuf_getline(&item, fh, '\n');
 	p = skip_prefix(item.buf, "action=");
-	if (!p)
+	if (p == item.buf)
 		return error("client sent bogus action line: %s", item.buf);
 	strbuf_addstr(action, p);
 
 	strbuf_getline(&item, fh, '\n');
 	p = skip_prefix(item.buf, "timeout=");
-	if (!p)
+	if (p == item.buf)
 		return error("client sent bogus timeout line: %s", item.buf);
 	*timeout = atoi(p);
 
diff --git a/credential.c b/credential.c
index e54753c..466beff 100644
--- a/credential.c
+++ b/credential.c
@@ -41,7 +41,7 @@ static int credential_config_callback(const char *var, const char *value,
 	const char *key, *dot;
 
 	key = skip_prefix(var, "credential.");
-	if (!key)
+	if (key == var)
 		return 0;
 
 	if (!value)
diff --git a/git-compat-util.h b/git-compat-util.h
index b73916b..dcb92c4 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -354,12 +354,7 @@ extern int starts_with(const char *str, const char *prefix);
 extern int prefixcmp(const char *str, const char *prefix);
 extern int ends_with(const char *str, const char *suffix);
 extern int suffixcmp(const char *str, const char *suffix);
-
-static inline const char *skip_prefix(const char *str, const char *prefix)
-{
-	size_t len = strlen(prefix);
-	return strncmp(str, prefix, len) ? NULL : str + len;
-}
+extern const char *skip_prefix(const char *str, const char *prefix);
 
 #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)
 
diff --git a/parse-options.c b/parse-options.c
index 7b8d3fa..4ec2fa3 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -240,7 +240,7 @@ static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg,
 again:
 		rest = skip_prefix(arg, long_name);
 		if (options->type == OPTION_ARGUMENT) {
-			if (!rest)
+			if (rest == arg)
 				continue;
 			if (*rest == '=')
 				return opterror(options, "takes no value", flags);
@@ -249,7 +249,7 @@ again:
 			p->out[p->cpidx++] = arg - 2;
 			return 0;
 		}
-		if (!rest) {
+		if (rest == arg) {
 			/* abbreviated? */
 			if (!strncmp(long_name, arg, arg_end - arg)) {
 is_abbreviated:
@@ -289,10 +289,11 @@ is_abbreviated:
 			flags |= OPT_UNSET;
 			rest = skip_prefix(arg + 3, long_name);
 			/* abbreviated and negated? */
-			if (!rest && starts_with(long_name, arg + 3))
-				goto is_abbreviated;
-			if (!rest)
+			if (rest == arg + 3) {
+				if (starts_with(long_name, arg + 3))
+					goto is_abbreviated;
 				continue;
+			}
 		}
 		if (*rest) {
 			if (*rest != '=')
diff --git a/strbuf.c b/strbuf.c
index 83caf4a..222df13 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -37,6 +37,16 @@ int suffixcmp(const char *str, const char *suffix)
 		return strcmp(str + len - suflen, suffix);
 }
 
+const char *skip_prefix(const char *str, const char *prefix)
+{
+	const char *p;
+	for (p = str; ; p++, prefix++)
+		if (!*prefix)
+			return p;
+		else if (*p != *prefix)
+			return str;
+}
+
 /*
  * Used as the default ->buf value, so that people can always assume
  * buf is non NULL and ->buf is NUL terminated even for a freshly
diff --git a/transport.c b/transport.c
index 824c5b9..8d3372f 100644
--- a/transport.c
+++ b/transport.c
@@ -191,7 +191,11 @@ static void set_upstreams(struct transport *transport, struct ref *refs,
 
 static const char *rsync_url(const char *url)
 {
-	return !starts_with(url, "rsync://") ? skip_prefix(url, "rsync:") : url;
+	if (!starts_with(url, "rsync://")) {
+		const char *rest = skip_prefix(url, "rsync:");
+		url = (rest == url) ? NULL : rest;
+	}
+	return url;
 }
 
 static struct ref *get_refs_via_rsync(struct transport *transport, int for_push)
diff --git a/urlmatch.c b/urlmatch.c
index ec87cba..51b5d23 100644
--- a/urlmatch.c
+++ b/urlmatch.c
@@ -484,7 +484,7 @@ int urlmatch_config_entry(const char *var, const char *value, void *cb)
 	int retval;
 
 	key = skip_prefix(var, collect->section);
-	if (!key || *(key++) != '.') {
+	if ((key == var) || (*(key++) != '.')) {
 		if (collect->cascade_fn)
 			return collect->cascade_fn(var, value, cb);
 		return 0; /* not interested */
-- 
1.8.5.2