[PATCH 00/12] Hard coded string length cleanup

[PATCH 00/12] Hard coded string length cleanup

[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 :)

Nguyễn Thái Ngọc Duy (12):
  Make starts_with() a wrapper of skip_prefix()
  Convert starts_with() to skip_prefix() for option parsing
  Add and use skip_prefix_defval()
  Replace some use of starts_with() with skip_prefix()
  Convert a lot of starts_with() to skip_prefix()
  fetch.c: replace some use of starts_with() with skip_prefix()
  connect.c: replace some use of starts_with() with skip_prefix()
  refs.c: replace some use of starts_with() with skip_prefix()
  diff.c: reduce code duplication in --stat-xxx parsing
  environment.c: replace starts_with() in strip_namespace() with skip_prefix()
  diff.c: convert diff_scoreopt_parse to use skip_prefix()
  refs.c: use skip_prefix() in prune_ref()

 builtin/branch.c         |   3 +-
 builtin/checkout.c       |   6 +-
 builtin/fast-export.c    |   3 +-
 builtin/fetch-pack.c     |  13 ++--
 builtin/fetch.c          |  16 ++---
 builtin/for-each-ref.c   |   9 +--
 builtin/index-pack.c     |  17 +++---
 builtin/ls-remote.c      |   9 +--
 builtin/mailinfo.c       |  11 ++--
 builtin/merge.c          |  12 ++--
 builtin/reflog.c         |   9 +--
 builtin/remote.c         |   3 +-
 builtin/rev-parse.c      |  41 ++++++-------
 builtin/send-pack.c      |  18 +++---
 builtin/show-branch.c    |  14 ++---
 builtin/unpack-objects.c |   5 +-
 builtin/update-ref.c     |  21 +++----
 commit.c                 |   5 +-
 connect.c                |   6 +-
 daemon.c                 |  75 +++++++++++------------
 diff.c                   | 153 +++++++++++++++++++++--------------------------
 environment.c            |   4 +-
 fetch-pack.c             |   9 +--
 git-compat-util.h        |  15 ++++-
 git.c                    |  16 ++---
 http-backend.c           |   5 +-
 http-push.c              |   6 +-
 http.c                   |   5 +-
 log-tree.c               |   5 +-
 merge-recursive.c        |  13 ++--
 notes.c                  |   6 +-
 pager.c                  |   2 +-
 pathspec.c               |   5 +-
 pretty.c                 |   3 +-
 refs.c                   |  20 ++++---
 revision.c               |  60 +++++++++----------
 setup.c                  |   3 +-
 sha1_name.c              |  12 +---
 strbuf.c                 |   9 ---
 tag.c                    |   7 +--
 transport-helper.c       |  15 +++--
 transport.c              |  14 +++--
 upload-pack.c            |   5 +-
 wt-status.c              |  15 ++---
 44 files changed, 334 insertions(+), 369 deletions(-)

-- 
1.8.5.1.208.g019362e

[PATCH 01/12] Make starts_with() a wrapper of skip_prefix()

[Nguyễn Thái Ngọc Duy]

starts_with() started out as a copy of prefixcmp(). But if we don't
care about the sorting order, the logic looks closer to
skip_prefix(). This looks like a good thing to do.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 git-compat-util.h | 6 +++++-
 strbuf.c          | 9 ---------
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index b73916b..84f1078 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -350,7 +350,6 @@ 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 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);
@@ -361,6 +360,11 @@ static inline const char *skip_prefix(const char *str, const char *prefix)
 	return strncmp(str, prefix, len) ? NULL : str + len;
 }
 
+static inline int starts_with(const char *str, const char *prefix)
+{
+	return skip_prefix(str, prefix) != NULL;
+}
+
 #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)
 
 #ifndef PROT_READ
diff --git a/strbuf.c b/strbuf.c
index 83caf4a..bd4c0d8 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -1,15 +1,6 @@
 #include "cache.h"
 #include "refs.h"
 
-int starts_with(const char *str, const char *prefix)
-{
-	for (; ; str++, prefix++)
-		if (!*prefix)
-			return 1;
-		else if (*str != *prefix)
-			return 0;
-}
-
 int prefixcmp(const char *str, const char *prefix)
 {
 	for (; ; str++, prefix++)
-- 
1.8.5.1.208.g019362e

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

[Nguyễn Thái Ngọc Duy]

The code that's not converted to use parse_options() often does

  if (!starts_with(arg, "foo=")) {
     value = atoi(arg + 4);
  }

This patch removes those magic numbers with skip_prefix()

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/fetch-pack.c     | 13 +++++----
 builtin/index-pack.c     | 17 +++++------
 builtin/ls-remote.c      |  9 +++---
 builtin/mailinfo.c       |  5 ++--
 builtin/reflog.c         |  9 +++---
 builtin/rev-parse.c      | 41 +++++++++++++-------------
 builtin/send-pack.c      | 18 ++++++------
 builtin/unpack-objects.c |  5 ++--
 builtin/update-ref.c     | 21 +++++++-------
 daemon.c                 | 75 ++++++++++++++++++++++++------------------------
 diff.c                   | 49 +++++++++++++++----------------
 git.c                    | 13 +++++----
 merge-recursive.c        | 13 +++++----
 revision.c               | 60 +++++++++++++++++++-------------------
 upload-pack.c            |  5 ++--
 15 files changed, 182 insertions(+), 171 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 8b8978a2..2df1423 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -47,13 +47,14 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 
 	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;
 			continue;
 		}
-		if (starts_with(arg, "--exec=")) {
-			args.uploadpack = arg + 7;
+		if ((optarg = skip_prefix(arg, "--exec=")) != NULL) {
+			args.uploadpack = optarg;
 			continue;
 		}
 		if (!strcmp("--quiet", arg) || !strcmp("-q", arg)) {
@@ -89,8 +90,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 ((optarg = skip_prefix(arg, "--depth=")) != NULL) {
+			args.depth = strtol(optarg, NULL, 0);
 			continue;
 		}
 		if (!strcmp("--no-progress", arg)) {
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 2f37a38..67eff7a 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1511,6 +1511,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
+		const char *optarg;
 
 		if (*arg == '-') {
 			if (!strcmp(arg, "--stdin")) {
@@ -1534,11 +1535,11 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 				stat_only = 1;
 			} else if (!strcmp(arg, "--keep")) {
 				keep_msg = "";
-			} else if (starts_with(arg, "--keep=")) {
-				keep_msg = arg + 7;
-			} else if (starts_with(arg, "--threads=")) {
+			} else if ((optarg = skip_prefix(arg, "--keep=")) != NULL) {
+				keep_msg = optarg;
+			} else if ((optarg = skip_prefix(arg, "--threads=")) != NULL) {
 				char *end;
-				nr_threads = strtoul(arg+10, &end, 0);
+				nr_threads = strtoul(optarg, &end, 0);
 				if (!arg[10] || *end || nr_threads < 0)
 					usage(index_pack_usage);
 #ifdef NO_PTHREADS
@@ -1547,13 +1548,13 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 						  "ignoring %s"), arg);
 				nr_threads = 1;
 #endif
-			} else if (starts_with(arg, "--pack_header=")) {
+			} else if ((optarg = skip_prefix(arg, "--pack_header=")) != NULL) {
 				struct pack_header *hdr;
 				char *c;
 
 				hdr = (struct pack_header *)input_buffer;
 				hdr->hdr_signature = htonl(PACK_SIGNATURE);
-				hdr->hdr_version = htonl(strtoul(arg + 14, &c, 10));
+				hdr->hdr_version = htonl(strtoul(optarg, &c, 10));
 				if (*c != ',')
 					die(_("bad %s"), arg);
 				hdr->hdr_entries = htonl(strtoul(c + 1, &c, 10));
@@ -1566,9 +1567,9 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 				if (index_name || (i+1) >= argc)
 					usage(index_pack_usage);
 				index_name = argv[++i];
-			} else if (starts_with(arg, "--index-version=")) {
+			} else if ((optarg = skip_prefix(arg, "--index-version=")) != NULL) {
 				char *c;
-				opts.version = strtoul(arg + 16, &c, 10);
+				opts.version = strtoul(optarg, &c, 10);
 				if (opts.version > 2)
 					die(_("bad %s"), arg);
 				if (*c == ',')
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 39e5144..15c9fb3 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -48,14 +48,15 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
+		const char *optarg;
 
 		if (*arg == '-') {
-			if (starts_with(arg, "--upload-pack=")) {
-				uploadpack = arg + 14;
+			if ((optarg = skip_prefix(arg, "--upload-pack=")) != NULL) {
+				uploadpack = optarg;
 				continue;
 			}
-			if (starts_with(arg, "--exec=")) {
-				uploadpack = arg + 7;
+			if ((optarg = skip_prefix(arg, "--exec=")) != NULL) {
+				uploadpack = optarg;
 				continue;
 			}
 			if (!strcmp("--tags", arg) || !strcmp("-t", arg)) {
diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c
index 2c3cd8e..2100e23 100644
--- a/builtin/mailinfo.c
+++ b/builtin/mailinfo.c
@@ -1002,6 +1002,7 @@ static const char mailinfo_usage[] =
 int cmd_mailinfo(int argc, const char **argv, const char *prefix)
 {
 	const char *def_charset;
+	const char *optarg;
 
 	/* NEEDSWORK: might want to do the optional .git/ directory
 	 * discovery
@@ -1020,8 +1021,8 @@ int cmd_mailinfo(int argc, const char **argv, const char *prefix)
 			metainfo_charset = def_charset;
 		else if (!strcmp(argv[1], "-n"))
 			metainfo_charset = NULL;
-		else if (starts_with(argv[1], "--encoding="))
-			metainfo_charset = argv[1] + 11;
+		else if ((optarg = skip_prefix(argv[1], "--encoding=")) != NULL)
+			metainfo_charset = optarg;
 		else if (!strcmp(argv[1], "--scissors"))
 			use_scissors = 1;
 		else if (!strcmp(argv[1], "--no-scissors"))
diff --git a/builtin/reflog.c b/builtin/reflog.c
index 852cff6..84a8bd9 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -608,15 +608,16 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
+		const char *optarg;
 		if (!strcmp(arg, "--dry-run") || !strcmp(arg, "-n"))
 			cb.dry_run = 1;
-		else if (starts_with(arg, "--expire=")) {
-			if (parse_expiry_date(arg + 9, &cb.expire_total))
+		else if ((optarg = skip_prefix(arg, "--expire=")) != NULL) {
+			if (parse_expiry_date(optarg, &cb.expire_total))
 				die(_("'%s' is not a valid timestamp"), arg);
 			explicit_expiry |= EXPIRE_TOTAL;
 		}
-		else if (starts_with(arg, "--expire-unreachable=")) {
-			if (parse_expiry_date(arg + 21, &cb.expire_unreachable))
+		else if ((optarg = skip_prefix(arg, "--expire-unreachable=")) != NULL) {
+			if (parse_expiry_date(optarg, &cb.expire_unreachable))
 				die(_("'%s' is not a valid timestamp"), arg);
 			explicit_expiry |= EXPIRE_UNREACH;
 		}
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 6e802fd..1a0bd12 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -505,6 +505,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 	git_config(git_default_config, NULL);
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
+		const char *optarg;
 
 		if (as_is) {
 			if (show_file(arg, output_prefix) && as_is < 2)
@@ -618,8 +619,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 				for_each_ref(show_reference, NULL);
 				continue;
 			}
-			if (starts_with(arg, "--disambiguate=")) {
-				for_each_abbrev(arg + 15, show_abbrev, NULL);
+			if ((optarg = skip_prefix(arg, "--disambiguate=")) != NULL) {
+				for_each_abbrev(optarg, show_abbrev, NULL);
 				continue;
 			}
 			if (!strcmp(arg, "--bisect")) {
@@ -627,8 +628,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 				for_each_ref_in("refs/bisect/good", anti_reference, NULL);
 				continue;
 			}
-			if (starts_with(arg, "--branches=")) {
-				for_each_glob_ref_in(show_reference, arg + 11,
+			if ((optarg = skip_prefix(arg, "--branches=")) != NULL) {
+				for_each_glob_ref_in(show_reference, optarg,
 					"refs/heads/", NULL);
 				clear_ref_exclusion(&ref_excludes);
 				continue;
@@ -638,8 +639,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 				clear_ref_exclusion(&ref_excludes);
 				continue;
 			}
-			if (starts_with(arg, "--tags=")) {
-				for_each_glob_ref_in(show_reference, arg + 7,
+			if ((optarg = skip_prefix(arg, "--tags=")) != NULL) {
+				for_each_glob_ref_in(show_reference, optarg,
 					"refs/tags/", NULL);
 				clear_ref_exclusion(&ref_excludes);
 				continue;
@@ -649,13 +650,13 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 				clear_ref_exclusion(&ref_excludes);
 				continue;
 			}
-			if (starts_with(arg, "--glob=")) {
-				for_each_glob_ref(show_reference, arg + 7, NULL);
+			if ((optarg = skip_prefix(arg, "--glob=")) != NULL) {
+				for_each_glob_ref(show_reference, optarg, NULL);
 				clear_ref_exclusion(&ref_excludes);
 				continue;
 			}
-			if (starts_with(arg, "--remotes=")) {
-				for_each_glob_ref_in(show_reference, arg + 10,
+			if ((optarg = skip_prefix(arg, "--remotes=")) != NULL) {
+				for_each_glob_ref_in(show_reference, optarg,
 					"refs/remotes/", NULL);
 				clear_ref_exclusion(&ref_excludes);
 				continue;
@@ -665,8 +666,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 				clear_ref_exclusion(&ref_excludes);
 				continue;
 			}
-			if (starts_with(arg, "--exclude=")) {
-				add_ref_exclusion(&ref_excludes, arg + 10);
+			if ((optarg = skip_prefix(arg, "--exclude=")) != NULL) {
+				add_ref_exclusion(&ref_excludes, optarg);
 				continue;
 			}
 			if (!strcmp(arg, "--local-env-vars")) {
@@ -747,20 +748,20 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 						: "false");
 				continue;
 			}
-			if (starts_with(arg, "--since=")) {
-				show_datestring("--max-age=", arg+8);
+			if ((optarg = skip_prefix(arg, "--since=")) != NULL) {
+				show_datestring("--max-age=", optarg);
 				continue;
 			}
-			if (starts_with(arg, "--after=")) {
-				show_datestring("--max-age=", arg+8);
+			if ((optarg = skip_prefix(arg, "--after=")) != NULL) {
+				show_datestring("--max-age=", optarg);
 				continue;
 			}
-			if (starts_with(arg, "--before=")) {
-				show_datestring("--min-age=", arg+9);
+			if ((optarg = skip_prefix(arg, "--before=")) != NULL) {
+				show_datestring("--min-age=", optarg);
 				continue;
 			}
-			if (starts_with(arg, "--until=")) {
-				show_datestring("--min-age=", arg+8);
+			if ((optarg = skip_prefix(arg, "--until=")) != NULL) {
+				show_datestring("--min-age=", optarg);
 				continue;
 			}
 			if (show_flag(arg) && verify)
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index e7f0b97..9efc422 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -113,18 +113,19 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 	argv++;
 	for (i = 1; i < argc; i++, argv++) {
 		const char *arg = *argv;
+		const char *optarg;
 
 		if (*arg == '-') {
-			if (starts_with(arg, "--receive-pack=")) {
-				receivepack = arg + 15;
+			if ((optarg = skip_prefix(arg, "--receive-pack=")) != NULL) {
+				receivepack = optarg;
 				continue;
 			}
-			if (starts_with(arg, "--exec=")) {
-				receivepack = arg + 7;
+			if ((optarg = skip_prefix(arg, "--exec=")) != NULL) {
+				receivepack = optarg;
 				continue;
 			}
-			if (starts_with(arg, "--remote=")) {
-				remote_name = arg + 9;
+			if ((optarg = skip_prefix(arg, "--remote=")) != NULL) {
+				remote_name = optarg;
 				continue;
 			}
 			if (!strcmp(arg, "--all")) {
@@ -181,9 +182,8 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 					exit(1);
 				continue;
 			}
-			if (starts_with(arg, "--" CAS_OPT_NAME "=")) {
-				if (parse_push_cas_option(&cas,
-							  strchr(arg, '=') + 1, 0) < 0)
+			if ((optarg = skip_prefix(arg, "--" CAS_OPT_NAME "=")) != NULL) {
+				if (parse_push_cas_option(&cas, optarg, 0) < 0)
 					exit(1);
 				continue;
 			}
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index 62ff673..a7cd823 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -505,6 +505,7 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix)
 
 	for (i = 1 ; i < argc; i++) {
 		const char *arg = argv[i];
+		const char *optarg;
 
 		if (*arg == '-') {
 			if (!strcmp(arg, "-n")) {
@@ -523,13 +524,13 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix)
 				strict = 1;
 				continue;
 			}
-			if (starts_with(arg, "--pack_header=")) {
+			if ((optarg = skip_prefix(arg, "--pack_header=")) != NULL) {
 				struct pack_header *hdr;
 				char *c;
 
 				hdr = (struct pack_header *)buffer;
 				hdr->hdr_signature = htonl(PACK_SIGNATURE);
-				hdr->hdr_version = htonl(strtoul(arg + 14, &c, 10));
+				hdr->hdr_version = htonl(strtoul(optarg, &c, 10));
 				if (*c != ',')
 					die("bad %s", arg);
 				hdr->hdr_entries = htonl(strtoul(c + 1, &c, 10));
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 1292cfe..09c921a 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -222,6 +222,7 @@ static void parse_cmd_option(const char *next)
 static void update_refs_stdin(void)
 {
 	struct strbuf cmd = STRBUF_INIT;
+	const char *optarg;
 
 	/* Read each line dispatch its command */
 	while (strbuf_getline(&cmd, stdin, line_termination) != EOF)
@@ -229,16 +230,16 @@ static void update_refs_stdin(void)
 			die("empty command in input");
 		else if (isspace(*cmd.buf))
 			die("whitespace before command: %s", cmd.buf);
-		else if (starts_with(cmd.buf, "update "))
-			parse_cmd_update(cmd.buf + 7);
-		else if (starts_with(cmd.buf, "create "))
-			parse_cmd_create(cmd.buf + 7);
-		else if (starts_with(cmd.buf, "delete "))
-			parse_cmd_delete(cmd.buf + 7);
-		else if (starts_with(cmd.buf, "verify "))
-			parse_cmd_verify(cmd.buf + 7);
-		else if (starts_with(cmd.buf, "option "))
-			parse_cmd_option(cmd.buf + 7);
+		else if ((optarg = skip_prefix(cmd.buf, "update ")) != NULL)
+			parse_cmd_update(optarg);
+		else if ((optarg = skip_prefix(cmd.buf, "create ")) != NULL)
+			parse_cmd_create(optarg);
+		else if ((optarg = skip_prefix(cmd.buf, "delete ")) != NULL)
+			parse_cmd_delete(optarg);
+		else if ((optarg = skip_prefix(cmd.buf, "verify ")) != NULL)
+			parse_cmd_verify(optarg);
+		else if ((optarg = skip_prefix(cmd.buf, "option ")) != NULL)
+			parse_cmd_option(optarg);
 		else
 			die("unknown command: %s", cmd.buf);
 
diff --git a/daemon.c b/daemon.c
index 7bee953..9d3cc18 100644
--- a/daemon.c
+++ b/daemon.c
@@ -39,8 +39,8 @@ static int strict_paths;
 static int export_all_trees;
 
 /* Take all paths relative to this one if non-NULL */
-static char *base_path;
-static char *interpolated_path;
+static const char *base_path;
+static const char *interpolated_path;
 static int base_path_relaxed;
 
 /* Flag indicating client sent extra args. */
@@ -253,7 +253,7 @@ static int daemon_error(const char *dir, const char *msg)
 	return -1;
 }
 
-static char *access_hook;
+static const char *access_hook;
 
 static int run_access_hook(struct daemon_service *service, const char *dir, const char *path)
 {
@@ -1164,15 +1164,16 @@ int main(int argc, char **argv)
 
 	for (i = 1; i < argc; i++) {
 		char *arg = argv[i];
+		const char *optarg;
 
-		if (starts_with(arg, "--listen=")) {
-			string_list_append(&listen_addr, xstrdup_tolower(arg + 9));
+		if ((optarg = skip_prefix(arg, "--listen=")) != NULL) {
+			string_list_append(&listen_addr, xstrdup_tolower(optarg));
 			continue;
 		}
-		if (starts_with(arg, "--port=")) {
+		if ((optarg = skip_prefix(arg, "--port=")) != NULL) {
 			char *end;
 			unsigned long n;
-			n = strtoul(arg+7, &end, 0);
+			n = strtoul(optarg, &end, 0);
 			if (arg[7] && !*end) {
 				listen_port = n;
 				continue;
@@ -1199,20 +1200,20 @@ int main(int argc, char **argv)
 			export_all_trees = 1;
 			continue;
 		}
-		if (starts_with(arg, "--access-hook=")) {
-			access_hook = arg + 14;
+		if ((optarg = skip_prefix(arg, "--access-hook=")) != NULL) {
+			access_hook = optarg;
 			continue;
 		}
-		if (starts_with(arg, "--timeout=")) {
-			timeout = atoi(arg+10);
+		if ((optarg = skip_prefix(arg, "--timeout=")) != NULL) {
+			timeout = atoi(optarg);
 			continue;
 		}
-		if (starts_with(arg, "--init-timeout=")) {
-			init_timeout = atoi(arg+15);
+		if ((optarg = skip_prefix(arg, "--init-timeout=")) != NULL) {
+			init_timeout = atoi(optarg);
 			continue;
 		}
-		if (starts_with(arg, "--max-connections=")) {
-			max_connections = atoi(arg+18);
+		if ((optarg = skip_prefix(arg, "--max-connections=")) != NULL) {
+			max_connections = atoi(optarg);
 			if (max_connections < 0)
 				max_connections = 0;	        /* unlimited */
 			continue;
@@ -1221,16 +1222,16 @@ int main(int argc, char **argv)
 			strict_paths = 1;
 			continue;
 		}
-		if (starts_with(arg, "--base-path=")) {
-			base_path = arg+12;
+		if ((optarg = skip_prefix(arg, "--base-path=")) != NULL) {
+			base_path = optarg;
 			continue;
 		}
 		if (!strcmp(arg, "--base-path-relaxed")) {
 			base_path_relaxed = 1;
 			continue;
 		}
-		if (starts_with(arg, "--interpolated-path=")) {
-			interpolated_path = arg+20;
+		if ((optarg = skip_prefix(arg, "--interpolated-path=")) != NULL) {
+			interpolated_path = optarg;
 			continue;
 		}
 		if (!strcmp(arg, "--reuseaddr")) {
@@ -1241,12 +1242,12 @@ int main(int argc, char **argv)
 			user_path = "";
 			continue;
 		}
-		if (starts_with(arg, "--user-path=")) {
-			user_path = arg + 12;
+		if ((optarg = skip_prefix(arg, "--user-path=")) != NULL) {
+			user_path = optarg;
 			continue;
 		}
-		if (starts_with(arg, "--pid-file=")) {
-			pid_file = arg + 11;
+		if ((optarg = skip_prefix(arg, "--pid-file=")) != NULL) {
+			pid_file = optarg;
 			continue;
 		}
 		if (!strcmp(arg, "--detach")) {
@@ -1254,35 +1255,35 @@ int main(int argc, char **argv)
 			log_syslog = 1;
 			continue;
 		}
-		if (starts_with(arg, "--user=")) {
-			user_name = arg + 7;
+		if ((optarg = skip_prefix(arg, "--user=")) != NULL) {
+			user_name = optarg;
 			continue;
 		}
-		if (starts_with(arg, "--group=")) {
-			group_name = arg + 8;
+		if ((optarg = skip_prefix(arg, "--group=")) != NULL) {
+			group_name = optarg;
 			continue;
 		}
-		if (starts_with(arg, "--enable=")) {
-			enable_service(arg + 9, 1);
+		if ((optarg = skip_prefix(arg, "--enable=")) != NULL) {
+			enable_service(optarg, 1);
 			continue;
 		}
-		if (starts_with(arg, "--disable=")) {
-			enable_service(arg + 10, 0);
+		if ((optarg = skip_prefix(arg, "--disable=")) != NULL) {
+			enable_service(optarg, 0);
 			continue;
 		}
-		if (starts_with(arg, "--allow-override=")) {
-			make_service_overridable(arg + 17, 1);
+		if ((optarg = skip_prefix(arg, "--allow-override=")) != NULL) {
+			make_service_overridable(optarg, 1);
 			continue;
 		}
-		if (starts_with(arg, "--forbid-override=")) {
-			make_service_overridable(arg + 18, 0);
+		if ((optarg = skip_prefix(arg, "--forbid-override=")) != NULL) {
+			make_service_overridable(optarg, 0);
 			continue;
 		}
-		if (starts_with(arg, "--informative-errors")) {
+		if ((optarg = skip_prefix(arg, "--informative-errors")) != NULL) {
 			informative_errors = 1;
 			continue;
 		}
-		if (starts_with(arg, "--no-informative-errors")) {
+		if ((optarg = skip_prefix(arg, "--no-informative-errors")) != NULL) {
 			informative_errors = 0;
 			continue;
 		}
diff --git a/diff.c b/diff.c
index b79432b..90a1929 100644
--- a/diff.c
+++ b/diff.c
@@ -2353,6 +2353,7 @@ static void builtin_diff(const char *name_a,
 		xdemitconf_t xecfg;
 		struct emit_callback ecbdata;
 		const struct userdiff_funcname *pe;
+		const char *optarg;
 
 		if (must_show_header) {
 			fprintf(o->file, "%s", header.buf);
@@ -2387,10 +2388,10 @@ static void builtin_diff(const char *name_a,
 			xdiff_set_find_func(&xecfg, pe->pattern, pe->cflags);
 		if (!diffopts)
 			;
-		else if (starts_with(diffopts, "--unified="))
-			xecfg.ctxlen = strtoul(diffopts + 10, NULL, 10);
-		else if (starts_with(diffopts, "-u"))
-			xecfg.ctxlen = strtoul(diffopts + 2, NULL, 10);
+		else if ((optarg = skip_prefix(diffopts, "--unified=")) != NULL)
+			xecfg.ctxlen = strtoul(optarg, NULL, 10);
+		else if ((optarg = skip_prefix(diffopts, "-u")) != NULL)
+			xecfg.ctxlen = strtoul(optarg, NULL, 10);
 		if (o->word_diff)
 			init_diff_words_data(&ecbdata, o, one, two);
 		xdi_diff_outf(&mf1, &mf2, fn_out_consume, &ecbdata,
@@ -3614,17 +3615,17 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		options->output_format |= DIFF_FORMAT_SHORTSTAT;
 	else if (!strcmp(arg, "-X") || !strcmp(arg, "--dirstat"))
 		return parse_dirstat_opt(options, "");
-	else if (starts_with(arg, "-X"))
-		return parse_dirstat_opt(options, arg + 2);
-	else if (starts_with(arg, "--dirstat="))
-		return parse_dirstat_opt(options, arg + 10);
+	else if ((optarg = skip_prefix(arg, "-X")) != NULL)
+		return parse_dirstat_opt(options, optarg);
+	else if ((optarg = skip_prefix(arg, "--dirstat=")) != NULL)
+		return parse_dirstat_opt(options, optarg);
 	else if (!strcmp(arg, "--cumulative"))
 		return parse_dirstat_opt(options, "cumulative");
 	else if (!strcmp(arg, "--dirstat-by-file"))
 		return parse_dirstat_opt(options, "files");
-	else if (starts_with(arg, "--dirstat-by-file=")) {
+	else if ((optarg = skip_prefix(arg, "--dirstat-by-file=")) != NULL) {
 		parse_dirstat_opt(options, "files");
-		return parse_dirstat_opt(options, arg + 18);
+		return parse_dirstat_opt(options, optarg);
 	}
 	else if (!strcmp(arg, "--check"))
 		options->output_format |= DIFF_FORMAT_CHECKDIFF;
@@ -3674,9 +3675,9 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		DIFF_OPT_CLR(options, RENAME_EMPTY);
 	else if (!strcmp(arg, "--relative"))
 		DIFF_OPT_SET(options, RELATIVE_NAME);
-	else if (starts_with(arg, "--relative=")) {
+	else if ((optarg = skip_prefix(arg, "--relative=")) != NULL) {
 		DIFF_OPT_SET(options, RELATIVE_NAME);
-		options->prefix = arg + 11;
+		options->prefix = optarg;
 	}
 
 	/* xdiff options */
@@ -3727,8 +3728,8 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		DIFF_OPT_CLR(options, FOLLOW_RENAMES);
 	else if (!strcmp(arg, "--color"))
 		options->use_color = 1;
-	else if (starts_with(arg, "--color=")) {
-		int value = git_config_colorbool(NULL, arg+8);
+	else if ((optarg = skip_prefix(arg, "--color=")) != NULL) {
+		int value = git_config_colorbool(NULL, optarg);
 		if (value < 0)
 			return error("option `color' expects \"always\", \"auto\", or \"never\"");
 		options->use_color = value;
@@ -3739,17 +3740,17 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		options->use_color = 1;
 		options->word_diff = DIFF_WORDS_COLOR;
 	}
-	else if (starts_with(arg, "--color-words=")) {
+	else if ((optarg = skip_prefix(arg, "--color-words=")) != NULL) {
 		options->use_color = 1;
 		options->word_diff = DIFF_WORDS_COLOR;
-		options->word_regex = arg + 14;
+		options->word_regex = optarg;
 	}
 	else if (!strcmp(arg, "--word-diff")) {
 		if (options->word_diff == DIFF_WORDS_NONE)
 			options->word_diff = DIFF_WORDS_PLAIN;
 	}
-	else if (starts_with(arg, "--word-diff=")) {
-		const char *type = arg + 12;
+	else if ((optarg = skip_prefix(arg, "--word-diff=")) != NULL) {
+		const char *type = optarg;
 		if (!strcmp(type, "plain"))
 			options->word_diff = DIFF_WORDS_PLAIN;
 		else if (!strcmp(type, "color")) {
@@ -3784,13 +3785,13 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 	else if (!strcmp(arg, "--ignore-submodules")) {
 		DIFF_OPT_SET(options, OVERRIDE_SUBMODULE_CONFIG);
 		handle_ignore_submodules_arg(options, "all");
-	} else if (starts_with(arg, "--ignore-submodules=")) {
+	} else if ((optarg = skip_prefix(arg, "--ignore-submodules=")) != NULL) {
 		DIFF_OPT_SET(options, OVERRIDE_SUBMODULE_CONFIG);
-		handle_ignore_submodules_arg(options, arg + 20);
+		handle_ignore_submodules_arg(options, optarg);
 	} else if (!strcmp(arg, "--submodule"))
 		DIFF_OPT_SET(options, SUBMODULE_LOG);
-	else if (starts_with(arg, "--submodule="))
-		return parse_submodule_opt(options, arg + 12);
+	else if ((optarg = skip_prefix(arg, "--submodule=")) != NULL)
+		return parse_submodule_opt(options, optarg);
 
 	/* misc options */
 	else if (!strcmp(arg, "-z"))
@@ -3825,8 +3826,8 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 	}
 	else if (!strcmp(arg, "--abbrev"))
 		options->abbrev = DEFAULT_ABBREV;
-	else if (starts_with(arg, "--abbrev=")) {
-		options->abbrev = strtoul(arg + 9, NULL, 10);
+	else if ((optarg = skip_prefix(arg, "--abbrev=")) != NULL) {
+		options->abbrev = strtoul(optarg, NULL, 10);
 		if (options->abbrev < MINIMUM_ABBREV)
 			options->abbrev = MINIMUM_ABBREV;
 		else if (40 < options->abbrev)
diff --git a/git.c b/git.c
index 3799514..35fda7e 100644
--- a/git.c
+++ b/git.c
@@ -40,6 +40,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 
 	while (*argc > 0) {
 		const char *cmd = (*argv)[0];
+		const char *optarg;
 		if (cmd[0] != '-')
 			break;
 
@@ -92,8 +93,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 				*envchanged = 1;
 			(*argv)++;
 			(*argc)--;
-		} else if (starts_with(cmd, "--git-dir=")) {
-			setenv(GIT_DIR_ENVIRONMENT, cmd + 10, 1);
+		} else if ((optarg = skip_prefix(cmd, "--git-dir=")) != NULL) {
+			setenv(GIT_DIR_ENVIRONMENT, optarg, 1);
 			if (envchanged)
 				*envchanged = 1;
 		} else if (!strcmp(cmd, "--namespace")) {
@@ -106,8 +107,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 				*envchanged = 1;
 			(*argv)++;
 			(*argc)--;
-		} else if (starts_with(cmd, "--namespace=")) {
-			setenv(GIT_NAMESPACE_ENVIRONMENT, cmd + 12, 1);
+		} else if ((optarg = skip_prefix(cmd, "--namespace=")) != NULL) {
+			setenv(GIT_NAMESPACE_ENVIRONMENT, optarg, 1);
 			if (envchanged)
 				*envchanged = 1;
 		} else if (!strcmp(cmd, "--work-tree")) {
@@ -120,8 +121,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 				*envchanged = 1;
 			(*argv)++;
 			(*argc)--;
-		} else if (starts_with(cmd, "--work-tree=")) {
-			setenv(GIT_WORK_TREE_ENVIRONMENT, cmd + 12, 1);
+		} else if ((optarg = skip_prefix(cmd, "--work-tree=")) != NULL) {
+			setenv(GIT_WORK_TREE_ENVIRONMENT, optarg, 1);
 			if (envchanged)
 				*envchanged = 1;
 		} else if (!strcmp(cmd, "--bare")) {
diff --git a/merge-recursive.c b/merge-recursive.c
index a18bd15..ba7ecb6 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2055,6 +2055,7 @@ void init_merge_options(struct merge_options *o)
 
 int parse_merge_opt(struct merge_options *o, const char *s)
 {
+	const char *optarg;
 	if (!s || !*s)
 		return -1;
 	if (!strcmp(s, "ours"))
@@ -2063,14 +2064,14 @@ int parse_merge_opt(struct merge_options *o, const char *s)
 		o->recursive_variant = MERGE_RECURSIVE_THEIRS;
 	else if (!strcmp(s, "subtree"))
 		o->subtree_shift = "";
-	else if (starts_with(s, "subtree="))
-		o->subtree_shift = s + strlen("subtree=");
+	else if ((optarg = skip_prefix(s, "subtree=")) != NULL)
+		o->subtree_shift = optarg;
 	else if (!strcmp(s, "patience"))
 		o->xdl_opts = DIFF_WITH_ALG(o, PATIENCE_DIFF);
 	else if (!strcmp(s, "histogram"))
 		o->xdl_opts = DIFF_WITH_ALG(o, HISTOGRAM_DIFF);
-	else if (starts_with(s, "diff-algorithm=")) {
-		long value = parse_algorithm_value(s + strlen("diff-algorithm="));
+	else if ((optarg = skip_prefix(s, "diff-algorithm=")) != NULL) {
+		long value = parse_algorithm_value(optarg);
 		if (value < 0)
 			return -1;
 		/* clear out previous settings */
@@ -2088,8 +2089,8 @@ int parse_merge_opt(struct merge_options *o, const char *s)
 		o->renormalize = 1;
 	else if (!strcmp(s, "no-renormalize"))
 		o->renormalize = 0;
-	else if (starts_with(s, "rename-threshold=")) {
-		const char *score = s + strlen("rename-threshold=");
+	else if ((optarg = skip_prefix(s, "rename-threshold=")) != NULL) {
+		const char *score = optarg;
 		if ((o->rename_score = parse_rename_score(&score)) == -1 || *score != 0)
 			return -1;
 	}
diff --git a/revision.c b/revision.c
index a68fde6..94b66e8 100644
--- a/revision.c
+++ b/revision.c
@@ -1652,8 +1652,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->max_count = atoi(argv[1]);
 		revs->no_walk = 0;
 		return 2;
-	} else if (starts_with(arg, "-n")) {
-		revs->max_count = atoi(arg + 2);
+	} else if ((optarg = skip_prefix(arg, "-n")) != NULL) {
+		revs->max_count = atoi(optarg);
 		revs->no_walk = 0;
 	} else if ((argcount = parse_long_opt("max-age", argv, &optarg))) {
 		revs->max_age = atoi(optarg);
@@ -1712,11 +1712,11 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	} else if (!strcmp(arg, "--author-date-order")) {
 		revs->sort_order = REV_SORT_BY_AUTHOR_DATE;
 		revs->topo_order = 1;
-	} else if (starts_with(arg, "--early-output")) {
+	} else if ((optarg = skip_prefix(arg, "--early-output")) != NULL) {
 		int count = 100;
-		switch (arg[14]) {
+		switch (optarg[0]) {
 		case '=':
-			count = atoi(arg+15);
+			count = atoi(optarg + 1);
 			/* Fallthrough */
 		case 0:
 			revs->topo_order = 1;
@@ -1737,12 +1737,12 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->min_parents = 2;
 	} else if (!strcmp(arg, "--no-merges")) {
 		revs->max_parents = 1;
-	} else if (starts_with(arg, "--min-parents=")) {
-		revs->min_parents = atoi(arg+14);
+	} else if ((optarg = skip_prefix(arg, "--min-parents=")) != NULL) {
+		revs->min_parents = atoi(optarg);
 	} else if (starts_with(arg, "--no-min-parents")) {
 		revs->min_parents = 0;
-	} else if (starts_with(arg, "--max-parents=")) {
-		revs->max_parents = atoi(arg+14);
+	} else if ((optarg = skip_prefix(arg, "--max-parents=")) != NULL) {
+		revs->max_parents = atoi(optarg);
 	} else if (starts_with(arg, "--no-max-parents")) {
 		revs->max_parents = -1;
 	} else if (!strcmp(arg, "--boundary")) {
@@ -1818,32 +1818,30 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->verbose_header = 1;
 		revs->pretty_given = 1;
 		get_commit_format(arg+8, revs);
-	} else if (starts_with(arg, "--pretty=") || starts_with(arg, "--format=")) {
+	} else if ((optarg = skip_prefix(arg, "--pretty=")) != NULL ||
+		   (optarg = skip_prefix(arg, "--format=")) != NULL) {
 		/*
 		 * Detached form ("--pretty X" as opposed to "--pretty=X")
 		 * not allowed, since the argument is optional.
 		 */
 		revs->verbose_header = 1;
 		revs->pretty_given = 1;
-		get_commit_format(arg+9, revs);
+		get_commit_format(optarg, revs);
 	} else if (!strcmp(arg, "--show-notes") || !strcmp(arg, "--notes")) {
 		revs->show_notes = 1;
 		revs->show_notes_given = 1;
 		revs->notes_opt.use_default_notes = 1;
 	} else if (!strcmp(arg, "--show-signature")) {
 		revs->show_signature = 1;
-	} else if (starts_with(arg, "--show-notes=") ||
-		   starts_with(arg, "--notes=")) {
+	} else if ((optarg = skip_prefix(arg, "--show-notes=")) != NULL ||
+		   (optarg = skip_prefix(arg, "--notes=")) != NULL) {
 		struct strbuf buf = STRBUF_INIT;
 		revs->show_notes = 1;
 		revs->show_notes_given = 1;
-		if (starts_with(arg, "--show-notes")) {
-			if (revs->notes_opt.use_default_notes < 0)
-				revs->notes_opt.use_default_notes = 1;
-			strbuf_addstr(&buf, arg+13);
-		}
-		else
-			strbuf_addstr(&buf, arg+8);
+		if (starts_with(arg, "--show-notes") &&
+		    revs->notes_opt.use_default_notes < 0)
+			revs->notes_opt.use_default_notes = 1;
+		strbuf_addstr(&buf, optarg);
 		expand_notes_ref(&buf);
 		string_list_append(&revs->notes_opt.extra_notes_refs,
 				   strbuf_detach(&buf, NULL));
@@ -1880,8 +1878,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->abbrev = 0;
 	} else if (!strcmp(arg, "--abbrev")) {
 		revs->abbrev = DEFAULT_ABBREV;
-	} else if (starts_with(arg, "--abbrev=")) {
-		revs->abbrev = strtoul(arg + 9, NULL, 10);
+	} else if ((optarg = skip_prefix(arg, "--abbrev=")) != NULL) {
+		revs->abbrev = strtoul(optarg, NULL, 10);
 		if (revs->abbrev < MINIMUM_ABBREV)
 			revs->abbrev = MINIMUM_ABBREV;
 		else if (revs->abbrev > 40)
@@ -2027,20 +2025,20 @@ static int handle_revision_pseudo_opt(const char *submodule,
 	} else if ((argcount = parse_long_opt("exclude", argv, &optarg))) {
 		add_ref_exclusion(&revs->ref_excludes, optarg);
 		return argcount;
-	} else if (starts_with(arg, "--branches=")) {
+	} else if ((optarg = skip_prefix(arg, "--branches=")) != NULL) {
 		struct all_refs_cb cb;
 		init_all_refs_cb(&cb, revs, *flags);
-		for_each_glob_ref_in(handle_one_ref, arg + 11, "refs/heads/", &cb);
+		for_each_glob_ref_in(handle_one_ref, optarg, "refs/heads/", &cb);
 		clear_ref_exclusion(&revs->ref_excludes);
-	} else if (starts_with(arg, "--tags=")) {
+	} else if ((optarg = skip_prefix(arg, "--tags=")) != NULL) {
 		struct all_refs_cb cb;
 		init_all_refs_cb(&cb, revs, *flags);
-		for_each_glob_ref_in(handle_one_ref, arg + 7, "refs/tags/", &cb);
+		for_each_glob_ref_in(handle_one_ref, optarg, "refs/tags/", &cb);
 		clear_ref_exclusion(&revs->ref_excludes);
-	} else if (starts_with(arg, "--remotes=")) {
+	} else if ((optarg = skip_prefix(arg, "--remotes=")) != NULL) {
 		struct all_refs_cb cb;
 		init_all_refs_cb(&cb, revs, *flags);
-		for_each_glob_ref_in(handle_one_ref, arg + 10, "refs/remotes/", &cb);
+		for_each_glob_ref_in(handle_one_ref, optarg, "refs/remotes/", &cb);
 		clear_ref_exclusion(&revs->ref_excludes);
 	} else if (!strcmp(arg, "--reflog")) {
 		handle_reflog(revs, *flags);
@@ -2048,14 +2046,14 @@ static int handle_revision_pseudo_opt(const char *submodule,
 		*flags ^= UNINTERESTING | BOTTOM;
 	} else if (!strcmp(arg, "--no-walk")) {
 		revs->no_walk = REVISION_WALK_NO_WALK_SORTED;
-	} else if (starts_with(arg, "--no-walk=")) {
+	} else if ((optarg = skip_prefix(arg, "--no-walk=")) != NULL) {
 		/*
 		 * Detached form ("--no-walk X" as opposed to "--no-walk=X")
 		 * not allowed, since the argument is optional.
 		 */
-		if (!strcmp(arg + 10, "sorted"))
+		if (!strcmp(optarg, "sorted"))
 			revs->no_walk = REVISION_WALK_NO_WALK_SORTED;
-		else if (!strcmp(arg + 10, "unsorted"))
+		else if (!strcmp(optarg, "unsorted"))
 			revs->no_walk = REVISION_WALK_NO_WALK_UNSORTED;
 		else
 			return error("invalid argument to --no-walk");
diff --git a/upload-pack.c b/upload-pack.c
index ec56cdb..8523b42 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -799,6 +799,7 @@ int main(int argc, char **argv)
 
 	for (i = 1; i < argc; i++) {
 		char *arg = argv[i];
+		const char *optarg;
 
 		if (arg[0] != '-')
 			break;
@@ -814,8 +815,8 @@ int main(int argc, char **argv)
 			strict = 1;
 			continue;
 		}
-		if (starts_with(arg, "--timeout=")) {
-			timeout = atoi(arg+10);
+		if ((optarg = skip_prefix(arg, "--timeout=")) != NULL) {
+			timeout = atoi(optarg);
 			daemon_mode = 1;
 			continue;
 		}
-- 
1.8.5.1.208.g019362e

[PATCH 03/12] Add and use skip_prefix_defval()

[Nguyễn Thái Ngọc Duy]

This is a variant of skip_prefix() that returns a specied pointer
instead of NULL if no prefix is found. It's helpful to simplify

  if (starts_with(foo, "bar"))
    foo += 3;

into

  foo = skip_prefix_gently(foo, "bar", foo);

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/checkout.c    |  6 ++----
 builtin/fast-export.c |  3 +--
 builtin/merge.c       |  4 ++--
 builtin/show-branch.c | 14 +++++---------
 git-compat-util.h     |  9 +++++++--
 git.c                 |  3 +--
 notes.c               |  6 ++----
 wt-status.c           | 12 ++++--------
 8 files changed, 24 insertions(+), 33 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 5df3837..6531ed4 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1151,10 +1151,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		const char *argv0 = argv[0];
 		if (!argc || !strcmp(argv0, "--"))
 			die (_("--track needs a branch name"));
-		if (starts_with(argv0, "refs/"))
-			argv0 += 5;
-		if (starts_with(argv0, "remotes/"))
-			argv0 += 8;
+		argv0 = skip_prefix_defval(argv0, "refs/", argv0);
+		argv0 = skip_prefix_defval(argv0, "remotes/", argv0);
 		argv0 = strchr(argv0, '/');
 		if (!argv0 || !argv0[1])
 			die (_("Missing branch name; try -b"));
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index b8d8a3a..cd0a302 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -476,8 +476,7 @@ static void handle_tag(const char *name, struct tag *tag)
 		}
 	}
 
-	if (starts_with(name, "refs/tags/"))
-		name += 10;
+	name = skip_prefix_defval(name, "refs/tags/", name);
 	printf("tag %s\nfrom :%d\n%.*s%sdata %d\n%.*s\n",
 	       name, tagged_mark,
 	       (int)(tagger_end - tagger), tagger,
diff --git a/builtin/merge.c b/builtin/merge.c
index 4941a6c..590d907 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1106,8 +1106,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	 * current branch.
 	 */
 	branch = branch_to_free = resolve_refdup("HEAD", head_sha1, 0, &flag);
-	if (branch && starts_with(branch, "refs/heads/"))
-		branch += 11;
+	if (branch)
+		branch = skip_prefix_defval(branch, "refs/heads/", branch);
 	if (!branch || is_null_sha1(head_sha1))
 		head_commit = NULL;
 	else
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index d9217ce..6078132 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -284,8 +284,7 @@ static void show_one_commit(struct commit *commit, int no_name)
 		pp_commit_easy(CMIT_FMT_ONELINE, commit, &pretty);
 		pretty_str = pretty.buf;
 	}
-	if (starts_with(pretty_str, "[PATCH] "))
-		pretty_str += 8;
+	pretty_str = skip_prefix_defval(pretty_str, "[PATCH] ", pretty_str);
 
 	if (!no_name) {
 		if (name && name->head_name) {
@@ -473,14 +472,13 @@ static void snarf_refs(int head, int remotes)
 	}
 }
 
-static int rev_is_head(char *head, int headlen, char *name,
+static int rev_is_head(const char *head, int headlen, const char *name,
 		       unsigned char *head_sha1, unsigned char *sha1)
 {
 	if ((!head[0]) ||
 	    (head_sha1 && sha1 && hashcmp(head_sha1, sha1)))
 		return 0;
-	if (starts_with(head, "refs/heads/"))
-		head += 11;
+	head = skip_prefix_defval(head, "refs/heads/", head);
 	if (starts_with(name, "refs/heads/"))
 		name += 11;
 	else if (starts_with(name, "heads/"))
@@ -811,10 +809,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 					head_sha1, NULL))
 				has_head++;
 		}
-		if (!has_head) {
-			int offset = starts_with(head, "refs/heads/") ? 11 : 0;
-			append_one_rev(head + offset);
-		}
+		if (!has_head)
+			append_one_rev(skip_prefix_defval(head, "refs/heads/", head));
 	}
 
 	if (!ref_name_cnt) {
diff --git a/git-compat-util.h b/git-compat-util.h
index 84f1078..b72a80d 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -354,10 +354,15 @@ 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)
+static inline const char *skip_prefix_defval(const char *str, const char *prefix, const char *defval)
 {
 	size_t len = strlen(prefix);
-	return strncmp(str, prefix, len) ? NULL : str + len;
+	return strncmp(str, prefix, len) ? defval : str + len;
+}
+
+static inline const char *skip_prefix(const char *str, const char *prefix)
+{
+	return skip_prefix_defval(str, prefix, NULL);
 }
 
 static inline int starts_with(const char *str, const char *prefix)
diff --git a/git.c b/git.c
index 35fda7e..321ae81 100644
--- a/git.c
+++ b/git.c
@@ -579,8 +579,7 @@ int main(int argc, char **av)
 	argc--;
 	handle_options(&argv, &argc, NULL);
 	if (argc > 0) {
-		if (starts_with(argv[0], "--"))
-			argv[0] += 2;
+		argv[0] = skip_prefix_defval(argv[0], "--", argv[0]);
 	} else {
 		/* The user didn't specify a command; give them help */
 		commit_pager_choice();
diff --git a/notes.c b/notes.c
index 5f07c0b..31f513b 100644
--- a/notes.c
+++ b/notes.c
@@ -1243,10 +1243,8 @@ static void format_note(struct notes_tree *t, const unsigned char *object_sha1,
 		if (!ref || !strcmp(ref, GIT_NOTES_DEFAULT_REF)) {
 			strbuf_addstr(sb, "\nNotes:\n");
 		} else {
-			if (starts_with(ref, "refs/"))
-				ref += 5;
-			if (starts_with(ref, "notes/"))
-				ref += 6;
+			ref = skip_prefix_defval(ref, "refs/", ref);
+			ref = skip_prefix_defval(ref, "notes/", ref);
 			strbuf_addf(sb, "\nNotes (%s):\n", ref);
 		}
 	}
diff --git a/wt-status.c b/wt-status.c
index 4e55810..1f65039 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1179,14 +1179,10 @@ static void wt_status_get_detached_from(struct wt_status_state *state)
 	     /* perhaps sha1 is a tag, try to dereference to a commit */
 	     ((commit = lookup_commit_reference_gently(sha1, 1)) != NULL &&
 	      !hashcmp(cb.nsha1, commit->object.sha1)))) {
-		int ofs;
-		if (starts_with(ref, "refs/tags/"))
-			ofs = strlen("refs/tags/");
-		else if (starts_with(ref, "refs/remotes/"))
-			ofs = strlen("refs/remotes/");
-		else
-			ofs = 0;
-		state->detached_from = xstrdup(ref + ofs);
+		const char *p;
+		if ((p = skip_prefix_defval(ref, "refs/tags/", ref)) == ref)
+			p = skip_prefix_defval(ref, "refs/remotes/", ref);
+		state->detached_from = xstrdup(p);
 	} else
 		state->detached_from =
 			xstrdup(find_unique_abbrev(cb.nsha1, DEFAULT_ABBREV));
-- 
1.8.5.1.208.g019362e

[PATCH 04/12] Replace some use of starts_with() with skip_prefix()

[Nguyễn Thái Ngọc Duy]

All the changes follow the pattern

  if (!starts_with(foo, "bar"))
    return;
  foo += 3;

which is turned into

  if ((foo = skip_prefix(foo, "bar")) == NULL)
    return;

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/branch.c | 3 +--
 pretty.c         | 3 +--
 setup.c          | 3 +--
 tag.c            | 7 +++----
 wt-status.c      | 3 +--
 5 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index b4d7716..d063de2 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -868,9 +868,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	if (!strcmp(head, "HEAD")) {
 		detached = 1;
 	} else {
-		if (!starts_with(head, "refs/heads/"))
+		if ((head = skip_prefix(head, "refs/heads/")) == NULL)
 			die(_("HEAD not found below refs/heads!"));
-		head += 11;
 	}
 	hashcpy(merge_filter_ref, head_sha1);
 
diff --git a/pretty.c b/pretty.c
index 87db08b..08e30ec 100644
--- a/pretty.c
+++ b/pretty.c
@@ -40,10 +40,9 @@ static int git_pretty_formats_config(const char *var, const char *value, void *c
 	const char *fmt;
 	int i;
 
-	if (!starts_with(var, "pretty."))
+	if ((name = skip_prefix(var, "pretty.")) == NULL)
 		return 0;
 
-	name = var + strlen("pretty.");
 	for (i = 0; i < builtin_formats_len; i++) {
 		if (!strcmp(commit_formats[i].name, name))
 			return 0;
diff --git a/setup.c b/setup.c
index 6c3f85f..debfaab 100644
--- a/setup.c
+++ b/setup.c
@@ -304,14 +304,13 @@ const char *read_gitfile(const char *path)
 	if (len != st.st_size)
 		die("Error reading %s", path);
 	buf[len] = '\0';
-	if (!starts_with(buf, "gitdir: "))
+	if ((dir = (char *)skip_prefix(buf, "gitdir: ")) == NULL)
 		die("Invalid gitfile format: %s", path);
 	while (buf[len - 1] == '\n' || buf[len - 1] == '\r')
 		len--;
 	if (len < 9)
 		die("No path in gitfile: %s", path);
 	buf[len] = '\0';
-	dir = buf + 8;
 
 	if (!is_absolute_path(dir) && (slash = strrchr(path, '/'))) {
 		size_t pathlen = slash+1 - path;
diff --git a/tag.c b/tag.c
index 7b07921..9b63d1b 100644
--- a/tag.c
+++ b/tag.c
@@ -86,9 +86,8 @@ int parse_tag_buffer(struct tag *item, const void *data, unsigned long size)
 		return -1;
 	bufptr += 48; /* "object " + sha1 + "\n" */
 
-	if (!starts_with(bufptr, "type "))
+	if ((bufptr = skip_prefix(bufptr, "type ")) == NULL)
 		return -1;
-	bufptr += 5;
 	nl = memchr(bufptr, '\n', tail - bufptr);
 	if (!nl || sizeof(type) <= (nl - bufptr))
 		return -1;
@@ -109,11 +108,11 @@ int parse_tag_buffer(struct tag *item, const void *data, unsigned long size)
 		item->tagged = NULL;
 	}
 
-	if (bufptr + 4 < tail && starts_with(bufptr, "tag "))
+	if (bufptr + 4 < tail &&
+	    (bufptr = skip_prefix(bufptr, "tag ")) != NULL)
 		; 		/* good */
 	else
 		return -1;
-	bufptr += 4;
 	nl = memchr(bufptr, '\n', tail - bufptr);
 	if (!nl)
 		return -1;
diff --git a/wt-status.c b/wt-status.c
index 1f65039..185fa81 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1145,9 +1145,8 @@ static int grab_1st_switch(unsigned char *osha1, unsigned char *nsha1,
 	struct grab_1st_switch_cbdata *cb = cb_data;
 	const char *target = NULL, *end;
 
-	if (!starts_with(message, "checkout: moving from "))
+	if ((message = skip_prefix(message, "checkout: moving from ")) == NULL)
 		return 0;
-	message += strlen("checkout: moving from ");
 	target = strstr(message, " to ");
 	if (!target)
 		return 0;
-- 
1.8.5.1.208.g019362e

[PATCH 05/12] Convert a lot of starts_with() to skip_prefix()

[Nguyễn Thái Ngọc Duy]

The purpose is remove hard coded string length. Some could be a few
lines away from the string comparison and easy to be missed when the
string is changed.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/for-each-ref.c |  9 +++++----
 builtin/mailinfo.c     |  6 +++---
 builtin/merge.c        |  8 +++++---
 builtin/remote.c       |  3 +--
 commit.c               |  5 +----
 diff.c                 |  9 +++------
 fetch-pack.c           |  9 +++++----
 http-backend.c         |  5 +++--
 http-push.c            |  6 +++---
 http.c                 |  5 +++--
 log-tree.c             |  5 +++--
 pager.c                |  2 +-
 pathspec.c             |  5 +++--
 refs.c                 | 12 +++++++-----
 sha1_name.c            | 12 +++---------
 transport-helper.c     | 15 +++++++--------
 transport.c            | 14 ++++++++------
 17 files changed, 64 insertions(+), 66 deletions(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 6551e7b..25c1388 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -662,6 +662,7 @@ static void populate_value(struct refinfo *ref)
 		const char *refname;
 		const char *formatp;
 		struct branch *branch = NULL;
+		const char *next;
 
 		if (*name == '*') {
 			deref = 1;
@@ -674,18 +675,18 @@ static void populate_value(struct refinfo *ref)
 			refname = ref->symref ? ref->symref : "";
 		else if (starts_with(name, "upstream")) {
 			/* only local branches may have an upstream */
-			if (!starts_with(ref->refname, "refs/heads/"))
+			if ((next = skip_prefix(ref->refname, "refs/heads/")) == NULL)
 				continue;
-			branch = branch_get(ref->refname + 11);
+			branch = branch_get(next);
 
 			if (!branch || !branch->merge || !branch->merge[0] ||
 			    !branch->merge[0]->dst)
 				continue;
 			refname = branch->merge[0]->dst;
-		} else if (starts_with(name, "color:")) {
+		} else if ((next = skip_prefix(name, "color:")) != NULL) {
 			char color[COLOR_MAXLEN] = "";
 
-			color_parse(name + 6, "--format", color);
+			color_parse(next, "--format", color);
 			v->s = xstrdup(color);
 			continue;
 		} else if (!strcmp(name, "flag")) {
diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c
index 2100e23..daaafbd 100644
--- a/builtin/mailinfo.c
+++ b/builtin/mailinfo.c
@@ -328,13 +328,13 @@ static int check_header(const struct strbuf *line,
 	}
 
 	/* for inbody stuff */
-	if (starts_with(line->buf, ">From") && isspace(line->buf[5])) {
+	if (isspace(*skip_prefix_defval(line->buf, ">From", "NOSPACE"))) {
 		ret = 1; /* Should this return 0? */
 		goto check_header_out;
 	}
-	if (starts_with(line->buf, "[PATCH]") && isspace(line->buf[7])) {
+	if (isspace(*skip_prefix_defval(line->buf, "[PATCH]", "NOSPACE"))) {
 		for (i = 0; header[i]; i++) {
-			if (!memcmp("Subject", header[i], 7)) {
+			if (starts_with(header[i], "Subject")) {
 				handle_header(&hdr_data[i], line);
 				ret = 1;
 				goto check_header_out;
diff --git a/builtin/merge.c b/builtin/merge.c
index 590d907..603f80a 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -569,10 +569,12 @@ static void parse_branch_merge_options(char *bmo)
 static int git_merge_config(const char *k, const char *v, void *cb)
 {
 	int status;
+	const char *kk, *kkk;
 
-	if (branch && starts_with(k, "branch.") &&
-		starts_with(k + 7, branch) &&
-		!strcmp(k + 7 + strlen(branch), ".mergeoptions")) {
+	if (branch &&
+	    (kk = skip_prefix(k, "branch.")) != NULL &&
+	    (kkk = skip_prefix(kk, branch)) != NULL &&
+	    !strcmp(kkk, ".mergeoptions")) {
 		free(branch_mergeoptions);
 		branch_mergeoptions = xstrdup(v);
 		return 0;
diff --git a/builtin/remote.c b/builtin/remote.c
index b3ab4cf..218c8c8 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -259,14 +259,13 @@ static const char *abbrev_ref(const char *name, const char *prefix)
 
 static int config_read_branches(const char *key, const char *value, void *cb)
 {
-	if (starts_with(key, "branch.")) {
+	if ((key = skip_prefix(key, "branch.")) != NULL) {
 		const char *orig_key = key;
 		char *name;
 		struct string_list_item *item;
 		struct branch_info *info;
 		enum { REMOTE, MERGE, REBASE } type;
 
-		key += 7;
 		if (ends_with(key, ".remote")) {
 			name = xstrndup(key, strlen(key) - 7);
 			type = REMOTE;
diff --git a/commit.c b/commit.c
index 5df1df7..eed2ff9 100644
--- a/commit.c
+++ b/commit.c
@@ -1193,10 +1193,7 @@ static void parse_gpg_output(struct signature_check *sigc)
 	for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {
 		const char *found, *next;
 
-		if (starts_with(buf, sigcheck_gpg_status[i].check + 1)) {
-			/* At the very beginning of the buffer */
-			found = buf + strlen(sigcheck_gpg_status[i].check + 1);
-		} else {
+		if ((found = skip_prefix(buf, sigcheck_gpg_status[i].check + 1)) == NULL) {
 			found = strstr(buf, sigcheck_gpg_status[i].check);
 			if (!found)
 				continue;
diff --git a/diff.c b/diff.c
index 90a1929..d754e2f 100644
--- a/diff.c
+++ b/diff.c
@@ -3388,13 +3388,10 @@ static inline int short_opt(char opt, const char **argv,
 int parse_long_opt(const char *opt, const char **argv,
 		   const char **optarg)
 {
-	const char *arg = argv[0];
-	if (arg[0] != '-' || arg[1] != '-')
-		return 0;
-	arg += strlen("--");
-	if (!starts_with(arg, opt))
+	const char *arg;
+	if ((arg = skip_prefix(argv[0], "--")) == NULL ||
+	    (arg = skip_prefix(arg, opt)) == NULL)
 		return 0;
-	arg += strlen(opt);
 	if (*arg == '=') { /* stuck form: --option=value */
 		*optarg = arg + 1;
 		return 1;
diff --git a/fetch-pack.c b/fetch-pack.c
index 760ed16..723ff06 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -317,18 +317,19 @@ static int find_common(struct fetch_pack_args *args,
 
 	if (args->depth > 0) {
 		char *line;
+		const char *sha1_str;
 		unsigned char sha1[20];
 
 		send_request(args, fd[1], &req_buf);
 		while ((line = packet_read_line(fd[0], NULL))) {
-			if (starts_with(line, "shallow ")) {
-				if (get_sha1_hex(line + 8, sha1))
+			if ((sha1_str = skip_prefix(line, "shallow ")) != NULL) {
+				if (get_sha1_hex(sha1_str, sha1))
 					die("invalid shallow line: %s", line);
 				register_shallow(sha1);
 				continue;
 			}
-			if (starts_with(line, "unshallow ")) {
-				if (get_sha1_hex(line + 10, sha1))
+			if ((sha1_str = skip_prefix(line, "unshallow ")) != NULL) {
+				if (get_sha1_hex(sha1_str, sha1))
 					die("invalid unshallow line: %s", line);
 				if (!lookup_object(sha1))
 					die("object not found: %s", line);
diff --git a/http-backend.c b/http-backend.c
index d2c0a62..e780c55 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -221,17 +221,18 @@ static void get_idx_file(char *name)
 
 static int http_config(const char *var, const char *value, void *cb)
 {
+	const char *p;
 	if (!strcmp(var, "http.getanyfile")) {
 		getanyfile = git_config_bool(var, value);
 		return 0;
 	}
 
-	if (starts_with(var, "http.")) {
+	if ((p = skip_prefix(var, "http.")) != NULL) {
 		int i;
 
 		for (i = 0; i < ARRAY_SIZE(rpc_service); i++) {
 			struct rpc_service *svc = &rpc_service[i];
-			if (!strcmp(var + 5, svc->config_name)) {
+			if (!strcmp(p, svc->config_name)) {
 				svc->enabled = git_config_bool(var, value);
 				return 0;
 			}
diff --git a/http-push.c b/http-push.c
index d4b40c9..5db6f28 100644
--- a/http-push.c
+++ b/http-push.c
@@ -771,9 +771,9 @@ static void handle_new_lock_ctx(struct xml_ctx *ctx, int tag_closed)
 			lock->owner = xmalloc(strlen(ctx->cdata) + 1);
 			strcpy(lock->owner, ctx->cdata);
 		} else if (!strcmp(ctx->name, DAV_ACTIVELOCK_TIMEOUT)) {
-			if (starts_with(ctx->cdata, "Second-"))
-				lock->timeout =
-					strtol(ctx->cdata + 7, NULL, 10);
+			const char *p;
+			if ((p = skip_prefix(ctx->cdata, "Second-")) != NULL)
+				lock->timeout = strtol(p, NULL, 10);
 		} else if (!strcmp(ctx->name, DAV_ACTIVELOCK_TOKEN)) {
 			lock->token = xmalloc(strlen(ctx->cdata) + 1);
 			strcpy(lock->token, ctx->cdata);
diff --git a/http.c b/http.c
index 70eaa26..1120ed2 100644
--- a/http.c
+++ b/http.c
@@ -1098,6 +1098,7 @@ int http_fetch_ref(const char *base, struct ref *ref)
 	char *url;
 	struct strbuf buffer = STRBUF_INIT;
 	int ret = -1;
+	const char *p;
 
 	options.no_cache = 1;
 
@@ -1106,8 +1107,8 @@ int http_fetch_ref(const char *base, struct ref *ref)
 		strbuf_rtrim(&buffer);
 		if (buffer.len == 40)
 			ret = get_sha1_hex(buffer.buf, ref->old_sha1);
-		else if (starts_with(buffer.buf, "ref: ")) {
-			ref->symref = xstrdup(buffer.buf + 5);
+		else if ((p = skip_prefix(buffer.buf, "ref: ")) != NULL) {
+			ref->symref = xstrdup(p);
 			ret = 0;
 		}
 	}
diff --git a/log-tree.c b/log-tree.c
index 642faff..cef7c8d 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -96,13 +96,14 @@ static void add_name_decoration(enum decoration_type type, const char *name, str
 static int add_ref_decoration(const char *refname, const unsigned char *sha1, int flags, void *cb_data)
 {
 	struct object *obj;
+	const char *name;
 	enum decoration_type type = DECORATION_NONE;
 
-	if (starts_with(refname, "refs/replace/")) {
+	if ((name = skip_prefix(refname, "refs/replace/")) != NULL) {
 		unsigned char original_sha1[20];
 		if (!read_replace_refs)
 			return 0;
-		if (get_sha1_hex(refname + 13, original_sha1)) {
+		if (get_sha1_hex(name, original_sha1)) {
 			warning("invalid replace ref %s", refname);
 			return 0;
 		}
diff --git a/pager.c b/pager.c
index 345b0bc..175cd9f 100644
--- a/pager.c
+++ b/pager.c
@@ -151,7 +151,7 @@ int decimal_width(int number)
 static int pager_command_config(const char *var, const char *value, void *data)
 {
 	struct pager_config *c = data;
-	if (starts_with(var, "pager.") && !strcmp(var + 6, c->cmd)) {
+	if (!strcmp(skip_prefix_defval(var, "pager.", ""), c->cmd)) {
 		int b = git_config_maybe_bool(var, value);
 		if (b >= 0)
 			c->want = b;
diff --git a/pathspec.c b/pathspec.c
index 52d38a4..e15f215 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -149,14 +149,15 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
 			if (!len)
 				continue;
 			for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
+				const char *prefix_str;
 				if (strlen(pathspec_magic[i].name) == len &&
 				    !strncmp(pathspec_magic[i].name, copyfrom, len)) {
 					magic |= pathspec_magic[i].bit;
 					break;
 				}
-				if (starts_with(copyfrom, "prefix:")) {
+				if ((prefix_str = skip_prefix(copyfrom, "prefix:")) != NULL) {
 					char *endptr;
-					pathspec_prefix = strtol(copyfrom + 7,
+					pathspec_prefix = strtol(prefix_str,
 								 &endptr, 10);
 					if (endptr - copyfrom != len)
 						die(_("invalid parameter for pathspec magic 'prefix'"));
diff --git a/refs.c b/refs.c
index 3926136..5e378bc 100644
--- a/refs.c
+++ b/refs.c
@@ -1873,11 +1873,13 @@ int for_each_rawref(each_ref_fn fn, void *cb_data)
 
 const char *prettify_refname(const char *name)
 {
-	return name + (
-		starts_with(name, "refs/heads/") ? 11 :
-		starts_with(name, "refs/tags/") ? 10 :
-		starts_with(name, "refs/remotes/") ? 13 :
-		0);
+	const char *p;
+	if ((p = skip_prefix(name, "refs/heads/")) != NULL ||
+	    (p = skip_prefix(name, "refs/tags/")) != NULL ||
+	    (p = skip_prefix(name, "refs/remotes/")) != NULL)
+		return p;
+	else
+		return name;
 }
 
 const char *ref_rev_parse_rules[] = {
diff --git a/sha1_name.c b/sha1_name.c
index b1873d8..3fc4ede 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -546,14 +546,10 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 		if (read_ref_at(real_ref, at_time, nth, sha1, NULL,
 				&co_time, &co_tz, &co_cnt)) {
 			if (!len) {
-				if (starts_with(real_ref, "refs/heads/")) {
-					str = real_ref + 11;
-					len = strlen(real_ref + 11);
-				} else {
+				if ((str = skip_prefix(real_ref, "refs/heads/")) == NULL)
 					/* detached HEAD */
 					str = "HEAD";
-					len = 4;
-				}
+				len = strlen(str);
 			}
 			if (at_time)
 				warning("Log for '%.*s' only goes "
@@ -909,10 +905,8 @@ static int grab_nth_branch_switch(unsigned char *osha1, unsigned char *nsha1,
 	const char *match = NULL, *target = NULL;
 	size_t len;
 
-	if (starts_with(message, "checkout: moving from ")) {
-		match = message + strlen("checkout: moving from ");
+	if ((match = skip_prefix(message, "checkout: moving from ")) != NULL)
 		target = strstr(match, " to ");
-	}
 
 	if (!match || !target)
 		return 0;
diff --git a/transport-helper.c b/transport-helper.c
index 2010674..601aba8 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -373,10 +373,10 @@ static int fetch_with_fetch(struct transport *transport,
 	sendline(data, &buf);
 
 	while (1) {
+		const char *name;
 		recvline(data, &buf);
 
-		if (starts_with(buf.buf, "lock ")) {
-			const char *name = buf.buf + 5;
+		if ((name = skip_prefix(buf.buf, "lock ")) != NULL) {
 			if (transport->pack_lockfile)
 				warning("%s also locked %s", data->name, name);
 			else
@@ -643,16 +643,15 @@ static int push_update_ref_status(struct strbuf *buf,
 				   struct ref **ref,
 				   struct ref *remote_refs)
 {
-	char *refname, *msg;
+	const char *refname;
+	char *msg;
 	int status;
 
-	if (starts_with(buf->buf, "ok ")) {
+	if ((refname = skip_prefix(buf->buf, "ok ")) != NULL)
 		status = REF_STATUS_OK;
-		refname = buf->buf + 3;
-	} else if (starts_with(buf->buf, "error ")) {
+	else if ((refname = skip_prefix(buf->buf, "error ")) != NULL)
 		status = REF_STATUS_REMOTE_REJECT;
-		refname = buf->buf + 6;
-	} else
+	else
 		die("expected ok/error, helper said '%s'", buf->buf);
 
 	msg = strchr(refname, ' ');
diff --git a/transport.c b/transport.c
index 824c5b9..e88c2dc 100644
--- a/transport.c
+++ b/transport.c
@@ -147,9 +147,9 @@ static void set_upstreams(struct transport *transport, struct ref *refs,
 {
 	struct ref *ref;
 	for (ref = refs; ref; ref = ref->next) {
-		const char *localname;
+		const char *localname, *short_local;
 		const char *tmp;
-		const char *remotename;
+		const char *remotename, *short_remote;
 		unsigned char sha[20];
 		int flag = 0;
 		/*
@@ -173,18 +173,20 @@ static void set_upstreams(struct transport *transport, struct ref *refs,
 			localname = tmp;
 
 		/* Both source and destination must be local branches. */
-		if (!localname || !starts_with(localname, "refs/heads/"))
+		if (!localname ||
+		    (short_local = skip_prefix(localname, "refs/heads/")) == NULL)
 			continue;
-		if (!remotename || !starts_with(remotename, "refs/heads/"))
+		if (!remotename ||
+		    (short_remote = skip_prefix(remotename, "refs/heads/")) == NULL)
 			continue;
 
 		if (!pretend)
 			install_branch_config(BRANCH_CONFIG_VERBOSE,
-				localname + 11, transport->remote->name,
+				short_local, transport->remote->name,
 				remotename);
 		else
 			printf("Would set upstream of '%s' to '%s' of '%s'\n",
-				localname + 11, remotename + 11,
+				short_local, short_remote,
 				transport->remote->name);
 	}
 }
-- 
1.8.5.1.208.g019362e

[PATCH 06/12] fetch.c: replace some use of starts_with() with skip_prefix()

[Nguyễn Thái Ngọc Duy]

Variable "what" is set in an if/else chain. If all fails (and "what"
is set to NULL) it'll be reset in the final "else" block.

in get_remote_group(), "key" is only used once. So changing it does
not harm.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/fetch.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 1e7d617..38f4f7b 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -589,18 +589,12 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 				kind = "";
 				what = "";
 			}
-			else if (starts_with(rm->name, "refs/heads/")) {
+			else if ((what = skip_prefix(rm->name, "refs/heads/")) != NULL)
 				kind = "branch";
-				what = rm->name + 11;
-			}
-			else if (starts_with(rm->name, "refs/tags/")) {
+			else if ((what = skip_prefix(rm->name, "refs/tags/")) != NULL)
 				kind = "tag";
-				what = rm->name + 10;
-			}
-			else if (starts_with(rm->name, "refs/remotes/")) {
+			else if ((what = skip_prefix(rm->name, "refs/remotes/")) != NULL)
 				kind = "remote-tracking branch";
-				what = rm->name + 13;
-			}
 			else {
 				kind = "";
 				what = rm->name;
@@ -896,8 +890,8 @@ static int get_remote_group(const char *key, const char *value, void *priv)
 {
 	struct remote_group_data *g = priv;
 
-	if (starts_with(key, "remotes.") &&
-			!strcmp(key + 8, g->name)) {
+	if ((key = skip_prefix(key, "remotes.")) != NULL &&
+	    !strcmp(key, g->name)) {
 		/* split list by white space */
 		int space = strcspn(value, " \t\n");
 		while (*value) {
-- 
1.8.5.1.208.g019362e

[PATCH 07/12] connect.c: replace some use of starts_with() with skip_prefix()

[Nguyễn Thái Ngọc Duy]

"name" will be reset unconditionally soon after skip_prefix() returns
NULL.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 connect.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/connect.c b/connect.c
index c763eed..1bb70aa 100644
--- a/connect.c
+++ b/connect.c
@@ -131,7 +131,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 	for (;;) {
 		struct ref *ref;
 		unsigned char old_sha1[20];
-		char *name;
+		const char *name;
 		int len, name_len;
 		char *buffer = packet_buffer;
 
@@ -145,8 +145,8 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 		if (!len)
 			break;
 
-		if (len > 4 && starts_with(buffer, "ERR "))
-			die("remote error: %s", buffer + 4);
+		if ((name = skip_prefix(buffer, "ERR ")) != NULL)
+			die("remote error: %s", name);
 
 		if (len < 42 || get_sha1_hex(buffer, old_sha1) || buffer[40] != ' ')
 			die("protocol error: expected sha/ref, got '%s'", buffer);
-- 
1.8.5.1.208.g019362e

[PATCH 08/12] refs.c: replace some use of starts_with() with skip_prefix()

[Nguyễn Thái Ngọc Duy]

It's out of context, but there are neither changes in buffer nor buf
between two chunks.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 refs.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/refs.c b/refs.c
index 5e378bc..1fb658f 100644
--- a/refs.c
+++ b/refs.c
@@ -1339,7 +1339,7 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea
 	for (;;) {
 		char path[PATH_MAX];
 		struct stat st;
-		char *buf;
+		const char *buf;
 		int fd;
 
 		if (--depth < 0)
@@ -1415,7 +1415,7 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea
 		/*
 		 * Is it a symbolic ref?
 		 */
-		if (!starts_with(buffer, "ref:")) {
+		if ((buf = skip_prefix(buffer, "ref:")) == NULL) {
 			/*
 			 * Please note that FETCH_HEAD has a second
 			 * line containing other data.
@@ -1430,7 +1430,6 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea
 		}
 		if (flag)
 			*flag |= REF_ISSYMREF;
-		buf = buffer + 4;
 		while (isspace(*buf))
 			buf++;
 		if (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {
-- 
1.8.5.1.208.g019362e

[PATCH 09/12] diff.c: reduce code duplication in --stat-xxx parsing

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c | 67 +++++++++++++++++++++++++-----------------------------------------
 1 file changed, 25 insertions(+), 42 deletions(-)

diff --git a/diff.c b/diff.c
index d754e2f..4da77fd 100644
--- a/diff.c
+++ b/diff.c
@@ -3405,6 +3405,23 @@ int parse_long_opt(const char *opt, const char **argv,
 	return 2;
 }
 
+static int parse_statopt(const char *arg, const char *next,
+			 const char *opt, int *value, char **end)
+{
+	const char *p = skip_prefix(arg, opt);
+	if (!p)
+		return 0;
+	if (*p == '=')
+		*value = strtoul(p + 1, end, 10);
+	else if (!*p && !next)
+		die("Option '--stat%s' requires a value", opt);
+	else if (!*p) {
+		*value = strtoul(next, end, 10);
+		return 2;
+	}
+	return 1;
+}
+
 static int stat_opt(struct diff_options *options, const char **av)
 {
 	const char *arg = av[0];
@@ -3417,50 +3434,16 @@ static int stat_opt(struct diff_options *options, const char **av)
 
 	arg += strlen("--stat");
 	end = (char *)arg;
-
 	switch (*arg) {
 	case '-':
-		if (starts_with(arg, "-width")) {
-			arg += strlen("-width");
-			if (*arg == '=')
-				width = strtoul(arg + 1, &end, 10);
-			else if (!*arg && !av[1])
-				die("Option '--stat-width' requires a value");
-			else if (!*arg) {
-				width = strtoul(av[1], &end, 10);
-				argcount = 2;
-			}
-		} else if (starts_with(arg, "-name-width")) {
-			arg += strlen("-name-width");
-			if (*arg == '=')
-				name_width = strtoul(arg + 1, &end, 10);
-			else if (!*arg && !av[1])
-				die("Option '--stat-name-width' requires a value");
-			else if (!*arg) {
-				name_width = strtoul(av[1], &end, 10);
-				argcount = 2;
-			}
-		} else if (starts_with(arg, "-graph-width")) {
-			arg += strlen("-graph-width");
-			if (*arg == '=')
-				graph_width = strtoul(arg + 1, &end, 10);
-			else if (!*arg && !av[1])
-				die("Option '--stat-graph-width' requires a value");
-			else if (!*arg) {
-				graph_width = strtoul(av[1], &end, 10);
-				argcount = 2;
-			}
-		} else if (starts_with(arg, "-count")) {
-			arg += strlen("-count");
-			if (*arg == '=')
-				count = strtoul(arg + 1, &end, 10);
-			else if (!*arg && !av[1])
-				die("Option '--stat-count' requires a value");
-			else if (!*arg) {
-				count = strtoul(av[1], &end, 10);
-				argcount = 2;
-			}
-		}
+		if ((argcount = parse_statopt(arg, av[1], "-width", &width, &end)) != 0 ||
+		    (argcount = parse_statopt(arg, av[1], "-name-width", &name_width, &end)) != 0 ||
+		    (argcount = parse_statopt(arg, av[1], "-graph-width", &graph_width, &end)) != 0 ||
+		    (argcount = parse_statopt(arg, av[1], "-count", &count, &end)) != 0)
+			/* nothing else, it's the OR chain that's important */
+			;
+		else
+			argcount = 1;
 		break;
 	case '=':
 		width = strtoul(arg+1, &end, 10);
-- 
1.8.5.1.208.g019362e

[PATCH 10/12] environment.c: replace starts_with() in strip_namespace() with skip_prefix()

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 environment.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/environment.c b/environment.c
index 3c76905..bc2d916 100644
--- a/environment.c
+++ b/environment.c
@@ -171,9 +171,7 @@ const char *get_git_namespace(void)
 
 const char *strip_namespace(const char *namespaced_ref)
 {
-	if (!starts_with(namespaced_ref, get_git_namespace()))
-		return NULL;
-	return namespaced_ref + namespace_len;
+	return skip_prefix(namespaced_ref, get_git_namespace());
 }
 
 static int git_work_tree_initialized;
-- 
1.8.5.1.208.g019362e

[PATCH 11/12] diff.c: convert diff_scoreopt_parse to use skip_prefix()

[Nguyễn Thái Ngọc Duy]

While at there, partly fix the reporting as well. The reported value
with "arg+2" is only correct with -C/-B/-M, not with long option names.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/diff.c b/diff.c
index 4da77fd..d629cc5 100644
--- a/diff.c
+++ b/diff.c
@@ -3367,7 +3367,7 @@ static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *va
 	return 1;
 }
 
-static int diff_scoreopt_parse(const char *opt);
+static int diff_scoreopt_parse(const char **opt);
 
 static inline int short_opt(char opt, const char **argv,
 			    const char **optarg)
@@ -3627,13 +3627,13 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 	/* renames options */
 	else if (starts_with(arg, "-B") || starts_with(arg, "--break-rewrites=") ||
 		 !strcmp(arg, "--break-rewrites")) {
-		if ((options->break_opt = diff_scoreopt_parse(arg)) == -1)
-			return error("invalid argument to -B: %s", arg+2);
+		if ((options->break_opt = diff_scoreopt_parse(&arg)) == -1)
+			return error("invalid argument to -B: %s", arg);
 	}
 	else if (starts_with(arg, "-M") || starts_with(arg, "--find-renames=") ||
 		 !strcmp(arg, "--find-renames")) {
-		if ((options->rename_score = diff_scoreopt_parse(arg)) == -1)
-			return error("invalid argument to -M: %s", arg+2);
+		if ((options->rename_score = diff_scoreopt_parse(&arg)) == -1)
+			return error("invalid argument to -M: %s", arg);
 		options->detect_rename = DIFF_DETECT_RENAME;
 	}
 	else if (!strcmp(arg, "-D") || !strcmp(arg, "--irreversible-delete")) {
@@ -3643,8 +3643,8 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		 !strcmp(arg, "--find-copies")) {
 		if (options->detect_rename == DIFF_DETECT_COPY)
 			DIFF_OPT_SET(options, FIND_COPIES_HARDER);
-		if ((options->rename_score = diff_scoreopt_parse(arg)) == -1)
-			return error("invalid argument to -C: %s", arg+2);
+		if ((options->rename_score = diff_scoreopt_parse(&arg)) == -1)
+			return error("invalid argument to -C: %s", arg);
 		options->detect_rename = DIFF_DETECT_COPY;
 	}
 	else if (!strcmp(arg, "--no-renames"))
@@ -3879,29 +3879,29 @@ int parse_rename_score(const char **cp_p)
 	return (int)((num >= scale) ? MAX_SCORE : (MAX_SCORE * num / scale));
 }
 
-static int diff_scoreopt_parse(const char *opt)
+static int diff_scoreopt_parse(const char **opt_p)
 {
+	const char *opt = *opt_p;
 	int opt1, opt2, cmd;
 
 	if (*opt++ != '-')
 		return -1;
 	cmd = *opt++;
+	*opt_p = opt;
 	if (cmd == '-') {
 		/* convert the long-form arguments into short-form versions */
-		if (starts_with(opt, "break-rewrites")) {
-			opt += strlen("break-rewrites");
+		if ((opt = skip_prefix_defval(opt, "break-rewrites", *opt_p)) != *opt_p) {
 			if (*opt == 0 || *opt++ == '=')
 				cmd = 'B';
-		} else if (starts_with(opt, "find-copies")) {
-			opt += strlen("find-copies");
+		} else if ((opt = skip_prefix_defval(opt, "find-copies", *opt_p)) != *opt_p) {
 			if (*opt == 0 || *opt++ == '=')
 				cmd = 'C';
-		} else if (starts_with(opt, "find-renames")) {
-			opt += strlen("find-renames");
+		} else if ((opt = skip_prefix_defval(opt, "find-renames", *opt_p)) != *opt_p) {
 			if (*opt == 0 || *opt++ == '=')
 				cmd = 'M';
 		}
 	}
+	*opt_p = opt;
 	if (cmd != 'M' && cmd != 'C' && cmd != 'B')
 		return -1; /* that is not a -M, -C nor -B option */
 
-- 
1.8.5.1.208.g019362e

[PATCH 12/12] refs.c: use skip_prefix() in prune_ref()

[Nguyễn Thái Ngọc Duy]

This removes the magic number 5, which is the string length of
"refs/". This comes from get_loose_refs(), called in packed_refs().

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 refs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index 1fb658f..217093f 100644
--- a/refs.c
+++ b/refs.c
@@ -2318,7 +2318,8 @@ static void try_remove_empty_parents(char *name)
 /* make sure nobody touched the ref, and unlink */
 static void prune_ref(struct ref_to_prune *r)
 {
-	struct ref_lock *lock = lock_ref_sha1(r->name + 5, r->sha1);
+	const char *name = skip_prefix_defval(r->name, "refs/", r->name);
+	struct ref_lock *lock = lock_ref_sha1(name, r->sha1);
 
 	if (lock) {
 		unlink_or_warn(git_path("%s", r->name));
-- 
1.8.5.1.208.g019362e

Re: [PATCH 03/12] Add and use skip_prefix_defval()

[Kent R. Spillner]

On Wed, Dec 18, 2013 at 09:53:48PM +0700, Nguyễn Thái Ngọc Duy wrote:
> This is a variant of skip_prefix() that returns a specied pointer
> instead of NULL if no prefix is found. It's helpful to simplify
> 
>   if (starts_with(foo, "bar"))
>     foo += 3;
> 
> into
> 
>   foo = skip_prefix_gently(foo, "bar", foo);

Should this be skip_prefix_defval instead of skip_prefix_gently?

Re: [PATCH 01/12] Make starts_with() a wrapper of skip_prefix()

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> starts_with() started out as a copy of prefixcmp(). But if we don't
> care about the sorting order, the logic looks closer to
> skip_prefix(). This looks like a good thing to do.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---

Sure, but the implementation of skip_prefix() scans the prefix
string twice, while prefixcmp() aka starts_with() does it only once.

I'd expect a later step in this series would rectify this micro
regression in the performance, though ;-)

>  git-compat-util.h | 6 +++++-
>  strbuf.c          | 9 ---------
>  2 files changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index b73916b..84f1078 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -350,7 +350,6 @@ 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 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);
> @@ -361,6 +360,11 @@ static inline const char *skip_prefix(const char *str, const char *prefix)
>  	return strncmp(str, prefix, len) ? NULL : str + len;
>  }
>  
> +static inline int starts_with(const char *str, const char *prefix)
> +{
> +	return skip_prefix(str, prefix) != NULL;
> +}
> +
>  #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)
>  
>  #ifndef PROT_READ
> diff --git a/strbuf.c b/strbuf.c
> index 83caf4a..bd4c0d8 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -1,15 +1,6 @@
>  #include "cache.h"
>  #include "refs.h"
>  
> -int starts_with(const char *str, const char *prefix)
> -{
> -	for (; ; str++, prefix++)
> -		if (!*prefix)
> -			return 1;
> -		else if (*str != *prefix)
> -			return 0;
> -}
> -
>  int prefixcmp(const char *str, const char *prefix)
>  {
>  	for (; ; str++, prefix++)

Re: [PATCH 03/12] Add and use skip_prefix_defval()

[Junio C Hamano]

"Kent R. Spillner" <kspillner@acm.org> writes:

> On Wed, Dec 18, 2013 at 09:53:48PM +0700, Nguyễn Thái Ngọc Duy wrote:
>> This is a variant of skip_prefix() that returns a specied pointer
>> instead of NULL if no prefix is found. It's helpful to simplify
>> 
>>   if (starts_with(foo, "bar"))
>>     foo += 3;
>> 
>> into
>> 
>>   foo = skip_prefix_gently(foo, "bar", foo);
>
> Should this be skip_prefix_defval instead of skip_prefix_gently?

Yes.

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

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> 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 :)

Overall the goal of getting rid of the "if the first N bytes of the
ptr match this string, advance ptr by N bytes" pattern, whether the
first N is implicitly given by using prefixcmp() or explicitly given
by using memcmp(), is very good.  And the result looked good from a
cursory read---though I have not gone through the series with fine
toothed comb yet.

Thanks.

>
> Nguyễn Thái Ngọc Duy (12):
>   Make starts_with() a wrapper of skip_prefix()
>   Convert starts_with() to skip_prefix() for option parsing
>   Add and use skip_prefix_defval()
>   Replace some use of starts_with() with skip_prefix()
>   Convert a lot of starts_with() to skip_prefix()
>   fetch.c: replace some use of starts_with() with skip_prefix()
>   connect.c: replace some use of starts_with() with skip_prefix()
>   refs.c: replace some use of starts_with() with skip_prefix()
>   diff.c: reduce code duplication in --stat-xxx parsing
>   environment.c: replace starts_with() in strip_namespace() with skip_prefix()
>   diff.c: convert diff_scoreopt_parse to use skip_prefix()
>   refs.c: use skip_prefix() in prune_ref()
>
>  builtin/branch.c         |   3 +-
>  builtin/checkout.c       |   6 +-
>  builtin/fast-export.c    |   3 +-
>  builtin/fetch-pack.c     |  13 ++--
>  builtin/fetch.c          |  16 ++---
>  builtin/for-each-ref.c   |   9 +--
>  builtin/index-pack.c     |  17 +++---
>  builtin/ls-remote.c      |   9 +--
>  builtin/mailinfo.c       |  11 ++--
>  builtin/merge.c          |  12 ++--
>  builtin/reflog.c         |   9 +--
>  builtin/remote.c         |   3 +-
>  builtin/rev-parse.c      |  41 ++++++-------
>  builtin/send-pack.c      |  18 +++---
>  builtin/show-branch.c    |  14 ++---
>  builtin/unpack-objects.c |   5 +-
>  builtin/update-ref.c     |  21 +++----
>  commit.c                 |   5 +-
>  connect.c                |   6 +-
>  daemon.c                 |  75 +++++++++++------------
>  diff.c                   | 153 +++++++++++++++++++++--------------------------
>  environment.c            |   4 +-
>  fetch-pack.c             |   9 +--
>  git-compat-util.h        |  15 ++++-
>  git.c                    |  16 ++---
>  http-backend.c           |   5 +-
>  http-push.c              |   6 +-
>  http.c                   |   5 +-
>  log-tree.c               |   5 +-
>  merge-recursive.c        |  13 ++--
>  notes.c                  |   6 +-
>  pager.c                  |   2 +-
>  pathspec.c               |   5 +-
>  pretty.c                 |   3 +-
>  refs.c                   |  20 ++++---
>  revision.c               |  60 +++++++++----------
>  setup.c                  |   3 +-
>  sha1_name.c              |  12 +---
>  strbuf.c                 |   9 ---
>  tag.c                    |   7 +--
>  transport-helper.c       |  15 +++--
>  transport.c              |  14 +++--
>  upload-pack.c            |   5 +-
>  wt-status.c              |  15 ++---
>  44 files changed, 334 insertions(+), 369 deletions(-)

Re: [PATCH 01/12] Make starts_with() a wrapper of skip_prefix()

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> starts_with() started out as a copy of prefixcmp(). But if we don't
>> care about the sorting order, the logic looks closer to
>> skip_prefix(). This looks like a good thing to do.
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>
> Sure, but the implementation of skip_prefix() scans the prefix
> string twice, while prefixcmp() aka starts_with() does it only once.
>
> I'd expect a later step in this series would rectify this micro
> regression in the performance, though ;-)

... and I did not see one, but it would be trivial on top.

 git-compat-util.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index b72a80d..59265af 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -356,8 +356,11 @@ extern int suffixcmp(const char *str, const char *suffix);
 
 static inline const char *skip_prefix_defval(const char *str, const char *prefix, const char *defval)
 {
-	size_t len = strlen(prefix);
-	return strncmp(str, prefix, len) ? defval : str + len;
+	for ( ; ; str++, prefix++)
+		if (!*prefix)
+			return str;
+		else if (*str != *prefix)
+			return defval;
 }
 
 static inline const char *skip_prefix(const char *str, const char *prefix)

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

[René Scharfe]

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

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

[Duy Nguyen]

On Fri, Dec 20, 2013 at 6:32 AM, René Scharfe <l.s.r@web.de> wrote:
> 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?

That was skip_prefix_gently() that I forgot to replace in a commit
message, before I turned it into _defval variant. The reason for
_defval is it could be use to chain expression together without adding
temporary variables, e.g.

-       if (starts_with(line->buf, ">From") && isspace(line->buf[5])) {
+       if (isspace(*skip_prefix_defval(line->buf, ">From", "NOSPACE"))) {

Without _defval, one would need to do if ((p = skip_prefix(..)) &&
isspace(*p)). I'm not entirely sure this is a good thing though as it
could make it a bit harder to read.
-- 
Duy

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

[René Scharfe]

Am 20.12.2013 00:50, schrieb Duy Nguyen:
> On Fri, Dec 20, 2013 at 6:32 AM, René Scharfe <l.s.r@web.de> wrote:
>> 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?
> 
> That was skip_prefix_gently() that I forgot to replace in a commit
> message, before I turned it into _defval variant. The reason for
> _defval is it could be use to chain expression together without adding
> temporary variables, e.g.
> 
> -       if (starts_with(line->buf, ">From") && isspace(line->buf[5])) {
> +       if (isspace(*skip_prefix_defval(line->buf, ">From", "NOSPACE"))) {
> 
> Without _defval, one would need to do if ((p = skip_prefix(..)) &&
> isspace(*p)). I'm not entirely sure this is a good thing though as it
> could make it a bit harder to read.

That usage is quite rare compared to occurrences of
skip_prefix_defval(foo, prefix, foo), no?  Adding a temporary variable
for them wouldn't be that bad if we can simplify the API to a single
function -- if that one is usable, that is.

On the other hand, we could add a special function for that example
and we'd already have three users in the tree (patch below).  I think
that's too narrow a use case for a library function, though.  Doing
the following instead in the three cases doesn't seem to be too bad:

	rest = skip_prefix(line->buf, ">From");
	if (rest != line->buf && isspace(*rest)) {

---
 builtin/apply.c    | 2 +-
 builtin/mailinfo.c | 4 ++--
 git-compat-util.h  | 1 +
 strbuf.c           | 9 +++++++++
 4 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index b0d0986..b96befd 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -433,7 +433,7 @@ static unsigned long linelen(const char *buffer, unsigned long size)
 
 static int is_dev_null(const char *str)
 {
-	return !memcmp("/dev/null", str, 9) && isspace(str[9]);
+	return skip_prefix_and_space(str, "/dev/null") != str;
 }
 
 #define TERM_SPACE	1
diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c
index 2c3cd8e..2575989 100644
--- a/builtin/mailinfo.c
+++ b/builtin/mailinfo.c
@@ -328,11 +328,11 @@ static int check_header(const struct strbuf *line,
 	}
 
 	/* for inbody stuff */
-	if (starts_with(line->buf, ">From") && isspace(line->buf[5])) {
+	if (skip_prefix_and_space(line->buf, ">From") != line->buf) {
 		ret = 1; /* Should this return 0? */
 		goto check_header_out;
 	}
-	if (starts_with(line->buf, "[PATCH]") && isspace(line->buf[7])) {
+	if (skip_prefix_and_space(line->buf, "[PATCH]") != line->buf) {
 		for (i = 0; header[i]; i++) {
 			if (!memcmp("Subject", header[i], 7)) {
 				handle_header(&hdr_data[i], line);
diff --git a/git-compat-util.h b/git-compat-util.h
index dcb92c4..a083918 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -355,6 +355,7 @@ 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);
 extern const char *skip_prefix(const char *str, const char *prefix);
+extern const char *skip_prefix_and_space(const char *str, const char *prefix);
 
 #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)
 
diff --git a/strbuf.c b/strbuf.c
index 222df13..768331f 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -47,6 +47,15 @@ const char *skip_prefix(const char *str, const char *prefix)
 			return str;
 }
 
+const char *skip_prefix_and_space(const char *str, const char *prefix)
+{
+	const char *p = skip_prefix(str, prefix);
+	if (((p != str) || !*prefix) && isspace(*p))
+		return p + 1;
+	else
+		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
-- 
1.8.5.2

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

[Duy Nguyen]

On Fri, Dec 20, 2013 at 8:06 AM, René Scharfe <l.s.r@web.de> wrote:
> Am 20.12.2013 00:50, schrieb Duy Nguyen:
>> On Fri, Dec 20, 2013 at 6:32 AM, René Scharfe <l.s.r@web.de> wrote:
>>> 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?
>>
>> That was skip_prefix_gently() that I forgot to replace in a commit
>> message, before I turned it into _defval variant. The reason for
>> _defval is it could be use to chain expression together without adding
>> temporary variables, e.g.
>>
>> -       if (starts_with(line->buf, ">From") && isspace(line->buf[5])) {
>> +       if (isspace(*skip_prefix_defval(line->buf, ">From", "NOSPACE"))) {
>>
>> Without _defval, one would need to do if ((p = skip_prefix(..)) &&
>> isspace(*p)). I'm not entirely sure this is a good thing though as it
>> could make it a bit harder to read.
>
> That usage is quite rare compared to occurrences of
> skip_prefix_defval(foo, prefix, foo), no?  Adding a temporary variable
> for them wouldn't be that bad if we can simplify the API to a single
> function -- if that one is usable, that is.
>
> On the other hand, we could add a special function for that example
> and we'd already have three users in the tree (patch below).  I think
> that's too narrow a use case for a library function, though.  Doing
> the following instead in the three cases doesn't seem to be too bad:
>
>         rest = skip_prefix(line->buf, ">From");
>         if (rest != line->buf && isspace(*rest)) {
>

OK I agree it's the minority and probably not even worth a wrapper.
But I disagree on changing the behavior of skip_prefix(). Some
in-flight topics might depend on "old" skip_prefix(). The
"skip_prefix() returns full string if not found" should have a
different name. I'll go with _gently again, unless you suggest another
name.

> ---
>  builtin/apply.c    | 2 +-
>  builtin/mailinfo.c | 4 ++--
>  git-compat-util.h  | 1 +
>  strbuf.c           | 9 +++++++++
>  4 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/apply.c b/builtin/apply.c
> index b0d0986..b96befd 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -433,7 +433,7 @@ static unsigned long linelen(const char *buffer, unsigned long size)
>
>  static int is_dev_null(const char *str)
>  {
> -       return !memcmp("/dev/null", str, 9) && isspace(str[9]);
> +       return skip_prefix_and_space(str, "/dev/null") != str;
>  }
>
>  #define TERM_SPACE     1
> diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c
> index 2c3cd8e..2575989 100644
> --- a/builtin/mailinfo.c
> +++ b/builtin/mailinfo.c
> @@ -328,11 +328,11 @@ static int check_header(const struct strbuf *line,
>         }
>
>         /* for inbody stuff */
> -       if (starts_with(line->buf, ">From") && isspace(line->buf[5])) {
> +       if (skip_prefix_and_space(line->buf, ">From") != line->buf) {
>                 ret = 1; /* Should this return 0? */
>                 goto check_header_out;
>         }
> -       if (starts_with(line->buf, "[PATCH]") && isspace(line->buf[7])) {
> +       if (skip_prefix_and_space(line->buf, "[PATCH]") != line->buf) {
>                 for (i = 0; header[i]; i++) {
>                         if (!memcmp("Subject", header[i], 7)) {
>                                 handle_header(&hdr_data[i], line);
> diff --git a/git-compat-util.h b/git-compat-util.h
> index dcb92c4..a083918 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -355,6 +355,7 @@ 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);
>  extern const char *skip_prefix(const char *str, const char *prefix);
> +extern const char *skip_prefix_and_space(const char *str, const char *prefix);
>
>  #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)
>
> diff --git a/strbuf.c b/strbuf.c
> index 222df13..768331f 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -47,6 +47,15 @@ const char *skip_prefix(const char *str, const char *prefix)
>                         return str;
>  }
>
> +const char *skip_prefix_and_space(const char *str, const char *prefix)
> +{
> +       const char *p = skip_prefix(str, prefix);
> +       if (((p != str) || !*prefix) && isspace(*p))
> +               return p + 1;
> +       else
> +               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
> --
> 1.8.5.2



-- 
Duy

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

[Johannes Sixt]

Am 12/18/2013 15:53, schrieb Nguyễn Thái Ngọc Duy:
> The code that's not converted to use parse_options() often does
> 
>   if (!starts_with(arg, "foo=")) {
>      value = atoi(arg + 4);
>   }
> 
> This patch removes those magic numbers with skip_prefix()
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  builtin/fetch-pack.c     | 13 +++++----
>  builtin/index-pack.c     | 17 +++++------
>  builtin/ls-remote.c      |  9 +++---
>  builtin/mailinfo.c       |  5 ++--
>  builtin/reflog.c         |  9 +++---
>  builtin/rev-parse.c      | 41 +++++++++++++-------------
>  builtin/send-pack.c      | 18 ++++++------
>  builtin/unpack-objects.c |  5 ++--
>  builtin/update-ref.c     | 21 +++++++-------
>  daemon.c                 | 75 ++++++++++++++++++++++++------------------------
>  diff.c                   | 49 +++++++++++++++----------------
>  git.c                    | 13 +++++----
>  merge-recursive.c        | 13 +++++----
>  revision.c               | 60 +++++++++++++++++++-------------------
>  upload-pack.c            |  5 ++--
>  15 files changed, 182 insertions(+), 171 deletions(-)
> 
> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> index 8b8978a2..2df1423 100644
> --- a/builtin/fetch-pack.c
> +++ b/builtin/fetch-pack.c
> @@ -47,13 +47,14 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
>  
>  	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.

-- Hannes

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

[Jeff King]

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);

-Peff

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

[Christian Couder]

On Fri, Dec 20, 2013 at 8:04 AM, Jeff King <peff@peff.net> wrote:
> 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.

I agree too.

> 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);

Yeah, I agree that the function signature you suggest is better, but I
like the "skip_prefix" name better.
Or perhaps "remove_prefix"?

Thanks,
Christian.

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

[René Scharfe]

Am 20.12.2013 08:04, schrieb Jeff King:
> 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

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

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Am 20.12.2013 00:50, schrieb Duy Nguyen:
>> On Fri, Dec 20, 2013 at 6:32 AM, René Scharfe <l.s.r@web.de> wrote:
>>> 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?
>> 
>> That was skip_prefix_gently() that I forgot to replace in a commit
>> message, before I turned it into _defval variant. The reason for
>> _defval is it could be use to chain expression together without adding
>> temporary variables, e.g.
>> 
>> -       if (starts_with(line->buf, ">From") && isspace(line->buf[5])) {
>> +       if (isspace(*skip_prefix_defval(line->buf, ">From", "NOSPACE"))) {
>> 
>> Without _defval, one would need to do if ((p = skip_prefix(..)) &&
>> isspace(*p)). I'm not entirely sure this is a good thing though as it
>> could make it a bit harder to read.
>
> That usage is quite rare compared to occurrences of
> skip_prefix_defval(foo, prefix, foo), no?  Adding a temporary variable
> for them wouldn't be that bad if we can simplify the API to a single
> function -- if that one is usable, that is.
>
> On the other hand, we could add a special function for that example
> and we'd already have three users in the tree (patch below).  I think
> that's too narrow a use case for a library function, though.  Doing
> the following instead in the three cases doesn't seem to be too bad:
>
> 	rest = skip_prefix(line->buf, ">From");
> 	if (rest != line->buf && isspace(*rest)) {

Yeah, I personally feel that the "NOSPACE" hack is a bit too ugly to
live in a code meant to be maintained for a longer term.  The above
with a "rest" variable, whose assignment is outside if () condition,
is so far the easiest to read, at least to me.

I am not convinced if skip-prefix-and-space is even a good
abstraction of anything; it feels a bit too specialized.

Thanks.

> ---
>  builtin/apply.c    | 2 +-
>  builtin/mailinfo.c | 4 ++--
>  git-compat-util.h  | 1 +
>  strbuf.c           | 9 +++++++++
>  4 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/apply.c b/builtin/apply.c
> index b0d0986..b96befd 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -433,7 +433,7 @@ static unsigned long linelen(const char *buffer, unsigned long size)
>  
>  static int is_dev_null(const char *str)
>  {
> -	return !memcmp("/dev/null", str, 9) && isspace(str[9]);
> +	return skip_prefix_and_space(str, "/dev/null") != str;
>  }
>  
>  #define TERM_SPACE	1
> diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c
> index 2c3cd8e..2575989 100644
> --- a/builtin/mailinfo.c
> +++ b/builtin/mailinfo.c
> @@ -328,11 +328,11 @@ static int check_header(const struct strbuf *line,
>  	}
>  
>  	/* for inbody stuff */
> -	if (starts_with(line->buf, ">From") && isspace(line->buf[5])) {
> +	if (skip_prefix_and_space(line->buf, ">From") != line->buf) {
>  		ret = 1; /* Should this return 0? */
>  		goto check_header_out;
>  	}
> -	if (starts_with(line->buf, "[PATCH]") && isspace(line->buf[7])) {
> +	if (skip_prefix_and_space(line->buf, "[PATCH]") != line->buf) {
>  		for (i = 0; header[i]; i++) {
>  			if (!memcmp("Subject", header[i], 7)) {
>  				handle_header(&hdr_data[i], line);
> diff --git a/git-compat-util.h b/git-compat-util.h
> index dcb92c4..a083918 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -355,6 +355,7 @@ 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);
>  extern const char *skip_prefix(const char *str, const char *prefix);
> +extern const char *skip_prefix_and_space(const char *str, const char *prefix);
>  
>  #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)
>  
> diff --git a/strbuf.c b/strbuf.c
> index 222df13..768331f 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -47,6 +47,15 @@ const char *skip_prefix(const char *str, const char *prefix)
>  			return str;
>  }
>  
> +const char *skip_prefix_and_space(const char *str, const char *prefix)
> +{
> +	const char *p = skip_prefix(str, prefix);
> +	if (((p != str) || !*prefix) && isspace(*p))
> +		return p + 1;
> +	else
> +		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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>   /* 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);

Sounds fine.  I recall earlier somebody wanting to have a good name
for this thing, and I think foo_gently is *not* it (the name is
about adding a variant that does not die outright to foo that checks
and dies if condition is not right).  

	starts_with(foo, prefix);
        strip_prefix(foo, prefix, &foo);

perhaps?

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

[Duy Nguyen]

On Sat, Dec 21, 2013 at 4:31 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>>   /* 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);
>
> Sounds fine.  I recall earlier somebody wanting to have a good name
> for this thing, and I think foo_gently is *not* it (the name is
> about adding a variant that does not die outright to foo that checks
> and dies if condition is not right).
>
>         starts_with(foo, prefix);
>         strip_prefix(foo, prefix, &foo);
>
> perhaps?

I still need consensus on the name here guys, parse_prefix.
remove_prefix or strip_prefix? If no other opinions i'll go with
strip_prefix (Jeff's comment before parse_prefix() also uses "strip")
-- 
Duy

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

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Sat, Dec 21, 2013 at 4:31 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Jeff King <peff@peff.net> writes:
>>
>>>   /* 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);
>>
>> Sounds fine.  I recall earlier somebody wanting to have a good name
>> for this thing, and I think foo_gently is *not* it (the name is
>> about adding a variant that does not die outright to foo that checks
>> and dies if condition is not right).
>>
>>         starts_with(foo, prefix);
>>         strip_prefix(foo, prefix, &foo);
>>
>> perhaps?
>
> I still need consensus on the name here guys, parse_prefix.
> remove_prefix or strip_prefix? If no other opinions i'll go with
> strip_prefix (Jeff's comment before parse_prefix() also uses "strip")

Yup, that comment is where I took "strip" from.  When you name your
thing as "X", using too generic a word "X", and then need to explain
what "X" does using a bit more specific word "Y", you are often
better off naming it after "Y".

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

[Jeff King]

On Thu, Dec 26, 2013 at 11:27:10AM -0800, Junio C Hamano wrote:

> > I still need consensus on the name here guys, parse_prefix.
> > remove_prefix or strip_prefix? If no other opinions i'll go with
> > strip_prefix (Jeff's comment before parse_prefix() also uses "strip")
> 
> Yup, that comment is where I took "strip" from.  When you name your
> thing as "X", using too generic a word "X", and then need to explain
> what "X" does using a bit more specific word "Y", you are often
> better off naming it after "Y".

FWIW, the reason I shied away from "strip" is that I did not want to
imply that the function mutates the string. But since nobody else seems
concerned with that, I think "strip" is fine.

-Peff