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

Re: [PATCH 02/12] Convert starts_with() to skip_prefix() for option parsing

From
René Scharfe <l.s.r@web.de>
Date
Dec 20, 2013, 10:43 UTC
Message-ID
<52B41F42.7050204@web.de>
In-Reply-To
<20131220070449.GA29717@sigill.intra.peff.net>
Am 20.12.2013 08:04, schrieb Jeff King:
Show 49 quoted lines
> On Fri, Dec 20, 2013 at 07:51:00AM +0100, Johannes Sixt wrote:
> 
>>>   	for (i = 1; i < argc && *argv[i] == '-'; i++) {
>>>   		const char *arg = argv[i];
>>> +		const char *optarg;
>>>   
>>> -		if (starts_with(arg, "--upload-pack=")) {
>>> -			args.uploadpack = arg + 14;
>>> +		if ((optarg = skip_prefix(arg, "--upload-pack=")) != NULL) {
>>> +			args.uploadpack = optarg;
>>
>> Quite frankly, I do not think this is an improvement. The old code is
>> *MUCH* easier to understand because "starts_with" is clearly a predicate
>> that is either true or false, but the code with "skip_prefix" is much
>> heavier on the eye with its extra level of parenthesis. That it removes a
>> hard-coded constant does not count much IMHO because it is very clear
>> where the value comes from.
> 
> Yeah, I agree that is unfortunate. Maybe we could have the best of both
> worlds, like:
> 
>    if (starts_with(arg, "--upload-pack=", &optarg))
>            ... use optarg ...
> 
> Probably we do not want to call it just "starts_with", as quite a few
> callers to do not care about what comes next, and would just pass NULL.
> 
> I cannot seem to think of a good name, though, as the "with" means that
> obvious things like "starts_with_value" naturally parse as a single
> (nonsensical) sentence.  Something like "parse_prefix" would work, but
> it is not as clearly a predicate as "starts_with" (but we have at least
> gotten rid of the extra parentheses).
> 
> Elsewhere in the thread, the concept was discussed of returning the full
> string to mean "did not match", which makes some other idioms simpler
> (but IMHO makes the simple cases like this even harder to read). My
> proposal splits the "start of string" out parameter from the boolean
> return, so it handles both cases naturally:
> 
>    /* here we care if we saw the prefix, as above */
>    if (parse_prefix(foo, prefix, &the_rest))
>        ...
> 
>    /*
>     * and here we do not care, and just want to optionally strip the
>     * prefix, and take the full value otherwise; we just have to ignore
>     * the return value in this case.
>     */
>    parse_prefix(foo, prefix, &foo);

It adds a bit of redundancy, but overall I like it. It fits the common case very well and looks nice. The patch below converts all calls of skip_prefix as well as the usage of starts_with and a magic number in builtin/fetch-pack.c.

I wonder how many of the 400+ uses of starts_with remain after a parse_prefix crusade. If only a few remain then it may make sense to unite the two functions under a common name.

---
 advice.c                   |  5 ++++-
 builtin/branch.c           |  6 +++---
 builtin/clone.c            | 13 ++++++++-----
 builtin/commit.c           |  6 ++----
 builtin/fetch-pack.c       | 14 +++++++-------
 builtin/fmt-merge-msg.c    |  4 ++--
 builtin/push.c             |  7 +++----
 builtin/remote.c           |  4 +---
 column.c                   |  5 +++--
 config.c                   |  3 +--
 credential-cache--daemon.c |  6 ++----
 credential.c               |  3 +--
 git-compat-util.h          |  1 +
 parse-options.c            | 11 ++++++-----
 strbuf.c                   | 12 +++++++++---
 transport.c                |  6 +++++-
 urlmatch.c                 |  3 +--
 17 files changed, 59 insertions(+), 50 deletions(-)
diff --git a/advice.c b/advice.c
index 3eca9f5..75fae9c 100644
--- a/advice.c
+++ b/advice.c
@@ -63,9 +63,12 @@ void advise(const char *advice, ...)
 
 int git_default_advice_config(const char *var, const char *value)
 {
-	const char *k = skip_prefix(var, "advice.");
+	const char *k;
 	int i;
 
+	if (!parse_prefix(var, "advice.", &k))
+		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..dae0d82 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)
+		parse_prefix(dst, prefix, &dst);
 	return xstrdup(dst);
 }
 
diff --git a/builtin/clone.c b/builtin/clone.c
index f98f529..e62fa26 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -578,11 +578,12 @@ 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 && parse_prefix(our->name, "refs/heads/", &head)) {
 		/* 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);
 		}
@@ -696,9 +697,11 @@ static void write_refspec_config(const char* src_ref_prefix,
 					strbuf_addf(&value, "+%s:%s%s", our_head_points_at->name,
 						branch_top->buf, option_branch);
 			} else if (remote_head_points_at) {
-				strbuf_addf(&value, "+%s:%s%s", remote_head_points_at->name,
-						branch_top->buf,
-						skip_prefix(remote_head_points_at->name, "refs/heads/"));
+				const char *name = remote_head_points_at->name;
+				const char *head = NULL;
+				parse_prefix(name, "refs/heads/", &head);
+				strbuf_addf(&value, "+%s:%s%s",
+					    name, branch_top->buf, head);
 			}
 			/*
 			 * otherwise, the next "git fetch" will
diff --git a/builtin/commit.c b/builtin/commit.c
index 3767478..e9bff59 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 = sb->buf;
 
 	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;
+	parse_prefix(start, tmpl.buf, &start);
 	strbuf_release(&tmpl);
 	return rest_is_empty(sb, start - sb->buf);
 }
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 8b8978a2..d673986 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -46,14 +46,14 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 	args.uploadpack = "git-upload-pack";
 
 	for (i = 1; i < argc && *argv[i] == '-'; i++) {
-		const char *arg = argv[i];
+		const char *optarg, *arg = argv[i];
 
-		if (starts_with(arg, "--upload-pack=")) {
-			args.uploadpack = arg + 14;
+		if (parse_prefix(arg, "--upload-pack=", &optarg)) {
+			args.uploadpack = optarg;
 			continue;
 		}
-		if (starts_with(arg, "--exec=")) {
-			args.uploadpack = arg + 7;
+		if (parse_prefix(arg, "--exec=", &optarg)) {
+			args.uploadpack = optarg;
 			continue;
 		}
 		if (!strcmp("--quiet", arg) || !strcmp("-q", arg)) {
@@ -89,8 +89,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 			args.verbose = 1;
 			continue;
 		}
-		if (starts_with(arg, "--depth=")) {
-			args.depth = strtol(arg + 8, NULL, 0);
+		if (parse_prefix(arg, "--depth=", &optarg)) {
+			args.depth = strtol(optarg, NULL, 0);
 			continue;
 		}
 		if (!strcmp("--no-progress", arg)) {
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 3906eda..ce889e3 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -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, " <")))
+	     parse_prefix(me, them->items->string, &me) &&
+	     starts_with(me, " <")))
 		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..f040e82 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -88,11 +88,10 @@ static NORETURN int die_push_simple(struct branch *branch, struct remote *remote
 	 * them the big ugly fully qualified ref.
 	 */
 	const char *advice_maybe = "";
-	const char *short_upstream =
-		skip_prefix(branch->merge[0]->src, "refs/heads/");
+	const char *short_upstream = branch->merge[0]->src;
+
+	parse_prefix(short_upstream, "refs/heads/", &short_upstream);
 
-	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..30d1987 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -250,9 +250,7 @@ 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;
+	parse_prefix(name, prefix, &name);
 	return name;
 }
 #define abbrev_branch(name) abbrev_ref((name), "refs/heads/")
diff --git a/column.c b/column.c
index 9367ba5..dfb2392 100644
--- a/column.c
+++ b/column.c
@@ -336,8 +336,9 @@ static int column_config(const char *var, const char *value,
 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)
+	const char *it;
+
+	if (!parse_prefix(var, "column.", &it))
 		return 0;
 
 	if (!strcmp(it, "ui"))
diff --git a/config.c b/config.c
index d969a5a..ee42010 100644
--- a/config.c
+++ b/config.c
@@ -133,8 +133,7 @@ int git_config_include(const char *var, const char *value, void *data)
 	if (ret < 0)
 		return ret;
 
-	type = skip_prefix(var, "include.");
-	if (!type)
+	if (!parse_prefix(var, "include.", &type))
 		return ret;
 
 	if (!strcmp(type, "path"))
diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c
index 390f194..3823a38 100644
--- a/credential-cache--daemon.c
+++ b/credential-cache--daemon.c
@@ -109,14 +109,12 @@ static int read_request(FILE *fh, struct credential *c,
 	const char *p;
 
 	strbuf_getline(&item, fh, '\n');
-	p = skip_prefix(item.buf, "action=");
-	if (!p)
+	if (!parse_prefix(item.buf, "action=", &p))
 		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 (!parse_prefix(item.buf, "timeout=", &p))
 		return error("client sent bogus timeout line: %s", item.buf);
 	*timeout = atoi(p);
 
diff --git a/credential.c b/credential.c
index e54753c..fd148a0 100644
--- a/credential.c
+++ b/credential.c
@@ -40,8 +40,7 @@ static int credential_config_callback(const char *var, const char *value,
 	struct credential *c = data;
 	const char *key, *dot;
 
-	key = skip_prefix(var, "credential.");
-	if (!key)
+	if (!parse_prefix(var, "credential.", &key))
 		return 0;
 
 	if (!value)
diff --git a/git-compat-util.h b/git-compat-util.h
index b73916b..0a8354a 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -350,6 +350,7 @@ extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_lis
 extern void set_error_routine(void (*routine)(const char *err, va_list params));
 extern void set_die_is_recursing_routine(int (*routine)(void));
 
+extern int parse_prefix(const char *str, const char *prefix, const char **rest);
 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);
diff --git a/parse-options.c b/parse-options.c
index 7b8d3fa..1e25203 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -238,7 +238,8 @@ static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg,
 			continue;
 
 again:
-		rest = skip_prefix(arg, long_name);
+		rest = NULL;
+		parse_prefix(arg, long_name, &rest);
 		if (options->type == OPTION_ARGUMENT) {
 			if (!rest)
 				continue;
@@ -287,12 +288,12 @@ is_abbreviated:
 				continue;
 			}
 			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 (!parse_prefix(arg + 3, long_name, &rest)) {
+				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..b78bc44 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -1,15 +1,21 @@
 #include "cache.h"
 #include "refs.h"
 
-int starts_with(const char *str, const char *prefix)
+int parse_prefix(const char *str, const char *prefix, const char **rest)
 {
 	for (; ; str++, prefix++)
-		if (!*prefix)
+		if (!*prefix) {
+			*rest = str;
 			return 1;
-		else if (*str != *prefix)
+		} else if (*str != *prefix)
 			return 0;
 }
 
+int starts_with(const char *str, const char *prefix)
+{
+	return parse_prefix(str, prefix, &str);
+}
+
 int prefixcmp(const char *str, const char *prefix)
 {
 	for (; ; str++, prefix++)
diff --git a/transport.c b/transport.c
index 824c5b9..775f2b1 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;
+	const char *rest = NULL;
+	if (starts_with(url, "rsync://"))
+		return url;
+	parse_prefix(url, "rsync:", &rest);
+	return rest;
 }
 
 static struct ref *get_refs_via_rsync(struct transport *transport, int for_push)
diff --git a/urlmatch.c b/urlmatch.c
index ec87cba..dfd1fa7 100644
--- a/urlmatch.c
+++ b/urlmatch.c
@@ -483,8 +483,7 @@ int urlmatch_config_entry(const char *var, const char *value, void *cb)
 	int user_matched = 0;
 	int retval;
 
-	key = skip_prefix(var, collect->section);
-	if (!key || *(key++) != '.') {
+	if (!parse_prefix(var, collect->section, &key) || *(key++) != '.') {
 		if (collect->cascade_fn)
 			return collect->cascade_fn(var, value, cb);
 		return 0; /* not interested */

-- 
1.8.5.2
Previous: Christian CouderNext: Junio C Hamano
Message 9 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.