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

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

From
René Scharfe <l.s.r@web.de>
Date
Dec 19, 2013, 23:32 UTC
Message-ID
<52B38213.2070702@web.de>
In-Reply-To
<1387378437-20646-1-git-send-email-pclouds@gmail.com>
Am 18.12.2013 15:53, schrieb Nguyễn Thái Ngọc Duy:
Show 10 quoted lines
> 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
Previous: Junio C HamanoNext: Duy Nguyen
Message 27 of 31 in “Hard coded string length cleanup”
  1. 00/12 Hard coded string length cleanupNguyễn Thái Ngọc Duy, Dec 18, 2013
  2. 01/12 Make starts_with() a wrapper of skip_prefix()Nguyễn Thái Ngọc Duy, Dec 18, 2013
  3. Junio C HamanoDec 18, 2013
  4. Junio C HamanoDec 18, 2013
  5. 02/12 Convert starts_with() to skip_prefix() for option parsingNguyễn Thái Ngọc Duy, Dec 18, 2013
  6. Johannes SixtDec 20, 2013
  7. Jeff KingDec 20, 2013
  8. Christian CouderDec 20, 2013
  9. René ScharfeDec 20, 2013
  10. Junio C HamanoDec 20, 2013
  11. Duy NguyenDec 21, 2013
  12. Junio C HamanoDec 26, 2013
  13. Jeff KingDec 28, 2013
  14. 03/12 Add and use skip_prefix_defval()Nguyễn Thái Ngọc Duy, Dec 18, 2013
  15. Kent R. SpillnerDec 18, 2013
  16. Junio C HamanoDec 18, 2013
  17. 04/12 Replace some use of starts_with() with skip_prefix()Nguyễn Thái Ngọc Duy, Dec 18, 2013
  18. 05/12 Convert a lot of starts_with() to skip_prefix()Nguyễn Thái Ngọc Duy, Dec 18, 2013
  19. 06/12 fetch.c: replace some use of starts_with() with skip_prefix()Nguyễn Thái Ngọc Duy, Dec 18, 2013
  20. 07/12 connect.c: replace some use of starts_with() with skip_prefix()Nguyễn Thái Ngọc Duy, Dec 18, 2013
  21. 08/12 refs.c: replace some use of starts_with() with skip_prefix()Nguyễn Thái Ngọc Duy, Dec 18, 2013
  22. 09/12 diff.c: reduce code duplication in --stat-xxx parsingNguyễn Thái Ngọc Duy, Dec 18, 2013
  23. 10/12 environment.c: replace starts_with() in strip_namespace() with skip_prefix()Nguyễn Thái Ngọc Duy, Dec 18, 2013
  24. 11/12 diff.c: convert diff_scoreopt_parse to use skip_prefix()Nguyễn Thái Ngọc Duy, Dec 18, 2013
  25. 12/12 refs.c: use skip_prefix() in prune_ref()Nguyễn Thái Ngọc Duy, Dec 18, 2013
  26. Junio C HamanoDec 18, 2013
  27. René ScharfeDec 19, 2013
  28. Duy NguyenDec 19, 2013
  29. René ScharfeDec 20, 2013
  30. Duy NguyenDec 20, 2013
  31. Junio C HamanoDec 20, 2013

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.