[PATCH 0/16] skip_prefix refactoring and cleanups

[PATCH 0/16] skip_prefix refactoring and cleanups

[Jeff King]

A while ago[1] we discussed refactoring skip_prefix (or adding something
like it) to make it more natural to call. This morning I decided to take
a look at doing this, and went down a rabbit hole of cleanups. This is
part one of the result.

The short of it is that skip_prefix can now be used like this:

  if (skip_prefix(arg, "--foo=", &value)
	handle_foo(value);

or even:

  /* arg remains valid if we didn't match! */
  if (skip_prefix(arg, "--foo=", &arg))
	handle_foo(arg);
  else if (skip_prefix(arg, "--bar", &arg))
	handle_bar(arg);

though the latter form is not always useful if the conditional code
wants to see all of "arg".

  [01/16]: parse_diff_color_slot: drop ofs parameter
  [02/16]: daemon: mark some strings as const
  [03/16]: avoid using skip_prefix as a boolean

    These ones are preparatory cleanup.

  [04/16]: refactor skip_prefix to return a boolean

    The actual refactoring; it changes the existing callers[2] at the
    same time.

  [05/16]: apply: use skip_prefix instead of raw addition
  [06/16]: fast-import: fix read of uninitialized argv memory
  [07/16]: transport-helper: avoid reading past end-of-string

    These three are conversions that actually fix bugs.

  [08/16]: use skip_prefix to avoid magic numbers
  [09/16]: use skip_prefix to avoid repeating strings

    These are the straightforward conversions lumped together by the
    problem they are solving.

  [10/16]: fast-import: use skip_prefix for parsing input
  [11/16]: daemon: use skip_prefix to avoid magic numbers
  [12/16]: stat_opt: check extra strlen call
  [13/16]: fast-import: refactor parsing of spaces
  [14/16]: fetch-pack: refactor parsing in get_ack
  [15/16]: git: avoid magic number with skip_prefix

    These ones are variants of the above two that needed extra
    discussion or attention for various reasons.

  [16/16]: use skip_prefix to avoid repeated calculations

    These ones don't solve a specific problem as the patches above do,
    but I think the code ends up more readable.

My conversions are by now means exhaustive. After grepping through all
of the starts_with up to about http.c, I decided to call it a day. But
we can easily convert more as time goes on.

[1] http://thread.gmane.org/gmane.comp.version-control.git/239438/focus=239564

    I started from scratch this morning, oblivious to the fact that René
    posted something very similar to patch 4 in that thread.

[2] I test-merged with 'pu'. There is a minor textual conflict with
    jk/commit-buffer-length that should be easy to resolve. There's also
    one new caller of skip_prefix added in cc/interpret-trailers. It
    needs this fix when merged:

diff --git a/trailer.c b/trailer.c
index eaf692b..987fa29 100644
--- a/trailer.c
+++ b/trailer.c
@@ -377,8 +377,7 @@ static int git_trailer_config(const char *conf_key, const char *value, void *cb)
 	enum trailer_info_type type;
 	int i;
 
-	trailer_item = skip_prefix(conf_key, "trailer.");
-	if (!trailer_item)
+	if (!skip_prefix(conf_key, "trailer.", &trailer_item))
 		return 0;
 
 	variable_name = strrchr(trailer_item, '.');

[PATCH 01/16] parse_diff_color_slot: drop ofs parameter

[Jeff King]

This function originally took a whole config variable name
("var") and an offset ("ofs"). It checked "var+ofs" against
each color slot, but reported errors using the whole "var".

However, since 8b8e862 (ignore unknown color configuration,
2009-12-12), it returns -1 rather than printing its own
error, and therefore only cares about var+ofs. We can drop
the ofs parameter and teach its sole caller to derive the
pointer itself.

Signed-off-by: Jeff King <peff@peff.net>
---
 diff.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/diff.c b/diff.c
index bba9a55..77c5eb4 100644
--- a/diff.c
+++ b/diff.c
@@ -52,23 +52,23 @@ static char diff_colors[][COLOR_MAXLEN] = {
 	GIT_COLOR_NORMAL,	/* FUNCINFO */
 };
 
-static int parse_diff_color_slot(const char *var, int ofs)
+static int parse_diff_color_slot(const char *var)
 {
-	if (!strcasecmp(var+ofs, "plain"))
+	if (!strcasecmp(var, "plain"))
 		return DIFF_PLAIN;
-	if (!strcasecmp(var+ofs, "meta"))
+	if (!strcasecmp(var, "meta"))
 		return DIFF_METAINFO;
-	if (!strcasecmp(var+ofs, "frag"))
+	if (!strcasecmp(var, "frag"))
 		return DIFF_FRAGINFO;
-	if (!strcasecmp(var+ofs, "old"))
+	if (!strcasecmp(var, "old"))
 		return DIFF_FILE_OLD;
-	if (!strcasecmp(var+ofs, "new"))
+	if (!strcasecmp(var, "new"))
 		return DIFF_FILE_NEW;
-	if (!strcasecmp(var+ofs, "commit"))
+	if (!strcasecmp(var, "commit"))
 		return DIFF_COMMIT;
-	if (!strcasecmp(var+ofs, "whitespace"))
+	if (!strcasecmp(var, "whitespace"))
 		return DIFF_WHITESPACE;
-	if (!strcasecmp(var+ofs, "func"))
+	if (!strcasecmp(var, "func"))
 		return DIFF_FUNCINFO;
 	return -1;
 }
@@ -240,7 +240,7 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
 		return -1;
 
 	if (starts_with(var, "diff.color.") || starts_with(var, "color.diff.")) {
-		int slot = parse_diff_color_slot(var, 11);
+		int slot = parse_diff_color_slot(var + 11);
 		if (slot < 0)
 			return 0;
 		if (!value)
-- 
2.0.0.566.gfe3e6b2

[PATCH 02/16] daemon: mark some strings as const

[Jeff King]

None of these strings is modified; marking them as const
will help later refactoring.

Signed-off-by: Jeff King <peff@peff.net>
---
 daemon.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/daemon.c b/daemon.c
index f9c63e9..18818c3 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. */
@@ -106,12 +106,12 @@ static void NORETURN daemon_die(const char *err, va_list params)
 	exit(1);
 }
 
-static const char *path_ok(char *directory)
+static const char *path_ok(const char *directory)
 {
 	static char rpath[PATH_MAX];
 	static char interp_path[PATH_MAX];
 	const char *path;
-	char *dir;
+	const char *dir;
 
 	dir = directory;
 
@@ -131,7 +131,7 @@ static const char *path_ok(char *directory)
 			 * "~alice/%s/foo".
 			 */
 			int namlen, restlen = strlen(dir);
-			char *slash = strchr(dir, '/');
+			const char *slash = strchr(dir, '/');
 			if (!slash)
 				slash = dir + restlen;
 			namlen = slash - dir;
@@ -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)
 {
@@ -318,7 +318,7 @@ error_return:
 	return -1;
 }
 
-static int run_service(char *dir, struct daemon_service *service)
+static int run_service(const char *dir, struct daemon_service *service)
 {
 	const char *path;
 	int enabled = service->enabled;
-- 
2.0.0.566.gfe3e6b2

[PATCH 03/16] avoid using skip_prefix as a boolean

[Jeff King]

There's no point in using:

  if (skip_prefix(buf, "foo"))

over

  if (starts_with(buf, "foo"))

as the point of skip_prefix is to return a pointer to the
data after the prefix. Using starts_with is more readable,
and will make refactoring skip_prefix easier.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/fmt-merge-msg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 3906eda..72378e6 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -298,7 +298,7 @@ static void credit_people(struct strbuf *out,
 	    (them->nr == 1 &&
 	     me &&
 	     (me = skip_prefix(me, them->items->string)) != NULL &&
-	     skip_prefix(me, " <")))
+	     starts_with(me, " <")))
 		return;
 	strbuf_addf(out, "\n%c %s ", comment_line_char, label);
 	add_people_count(out, them);
-- 
2.0.0.566.gfe3e6b2

[PATCH 04/16] refactor skip_prefix to return a boolean

[Jeff King]

The skip_prefix function returns a pointer to the content
past the prefix, or NULL if the prefix was not found. While
this is nice and simple, in practice it makes it hard to use
for two reasons:

  1. When you want to conditionally skip or keep the string
     as-is, you have to introduce a second temporary
     variable. For example:

       tmp = skip_prefix(buf, "foo");
       if (tmp)
	       buf = tmp;

  2. It is verbose to check the outcome in a conditional, as
     you need extra parentheses to silence compiler
     warnings. For example:

       if ((cp = skip_prefix(buf, "foo"))
	       /* do something with cp */

Both of these make it harder to use for long if-chains, and
we tend to use starts_with instead. However, the first line
of "do something" is often to then skip forward in buf past
the prefix, either using a magic constant or with an extra
strlen (which is generally computed at compile time, but
means we are repeating ourselves).

This patch refactors skip_prefix to return a simple boolean,
and to provide the pointer value as an out-parameter. If the
prefix is not found, the out-parameter is untouched. This
lets you write:

  if (skip_prefix(arg, "foo ", &arg))
	  do_foo(arg);
  else if (skip_prefix(arg, "bar ", &arg))
	  do_bar(arg);

Signed-off-by: Jeff King <peff@peff.net>
---
The diffstat is misleading. We actually save lines, except that I added
documentation for the function. :)

 advice.c                   |  5 ++++-
 branch.c                   |  4 ++--
 builtin/branch.c           |  6 +++---
 builtin/clone.c            | 11 +++++++----
 builtin/commit.c           |  5 ++---
 builtin/fmt-merge-msg.c    |  2 +-
 builtin/push.c             |  7 +++----
 builtin/remote.c           |  4 +---
 column.c                   |  5 +++--
 commit.c                   |  6 ++----
 config.c                   |  3 +--
 credential-cache--daemon.c |  6 ++----
 credential.c               |  3 +--
 fsck.c                     | 14 +++++---------
 git-compat-util.h          | 26 ++++++++++++++++++++++----
 parse-options.c            | 16 +++++++++-------
 transport.c                |  4 +++-
 urlmatch.c                 |  3 +--
 18 files changed, 72 insertions(+), 58 deletions(-)

diff --git a/advice.c b/advice.c
index c50ebdf..9b42033 100644
--- a/advice.c
+++ b/advice.c
@@ -61,9 +61,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 (!skip_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/branch.c b/branch.c
index 660097b..46e8aa8 100644
--- a/branch.c
+++ b/branch.c
@@ -50,11 +50,11 @@ static int should_setup_rebase(const char *origin)
 
 void install_branch_config(int flag, const char *local, const char *origin, const char *remote)
 {
-	const char *shortname = skip_prefix(remote, "refs/heads/");
+	const char *shortname = NULL;
 	struct strbuf key = STRBUF_INIT;
 	int rebasing = should_setup_rebase(origin);
 
-	if (shortname
+	if (skip_prefix(remote, "refs/heads/", &shortname)
 	    && !strcmp(local, shortname)
 	    && !origin) {
 		warning(_("Not setting branch %s as its own upstream."),
diff --git a/builtin/branch.c b/builtin/branch.c
index 652b1d2..0591b22 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)
+		skip_prefix(dst, prefix, &dst);
 	return xstrdup(dst);
 }
 
diff --git a/builtin/clone.c b/builtin/clone.c
index b12989d..a5b2d9d 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -584,11 +584,11 @@ 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 && skip_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,
 				   UPDATE_REFS_DIE_ON_ERR);
 			install_branch_config(0, head, option_origin, our->name);
@@ -703,9 +703,12 @@ 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) {
+				const char *head = remote_head_points_at->name;
+				if (!skip_prefix(head, "refs/heads/", &head))
+					die("BUG: remote HEAD points at non-head?");
+
 				strbuf_addf(&value, "+%s:%s%s", remote_head_points_at->name,
-						branch_top->buf,
-						skip_prefix(remote_head_points_at->name, "refs/heads/"));
+						branch_top->buf, head);
 			}
 			/*
 			 * otherwise, the next "git fetch" will
diff --git a/builtin/commit.c b/builtin/commit.c
index 5e2221c..ec75341 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1020,7 +1020,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;
@@ -1029,8 +1029,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)
+	if (!skip_prefix(sb->buf, tmpl.buf, &start))
 		start = sb->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 72378e6..3c19241 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -297,7 +297,7 @@ 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, them->items->string, &me) &&
 	     starts_with(me, " <")))
 		return;
 	strbuf_addf(out, "\n%c %s ", comment_line_char, label);
diff --git a/builtin/push.c b/builtin/push.c
index f8dfea4..f50e3d5 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -127,11 +127,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;
+
+	skip_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 c9102e8..a8efe3d 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;
+	skip_prefix(name, prefix, &name);
 	return name;
 }
 #define abbrev_branch(name) abbrev_ref((name), "refs/heads/")
diff --git a/column.c b/column.c
index 1a468de..ca878bc 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 (!skip_prefix(var, "column.", &it))
 		return 0;
 
 	if (!strcmp(it, "ui"))
diff --git a/commit.c b/commit.c
index 881be3b..dfc0eb0 100644
--- a/commit.c
+++ b/commit.c
@@ -556,8 +556,7 @@ static void record_author_date(struct author_date_slab *author_date,
 	     buf;
 	     buf = line_end + 1) {
 		line_end = strchrnul(buf, '\n');
-		ident_line = skip_prefix(buf, "author ");
-		if (!ident_line) {
+		if (!skip_prefix(buf, "author ", &ident_line)) {
 			if (!line_end[0] || line_end[1] == '\n')
 				return; /* end of header */
 			continue;
@@ -1183,8 +1182,7 @@ static void parse_gpg_output(struct signature_check *sigc)
 	for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {
 		const char *found, *next;
 
-		found = skip_prefix(buf, sigcheck_gpg_status[i].check + 1);
-		if (!found) {
+		if (!skip_prefix(buf, sigcheck_gpg_status[i].check + 1, &found)) {
 			found = strstr(buf, sigcheck_gpg_status[i].check);
 			if (!found)
 				continue;
diff --git a/config.c b/config.c
index a1aef1c..ba882a1 100644
--- a/config.c
+++ b/config.c
@@ -138,8 +138,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 (!skip_prefix(var, "include.", &type))
 		return ret;
 
 	if (!strcmp(type, "path"))
diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c
index 390f194..3b370ca 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 (!skip_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 (!skip_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..4d79d32 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 (!skip_prefix(var, "credential.", &key))
 		return 0;
 
 	if (!value)
diff --git a/fsck.c b/fsck.c
index abed62b..bdbea2b 100644
--- a/fsck.c
+++ b/fsck.c
@@ -278,20 +278,18 @@ static int fsck_ident(const char **ident, struct object *obj, fsck_error error_f
 
 static int fsck_commit(struct commit *commit, fsck_error error_func)
 {
-	const char *buffer = commit->buffer, *tmp;
+	const char *buffer = commit->buffer;
 	unsigned char tree_sha1[20], sha1[20];
 	struct commit_graft *graft;
 	int parents = 0;
 	int err;
 
-	buffer = skip_prefix(buffer, "tree ");
-	if (!buffer)
+	if (!skip_prefix(buffer, "tree ", &buffer))
 		return error_func(&commit->object, FSCK_ERROR, "invalid format - expected 'tree' line");
 	if (get_sha1_hex(buffer, tree_sha1) || buffer[40] != '\n')
 		return error_func(&commit->object, FSCK_ERROR, "invalid 'tree' line format - bad sha1");
 	buffer += 41;
-	while ((tmp = skip_prefix(buffer, "parent "))) {
-		buffer = tmp;
+	while (skip_prefix(buffer, "parent ", &buffer)) {
 		if (get_sha1_hex(buffer, sha1) || buffer[40] != '\n')
 			return error_func(&commit->object, FSCK_ERROR, "invalid 'parent' line format - bad sha1");
 		buffer += 41;
@@ -318,14 +316,12 @@ static int fsck_commit(struct commit *commit, fsck_error error_func)
 		if (p || parents)
 			return error_func(&commit->object, FSCK_ERROR, "parent objects missing");
 	}
-	buffer = skip_prefix(buffer, "author ");
-	if (!buffer)
+	if (!skip_prefix(buffer, "author ", &buffer))
 		return error_func(&commit->object, FSCK_ERROR, "invalid format - expected 'author' line");
 	err = fsck_ident(&buffer, &commit->object, error_func);
 	if (err)
 		return err;
-	buffer = skip_prefix(buffer, "committer ");
-	if (!buffer)
+	if (!skip_prefix(buffer, "committer ", &buffer))
 		return error_func(&commit->object, FSCK_ERROR, "invalid format - expected 'committer' line");
 	err = fsck_ident(&buffer, &commit->object, error_func);
 	if (err)
diff --git a/git-compat-util.h b/git-compat-util.h
index b6f03b3..556c839 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -349,13 +349,31 @@ extern void set_die_is_recursing_routine(int (*routine)(void));
 extern int starts_with(const char *str, const char *prefix);
 extern int ends_with(const char *str, const char *suffix);
 
-static inline const char *skip_prefix(const char *str, const char *prefix)
+/*
+ * If "str" begins with "prefix", return 1. If out is non-NULL,
+ * it it set to str + strlen(prefix) (i.e., the prefix is skipped).
+ *
+ * Otherwise, returns 0 and out is left untouched.
+ *
+ * Examples:
+ *
+ *   [extract branch name, fail if not a branch]
+ *   if (!skip_prefix(ref, "refs/heads/", &branch)
+ *	return -1;
+ *
+ *   [skip prefix if present, otherwise use whole string]
+ *   skip_prefix(name, "refs/heads/", &name);
+ */
+static inline int skip_prefix(const char *str, const char *prefix,
+			      const char **out)
 {
 	do {
-		if (!*prefix)
-			return str;
+		if (!*prefix) {
+			*out = str;
+			return 1;
+		}
 	} while (*str++ == *prefix++);
-	return NULL;
+	return 0;
 }
 
 #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)
diff --git a/parse-options.c b/parse-options.c
index b536896..e7dafa8 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -231,7 +231,8 @@ static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg,
 			continue;
 
 again:
-		rest = skip_prefix(arg, long_name);
+		if (!skip_prefix(arg, long_name, &rest))
+			rest = NULL;
 		if (options->type == OPTION_ARGUMENT) {
 			if (!rest)
 				continue;
@@ -280,12 +281,13 @@ 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)
-				continue;
+			if (!skip_prefix(arg + 3, long_name, &rest)) {
+				/* abbreviated and negated? */
+				if (starts_with(long_name, arg + 3))
+					goto is_abbreviated;
+				else
+					continue;
+			}
 		}
 		if (*rest) {
 			if (*rest != '=')
diff --git a/transport.c b/transport.c
index 325f03e..59c9727 100644
--- a/transport.c
+++ b/transport.c
@@ -192,7 +192,9 @@ 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://"))
+		skip_prefix(url, "rsync:", &url);
+	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..3d4c54b 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 (!skip_prefix(var, collect->section, &key) || *(key++) != '.') {
 		if (collect->cascade_fn)
 			return collect->cascade_fn(var, value, cb);
 		return 0; /* not interested */
-- 
2.0.0.566.gfe3e6b2

[PATCH 05/16] apply: use skip_prefix instead of raw addition

[Jeff King]

A submodule diff generally has content like:

  -Subproject commit [0-9a-f]{40}
  +Subproject commit [0-9a-f]{40}

When we are using "git apply --index" with a submodule, we
first apply the textual diff, and then parse that result to
figure out the new sha1.

If the diff has bogus input like:

  -Subproject commit 1234567890123456789012345678901234567890
  +bogus

we will parse the "bogus" portion. Our parser assumes that
the buffer starts with "Subproject commit", and blindly
skips past it using strlen(). This can cause us to read
random memory after the buffer.

This problem was unlikely to have come up in practice (since
it requires a malformed diff), and even when it did, we
likely noticed the problem anyway as the next operation was
to call get_sha1_hex on the random memory.

However, we can easily fix it by using skip_prefix to notice
the parsing error.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/apply.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 9c5724e..bc924ab 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3847,9 +3847,10 @@ static void add_index_file(const char *path, unsigned mode, void *buf, unsigned
 	ce->ce_flags = create_ce_flags(0);
 	ce->ce_namelen = namelen;
 	if (S_ISGITLINK(mode)) {
-		const char *s = buf;
+		const char *s;
 
-		if (get_sha1_hex(s + strlen("Subproject commit "), ce->sha1))
+		if (!skip_prefix(buf, "Subproject commit ", &s) ||
+		    get_sha1_hex(s, ce->sha1))
 			die(_("corrupt patch for submodule %s"), path);
 	} else {
 		if (!cached) {
-- 
2.0.0.566.gfe3e6b2

[PATCH 06/16] fast-import: fix read of uninitialized argv memory

[Jeff King]

Fast-import shares code between its command-line parser and
the "option" command. To do so, it strips the "--" from any
command-line options and passes them to the option parser.
However, it does not confirm that the option even begins
with "--" before blindly passing "arg + 2".

It does confirm that the option starts with "-", so the only
affected case was:

  git fast-import -

which would read uninitialized memory after the argument. We
can fix it by using skip_prefix and checking the result. As
a bonus, this gets rid of some magic numbers.

Signed-off-by: Jeff King <peff@peff.net>
---
 fast-import.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 6707a66..b2030cc 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -3342,18 +3342,21 @@ static void parse_argv(void)
 		if (*a != '-' || !strcmp(a, "--"))
 			break;
 
-		if (parse_one_option(a + 2))
+		if (!skip_prefix(a, "--", &a))
+			die("unknown option %s", a);
+
+		if (parse_one_option(a))
 			continue;
 
-		if (parse_one_feature(a + 2, 0))
+		if (parse_one_feature(a, 0))
 			continue;
 
-		if (starts_with(a + 2, "cat-blob-fd=")) {
-			option_cat_blob_fd(a + 2 + strlen("cat-blob-fd="));
+		if (skip_prefix(a, "cat-blob-fd=", &a)) {
+			option_cat_blob_fd(a);
 			continue;
 		}
 
-		die("unknown option %s", a);
+		die("unknown option --%s", a);
 	}
 	if (i != global_argc)
 		usage(fast_import_usage);
-- 
2.0.0.566.gfe3e6b2

[PATCH 07/16] transport-helper: avoid reading past end-of-string

[Jeff King]

We detect the "import-marks" capability by looking for that
string, but _without_ a trailing space. Then we skip past it
using strlen("import-marks "), with a space. So if a remote
helper gives us exactly "import-marks", we will read past
the end-of-string by one character.

This is unlikely to be a problem in practice, because such
input is malformed in the first place, and because there is
a good chance that the string has an extra NUL terminator
one character after the original (because it formerly had a
newline in it that we parsed off).

We can fix it by using skip_prefix with "import-marks ",
with the space. The other form appears to be a typo from
a515ebe (transport-helper: implement marks location as
capability, 2011-07-16); "import-marks" has never existed
without an argument, and it should match the "export-marks"
definition above.

Speaking of which, we can also use skip_prefix in a few
other places while we are in the function.

Signed-off-by: Jeff King <peff@peff.net>
---
 transport-helper.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 84c616f..3d8fe7d 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -153,7 +153,7 @@ static struct child_process *get_helper(struct transport *transport)
 	write_constant(helper->in, "capabilities\n");
 
 	while (1) {
-		const char *capname;
+		const char *capname, *arg;
 		int mandatory = 0;
 		if (recvline(data, &buf))
 			exit(128);
@@ -183,19 +183,19 @@ static struct child_process *get_helper(struct transport *transport)
 			data->export = 1;
 		else if (!strcmp(capname, "check-connectivity"))
 			data->check_connectivity = 1;
-		else if (!data->refspecs && starts_with(capname, "refspec ")) {
+		else if (!data->refspecs && skip_prefix(capname, "refspec ", &arg)) {
 			ALLOC_GROW(refspecs,
 				   refspec_nr + 1,
 				   refspec_alloc);
-			refspecs[refspec_nr++] = xstrdup(capname + strlen("refspec "));
+			refspecs[refspec_nr++] = xstrdup(arg);
 		} else if (!strcmp(capname, "connect")) {
 			data->connect = 1;
 		} else if (!strcmp(capname, "signed-tags")) {
 			data->signed_tags = 1;
-		} else if (starts_with(capname, "export-marks ")) {
-			data->export_marks = xstrdup(capname + strlen("export-marks "));
-		} else if (starts_with(capname, "import-marks")) {
-			data->import_marks = xstrdup(capname + strlen("import-marks "));
+		} else if (skip_prefix(capname, "export-marks ", &arg)) {
+			data->export_marks = xstrdup(arg);
+		} else if (skip_prefix(capname, "import-marks ", &arg)) {
+			data->import_marks = xstrdup(arg);
 		} else if (starts_with(capname, "no-private-update")) {
 			data->no_private_update = 1;
 		} else if (mandatory) {
-- 
2.0.0.566.gfe3e6b2

[PATCH 08/16] use skip_prefix to avoid magic numbers

[Jeff King]

It's a common idiom to match a prefix and then skip past it
with a magic number, like:

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

This is easy to get wrong, since you have to count the
prefix string yourself, and there's no compiler check if the
string changes.  We can use skip_prefix to avoid the magic
numbers here.

Note that some of these conversions could be much shorter.
For example:

  if (starts_with(arg, "--foo=")) {
	  bar = arg + 6;
	  continue;
  }

could become:

  if (skip_prefix(arg, "--foo=", &bar))
	  continue;

However, I have left it as:

  if (skip_prefix(arg, "--foo=", &v)) {
	  bar = v;
	  continue;
  }

to visually match nearby cases which need to actually
process the string. Like:

  if (skip_prefix(arg, "--foo=", &v)) {
	  bar = atoi(v);
	  continue;
  }

Signed-off-by: Jeff King <peff@peff.net>
---
 alias.c        |  3 ++-
 connect.c      | 11 +++++----
 convert.c      |  4 ++--
 daemon.c       | 73 ++++++++++++++++++++++++++++++----------------------------
 diff.c         | 65 ++++++++++++++++++++++++++-------------------------
 fast-import.c  | 69 +++++++++++++++++++++++++++++-------------------------
 fetch-pack.c   |  9 ++++----
 git.c          | 18 +++++++--------
 help.c         |  6 +++--
 http-backend.c | 11 +++++----
 http-push.c    | 11 +++++----
 11 files changed, 149 insertions(+), 131 deletions(-)

diff --git a/alias.c b/alias.c
index 5efc3d6..758c867 100644
--- a/alias.c
+++ b/alias.c
@@ -5,7 +5,8 @@ static char *alias_val;
 
 static int alias_lookup_cb(const char *k, const char *v, void *cb)
 {
-	if (starts_with(k, "alias.") && !strcmp(k + 6, alias_key)) {
+	const char *name;
+	if (skip_prefix(k, "alias.", &name) && !strcmp(name, alias_key)) {
 		if (!v)
 			return config_error_nonbool(k);
 		alias_val = xstrdup(v);
diff --git a/connect.c b/connect.c
index 94a6650..37ff018 100644
--- a/connect.c
+++ b/connect.c
@@ -129,6 +129,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 		char *name;
 		int len, name_len;
 		char *buffer = packet_buffer;
+		const char *arg;
 
 		len = packet_read(in, &src_buf, &src_len,
 				  packet_buffer, sizeof(packet_buffer),
@@ -140,12 +141,12 @@ 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 (len > 4 && skip_prefix(buffer, "ERR ", &arg))
+			die("remote error: %s", arg);
 
-		if (len == 48 && starts_with(buffer, "shallow ")) {
-			if (get_sha1_hex(buffer + 8, old_sha1))
-				die("protocol error: expected shallow sha-1, got '%s'", buffer + 8);
+		if (len == 48 && skip_prefix(buffer, "shallow ", &arg)) {
+			if (get_sha1_hex(arg, old_sha1))
+				die("protocol error: expected shallow sha-1, got '%s'", arg);
 			if (!shallow_points)
 				die("repository on the other end cannot be shallow");
 			sha1_array_append(shallow_points, old_sha1);
diff --git a/convert.c b/convert.c
index ab80b72..cb5fbb4 100644
--- a/convert.c
+++ b/convert.c
@@ -1121,9 +1121,9 @@ static int is_foreign_ident(const char *str)
 {
 	int i;
 
-	if (!starts_with(str, "$Id: "))
+	if (!skip_prefix(str, "$Id: ", &str))
 		return 0;
-	for (i = 5; str[i]; i++) {
+	for (i = 0; str[i]; i++) {
 		if (isspace(str[i]) && str[i+1] != '$')
 			return 1;
 	}
diff --git a/daemon.c b/daemon.c
index 18818c3..6d25828 100644
--- a/daemon.c
+++ b/daemon.c
@@ -235,8 +235,10 @@ static int service_enabled;
 
 static int git_daemon_config(const char *var, const char *value, void *cb)
 {
-	if (starts_with(var, "daemon.") &&
-	    !strcmp(var + 7, service_looking_at->config_name)) {
+	const char *service;
+
+	if (skip_prefix(var, "daemon.", &service) &&
+	    !strcmp(service, service_looking_at->config_name)) {
 		service_enabled = git_config_bool(var, value);
 		return 0;
 	}
@@ -1133,16 +1135,17 @@ int main(int argc, char **argv)
 
 	for (i = 1; i < argc; i++) {
 		char *arg = argv[i];
+		const char *v;
 
-		if (starts_with(arg, "--listen=")) {
-			string_list_append(&listen_addr, xstrdup_tolower(arg + 9));
+		if (skip_prefix(arg, "--listen=", &v)) {
+			string_list_append(&listen_addr, xstrdup_tolower(v));
 			continue;
 		}
-		if (starts_with(arg, "--port=")) {
+		if (skip_prefix(arg, "--port=", &v)) {
 			char *end;
 			unsigned long n;
-			n = strtoul(arg+7, &end, 0);
-			if (arg[7] && !*end) {
+			n = strtoul(v, &end, 0);
+			if (*v && !*end) {
 				listen_port = n;
 				continue;
 			}
@@ -1168,20 +1171,20 @@ int main(int argc, char **argv)
 			export_all_trees = 1;
 			continue;
 		}
-		if (starts_with(arg, "--access-hook=")) {
-			access_hook = arg + 14;
+		if (skip_prefix(arg, "--access-hook=", &v)) {
+			access_hook = v;
 			continue;
 		}
-		if (starts_with(arg, "--timeout=")) {
-			timeout = atoi(arg+10);
+		if (skip_prefix(arg, "--timeout=", &v)) {
+			timeout = atoi(v);
 			continue;
 		}
-		if (starts_with(arg, "--init-timeout=")) {
-			init_timeout = atoi(arg+15);
+		if (skip_prefix(arg, "--init-timeout=", &v)) {
+			init_timeout = atoi(v);
 			continue;
 		}
-		if (starts_with(arg, "--max-connections=")) {
-			max_connections = atoi(arg+18);
+		if (skip_prefix(arg, "--max-connections=", &v)) {
+			max_connections = atoi(v);
 			if (max_connections < 0)
 				max_connections = 0;	        /* unlimited */
 			continue;
@@ -1190,16 +1193,16 @@ int main(int argc, char **argv)
 			strict_paths = 1;
 			continue;
 		}
-		if (starts_with(arg, "--base-path=")) {
-			base_path = arg+12;
+		if (skip_prefix(arg, "--base-path=", &v)) {
+			base_path = v;
 			continue;
 		}
 		if (!strcmp(arg, "--base-path-relaxed")) {
 			base_path_relaxed = 1;
 			continue;
 		}
-		if (starts_with(arg, "--interpolated-path=")) {
-			interpolated_path = arg+20;
+		if (skip_prefix(arg, "--interpolated-path=", &v)) {
+			interpolated_path = v;
 			continue;
 		}
 		if (!strcmp(arg, "--reuseaddr")) {
@@ -1210,12 +1213,12 @@ int main(int argc, char **argv)
 			user_path = "";
 			continue;
 		}
-		if (starts_with(arg, "--user-path=")) {
-			user_path = arg + 12;
+		if (skip_prefix(arg, "--user-path=", &v)) {
+			user_path = v;
 			continue;
 		}
-		if (starts_with(arg, "--pid-file=")) {
-			pid_file = arg + 11;
+		if (skip_prefix(arg, "--pid-file=", &v)) {
+			pid_file = v;
 			continue;
 		}
 		if (!strcmp(arg, "--detach")) {
@@ -1223,28 +1226,28 @@ int main(int argc, char **argv)
 			log_syslog = 1;
 			continue;
 		}
-		if (starts_with(arg, "--user=")) {
-			user_name = arg + 7;
+		if (skip_prefix(arg, "--user=", &v)) {
+			user_name = v;
 			continue;
 		}
-		if (starts_with(arg, "--group=")) {
-			group_name = arg + 8;
+		if (skip_prefix(arg, "--group=", &v)) {
+			group_name = v;
 			continue;
 		}
-		if (starts_with(arg, "--enable=")) {
-			enable_service(arg + 9, 1);
+		if (skip_prefix(arg, "--enable=", &v)) {
+			enable_service(v, 1);
 			continue;
 		}
-		if (starts_with(arg, "--disable=")) {
-			enable_service(arg + 10, 0);
+		if (skip_prefix(arg, "--disable=", &v)) {
+			enable_service(v, 0);
 			continue;
 		}
-		if (starts_with(arg, "--allow-override=")) {
-			make_service_overridable(arg + 17, 1);
+		if (skip_prefix(arg, "--allow-override=", &v)) {
+			make_service_overridable(v, 1);
 			continue;
 		}
-		if (starts_with(arg, "--forbid-override=")) {
-			make_service_overridable(arg + 18, 0);
+		if (skip_prefix(arg, "--forbid-override=", &v)) {
+			make_service_overridable(v, 0);
 			continue;
 		}
 		if (!strcmp(arg, "--informative-errors")) {
diff --git a/diff.c b/diff.c
index 77c5eb4..97db932 100644
--- a/diff.c
+++ b/diff.c
@@ -231,6 +231,8 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
 
 int git_diff_basic_config(const char *var, const char *value, void *cb)
 {
+	const char *name;
+
 	if (!strcmp(var, "diff.renamelimit")) {
 		diff_rename_limit_default = git_config_int(var, value);
 		return 0;
@@ -239,8 +241,9 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
 	if (userdiff_config(var, value) < 0)
 		return -1;
 
-	if (starts_with(var, "diff.color.") || starts_with(var, "color.diff.")) {
-		int slot = parse_diff_color_slot(var + 11);
+	if (skip_prefix(var, "diff.color.", &name) ||
+	    skip_prefix(var, "color.diff.", &name)) {
+		int slot = parse_diff_color_slot(name);
 		if (slot < 0)
 			return 0;
 		if (!value)
@@ -2341,6 +2344,7 @@ static void builtin_diff(const char *name_a,
 	} else {
 		/* Crazy xdl interfaces.. */
 		const char *diffopts = getenv("GIT_DIFF_OPTS");
+		const char *v;
 		xpparam_t xpp;
 		xdemitconf_t xecfg;
 		struct emit_callback ecbdata;
@@ -2379,10 +2383,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 (skip_prefix(diffopts, "--unified=", &v))
+			xecfg.ctxlen = strtoul(v, NULL, 10);
+		else if (skip_prefix(diffopts, "-u", &v))
+			xecfg.ctxlen = strtoul(v, NULL, 10);
 		if (o->word_diff)
 			init_diff_words_data(&ecbdata, o, one, two);
 		xdi_diff_outf(&mf1, &mf2, fn_out_consume, &ecbdata,
@@ -3609,17 +3613,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 (skip_prefix(arg, "-X", &arg))
+		return parse_dirstat_opt(options, arg);
+	else if (skip_prefix(arg, "--dirstat=", &arg))
+		return parse_dirstat_opt(options, arg);
 	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 (skip_prefix(arg, "--dirstat-by-file=", &arg)) {
 		parse_dirstat_opt(options, "files");
-		return parse_dirstat_opt(options, arg + 18);
+		return parse_dirstat_opt(options, arg);
 	}
 	else if (!strcmp(arg, "--check"))
 		options->output_format |= DIFF_FORMAT_CHECKDIFF;
@@ -3669,9 +3673,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 (skip_prefix(arg, "--relative=", &arg)) {
 		DIFF_OPT_SET(options, RELATIVE_NAME);
-		options->prefix = arg + 11;
+		options->prefix = arg;
 	}
 
 	/* xdiff options */
@@ -3722,8 +3726,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 (skip_prefix(arg, "--color=", &arg)) {
+		int value = git_config_colorbool(NULL, arg);
 		if (value < 0)
 			return error("option `color' expects \"always\", \"auto\", or \"never\"");
 		options->use_color = value;
@@ -3734,29 +3738,28 @@ 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 (skip_prefix(arg, "--color-words=", &arg)) {
 		options->use_color = 1;
 		options->word_diff = DIFF_WORDS_COLOR;
-		options->word_regex = arg + 14;
+		options->word_regex = arg;
 	}
 	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;
-		if (!strcmp(type, "plain"))
+	else if (skip_prefix(arg, "--word-diff=", &arg)) {
+		if (!strcmp(arg, "plain"))
 			options->word_diff = DIFF_WORDS_PLAIN;
-		else if (!strcmp(type, "color")) {
+		else if (!strcmp(arg, "color")) {
 			options->use_color = 1;
 			options->word_diff = DIFF_WORDS_COLOR;
 		}
-		else if (!strcmp(type, "porcelain"))
+		else if (!strcmp(arg, "porcelain"))
 			options->word_diff = DIFF_WORDS_PORCELAIN;
-		else if (!strcmp(type, "none"))
+		else if (!strcmp(arg, "none"))
 			options->word_diff = DIFF_WORDS_NONE;
 		else
-			die("bad --word-diff argument: %s", type);
+			die("bad --word-diff argument: %s", arg);
 	}
 	else if ((argcount = parse_long_opt("word-diff-regex", av, &optarg))) {
 		if (options->word_diff == DIFF_WORDS_NONE)
@@ -3779,13 +3782,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 (skip_prefix(arg, "--ignore-submodules=", &arg)) {
 		DIFF_OPT_SET(options, OVERRIDE_SUBMODULE_CONFIG);
-		handle_ignore_submodules_arg(options, arg + 20);
+		handle_ignore_submodules_arg(options, arg);
 	} 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 (skip_prefix(arg, "--submodule=", &arg))
+		return parse_submodule_opt(options, arg);
 
 	/* misc options */
 	else if (!strcmp(arg, "-z"))
@@ -3820,8 +3823,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 (skip_prefix(arg, "--abbrev=", &arg)) {
+		options->abbrev = strtoul(arg, NULL, 10);
 		if (options->abbrev < MINIMUM_ABBREV)
 			options->abbrev = MINIMUM_ABBREV;
 		else if (40 < options->abbrev)
diff --git a/fast-import.c b/fast-import.c
index b2030cc..a3ffe30 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1912,8 +1912,9 @@ static void skip_optional_lf(void)
 
 static void parse_mark(void)
 {
-	if (starts_with(command_buf.buf, "mark :")) {
-		next_mark = strtoumax(command_buf.buf + 6, NULL, 10);
+	const char *v;
+	if (skip_prefix(command_buf.buf, "mark :", &v)) {
+		next_mark = strtoumax(v, NULL, 10);
 		read_next_command();
 	}
 	else
@@ -1922,14 +1923,15 @@ static void parse_mark(void)
 
 static int parse_data(struct strbuf *sb, uintmax_t limit, uintmax_t *len_res)
 {
+	const char *data;
 	strbuf_reset(sb);
 
-	if (!starts_with(command_buf.buf, "data "))
+	if (!skip_prefix(command_buf.buf, "data ", &data))
 		die("Expected 'data n' command, found: %s", command_buf.buf);
 
-	if (starts_with(command_buf.buf + 5, "<<")) {
-		char *term = xstrdup(command_buf.buf + 5 + 2);
-		size_t term_len = command_buf.len - 5 - 2;
+	if (skip_prefix(data, "<<", &data)) {
+		char *term = xstrdup(data);
+		size_t term_len = command_buf.len - (data - command_buf.buf);
 
 		strbuf_detach(&command_buf, NULL);
 		for (;;) {
@@ -1944,7 +1946,7 @@ static int parse_data(struct strbuf *sb, uintmax_t limit, uintmax_t *len_res)
 		free(term);
 	}
 	else {
-		uintmax_t len = strtoumax(command_buf.buf + 5, NULL, 10);
+		uintmax_t len = strtoumax(data, NULL, 10);
 		size_t n = 0, length = (size_t)len;
 
 		if (limit && limit < len) {
@@ -2676,6 +2678,7 @@ static void parse_new_commit(void)
 	struct hash_list *merge_list = NULL;
 	unsigned int merge_count;
 	unsigned char prev_fanout, new_fanout;
+	const char *v;
 
 	/* Obtain the branch name from the rest of our command */
 	sp = strchr(command_buf.buf, ' ') + 1;
@@ -2685,12 +2688,12 @@ static void parse_new_commit(void)
 
 	read_next_command();
 	parse_mark();
-	if (starts_with(command_buf.buf, "author ")) {
-		author = parse_ident(command_buf.buf + 7);
+	if (skip_prefix(command_buf.buf, "author ", &v)) {
+		author = parse_ident(v);
 		read_next_command();
 	}
-	if (starts_with(command_buf.buf, "committer ")) {
-		committer = parse_ident(command_buf.buf + 10);
+	if (skip_prefix(command_buf.buf, "committer ", &v)) {
+		committer = parse_ident(v);
 		read_next_command();
 	}
 	if (!committer)
@@ -2777,6 +2780,7 @@ static void parse_new_tag(void)
 	uintmax_t from_mark = 0;
 	unsigned char sha1[20];
 	enum object_type type;
+	const char *v;
 
 	/* Obtain the new tag name from the rest of our command */
 	sp = strchr(command_buf.buf, ' ') + 1;
@@ -2819,8 +2823,8 @@ static void parse_new_tag(void)
 	read_next_command();
 
 	/* tagger ... */
-	if (starts_with(command_buf.buf, "tagger ")) {
-		tagger = parse_ident(command_buf.buf + 7);
+	if (skip_prefix(command_buf.buf, "tagger ", &v)) {
+		tagger = parse_ident(v);
 		read_next_command();
 	} else
 		tagger = NULL;
@@ -3207,9 +3211,9 @@ static void option_export_pack_edges(const char *edges)
 
 static int parse_one_option(const char *option)
 {
-	if (starts_with(option, "max-pack-size=")) {
+	if (skip_prefix(option, "max-pack-size=", &option)) {
 		unsigned long v;
-		if (!git_parse_ulong(option + 14, &v))
+		if (!git_parse_ulong(option, &v))
 			return 0;
 		if (v < 8192) {
 			warning("max-pack-size is now in bytes, assuming --max-pack-size=%lum", v);
@@ -3219,17 +3223,17 @@ static int parse_one_option(const char *option)
 			v = 1024 * 1024;
 		}
 		max_packsize = v;
-	} else if (starts_with(option, "big-file-threshold=")) {
+	} else if (skip_prefix(option, "big-file-threshold=", &option)) {
 		unsigned long v;
-		if (!git_parse_ulong(option + 19, &v))
+		if (!git_parse_ulong(option, &v))
 			return 0;
 		big_file_threshold = v;
-	} else if (starts_with(option, "depth=")) {
-		option_depth(option + 6);
-	} else if (starts_with(option, "active-branches=")) {
-		option_active_branches(option + 16);
-	} else if (starts_with(option, "export-pack-edges=")) {
-		option_export_pack_edges(option + 18);
+	} else if (skip_prefix(option, "depth=", &option)) {
+		option_depth(option);
+	} else if (skip_prefix(option, "active-branches=", &option)) {
+		option_active_branches(option);
+	} else if (skip_prefix(option, "export-pack-edges=", &option)) {
+		option_export_pack_edges(option);
 	} else if (starts_with(option, "quiet")) {
 		show_stats = 0;
 	} else if (starts_with(option, "stats")) {
@@ -3243,15 +3247,16 @@ static int parse_one_option(const char *option)
 
 static int parse_one_feature(const char *feature, int from_stream)
 {
-	if (starts_with(feature, "date-format=")) {
-		option_date_format(feature + 12);
-	} else if (starts_with(feature, "import-marks=")) {
-		option_import_marks(feature + 13, from_stream, 0);
-	} else if (starts_with(feature, "import-marks-if-exists=")) {
-		option_import_marks(feature + strlen("import-marks-if-exists="),
-					from_stream, 1);
-	} else if (starts_with(feature, "export-marks=")) {
-		option_export_marks(feature + 13);
+	const char *arg;
+
+	if (skip_prefix(feature, "date-format=", &arg)) {
+		option_date_format(arg);
+	} else if (skip_prefix(feature, "import-marks=", &arg)) {
+		option_import_marks(arg, from_stream, 0);
+	} else if (skip_prefix(feature, "import-marks-if-exists=", &arg)) {
+		option_import_marks(arg, from_stream, 1);
+	} else if (skip_prefix(feature, "export-marks=", &arg)) {
+		option_export_marks(arg);
 	} else if (!strcmp(feature, "cat-blob")) {
 		; /* Don't die - this feature is supported */
 	} else if (!strcmp(feature, "relative-marks")) {
diff --git a/fetch-pack.c b/fetch-pack.c
index eeee2bb..3de3bd5 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -319,18 +319,19 @@ static int find_common(struct fetch_pack_args *args,
 
 	if (args->depth > 0) {
 		char *line;
+		const char *arg;
 		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 (skip_prefix(line, "shallow ", &arg)) {
+				if (get_sha1_hex(arg, sha1))
 					die("invalid shallow line: %s", line);
 				register_shallow(sha1);
 				continue;
 			}
-			if (starts_with(line, "unshallow ")) {
-				if (get_sha1_hex(line + 10, sha1))
+			if (skip_prefix(line, "unshallow ", &arg)) {
+				if (get_sha1_hex(arg, sha1))
 					die("invalid unshallow line: %s", line);
 				if (!lookup_object(sha1))
 					die("object not found: %s", line);
diff --git a/git.c b/git.c
index 7780572..b2bb09e 100644
--- a/git.c
+++ b/git.c
@@ -54,8 +54,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 		/*
 		 * Check remaining flags.
 		 */
-		if (starts_with(cmd, "--exec-path")) {
-			cmd += 11;
+		if (skip_prefix(cmd, "--exec-path", &cmd)) {
 			if (*cmd == '=')
 				git_set_argv_exec_path(cmd + 1);
 			else {
@@ -92,8 +91,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 (skip_prefix(cmd, "--git-dir=", &cmd)) {
+			setenv(GIT_DIR_ENVIRONMENT, cmd, 1);
 			if (envchanged)
 				*envchanged = 1;
 		} else if (!strcmp(cmd, "--namespace")) {
@@ -106,8 +105,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 (skip_prefix(cmd, "--namespace=", &cmd)) {
+			setenv(GIT_NAMESPACE_ENVIRONMENT, cmd, 1);
 			if (envchanged)
 				*envchanged = 1;
 		} else if (!strcmp(cmd, "--work-tree")) {
@@ -120,8 +119,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 (skip_prefix(cmd, "--work-tree=", &cmd)) {
+			setenv(GIT_WORK_TREE_ENVIRONMENT, cmd, 1);
 			if (envchanged)
 				*envchanged = 1;
 		} else if (!strcmp(cmd, "--bare")) {
@@ -578,8 +577,7 @@ int main(int argc, char **av)
 	 * So we just directly call the builtin handler, and die if
 	 * that one cannot handle it.
 	 */
-	if (starts_with(cmd, "git-")) {
-		cmd += 4;
+	if (skip_prefix(cmd, "git-", &cmd)) {
 		argv[0] = cmd;
 		handle_builtin(argc, argv);
 		die("cannot handle %s as a builtin", cmd);
diff --git a/help.c b/help.c
index b266b09..b0f1a69 100644
--- a/help.c
+++ b/help.c
@@ -251,11 +251,13 @@ static struct cmdnames aliases;
 
 static int git_unknown_cmd_config(const char *var, const char *value, void *cb)
 {
+	const char *p;
+
 	if (!strcmp(var, "help.autocorrect"))
 		autocorrect = git_config_int(var,value);
 	/* Also use aliases for command lookup */
-	if (starts_with(var, "alias."))
-		add_cmdname(&aliases, var + 6, strlen(var + 6));
+	if (skip_prefix(var, "alias.", &p))
+		add_cmdname(&aliases, p, strlen(p));
 
 	return git_default_config(var, value, cb);
 }
diff --git a/http-backend.c b/http-backend.c
index d2c0a62..57290d9 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -221,17 +221,19 @@ 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 (skip_prefix(var, "http.", &p)) {
 		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;
 			}
@@ -244,15 +246,16 @@ static int http_config(const char *var, const char *value, void *cb)
 
 static struct rpc_service *select_service(const char *name)
 {
+	const char *svc_name;
 	struct rpc_service *svc = NULL;
 	int i;
 
-	if (!starts_with(name, "git-"))
+	if (!skip_prefix(name, "git-", &svc_name))
 		forbidden("Unsupported service: '%s'", name);
 
 	for (i = 0; i < ARRAY_SIZE(rpc_service); i++) {
 		struct rpc_service *s = &rpc_service[i];
-		if (!strcmp(s->name, name + 4)) {
+		if (!strcmp(s->name, svc_name)) {
 			svc = s;
 			break;
 		}
diff --git a/http-push.c b/http-push.c
index de00d16..26dfa67 100644
--- a/http-push.c
+++ b/http-push.c
@@ -770,9 +770,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 *arg;
+			if (skip_prefix(ctx->cdata, "Second-", &arg))
+				lock->timeout = strtol(arg, NULL, 10);
 		} else if (!strcmp(ctx->name, DAV_ACTIVELOCK_TOKEN)) {
 			lock->token = xmalloc(strlen(ctx->cdata) + 1);
 			strcpy(lock->token, ctx->cdata);
@@ -1561,6 +1561,7 @@ static void fetch_symref(const char *path, char **symref, unsigned char *sha1)
 {
 	char *url;
 	struct strbuf buffer = STRBUF_INIT;
+	const char *name;
 
 	url = xmalloc(strlen(repo->url) + strlen(path) + 1);
 	sprintf(url, "%s%s", repo->url, path);
@@ -1578,8 +1579,8 @@ static void fetch_symref(const char *path, char **symref, unsigned char *sha1)
 		return;
 
 	/* If it's a symref, set the refname; otherwise try for a sha1 */
-	if (starts_with((char *)buffer.buf, "ref: ")) {
-		*symref = xmemdupz((char *)buffer.buf + 5, buffer.len - 6);
+	if (skip_prefix(buffer.buf, "ref: ", &name)) {
+		*symref = xmemdupz(name, buffer.len - (name - buffer.buf));
 	} else {
 		get_sha1_hex(buffer.buf, sha1);
 	}
-- 
2.0.0.566.gfe3e6b2

[PATCH 09/16] use skip_prefix to avoid repeating strings

[Jeff King]

It's a common idiom to match a prefix and then skip past it
with strlen, like:

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

This avoids magic numbers, but means we have to repeat the
string (and there is no compiler check that we didn't make a
typo in one of the strings).

We can use skip_prefix to handle this case without repeating
ourselves.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/checkout.c      |  4 ++--
 builtin/fmt-merge-msg.c |  6 +++---
 builtin/log.c           | 12 ++++++------
 diff.c                  | 27 +++++++++------------------
 help.c                  |  7 ++++---
 merge-recursive.c       | 15 ++++++++-------
 pretty.c                |  3 +--
 remote-curl.c           | 15 ++++++++-------
 remote.c                |  5 ++---
 sha1_name.c             |  4 +---
 10 files changed, 44 insertions(+), 54 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index f1dc56e..463cfee 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -776,8 +776,8 @@ static int switch_branches(const struct checkout_opts *opts,
 	if (!(flag & REF_ISSYMREF))
 		old.path = NULL;
 
-	if (old.path && starts_with(old.path, "refs/heads/"))
-		old.name = old.path + strlen("refs/heads/");
+	if (old.path)
+		skip_prefix(old.path, "refs/heads/", &old.name);
 
 	if (!new->name) {
 		new->name = "HEAD";
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 3c19241..ad3bc58 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -100,7 +100,8 @@ static int handle_line(char *line, struct merge_parents *merge_parents)
 {
 	int i, len = strlen(line);
 	struct origin_data *origin_data;
-	char *src, *origin;
+	char *src;
+	const char *origin;
 	struct src_data *src_data;
 	struct string_list_item *item;
 	int pulling_head = 0;
@@ -164,8 +165,7 @@ static int handle_line(char *line, struct merge_parents *merge_parents)
 		origin = line;
 		string_list_append(&src_data->tag, origin + 4);
 		src_data->head_status |= 2;
-	} else if (starts_with(line, "remote-tracking branch ")) {
-		origin = line + strlen("remote-tracking branch ");
+	} else if (skip_prefix(line, "remote-tracking branch ", &origin)) {
 		string_list_append(&src_data->r_branch, origin);
 		src_data->head_status |= 2;
 	} else {
diff --git a/builtin/log.c b/builtin/log.c
index a7ba211..0f59c25 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -872,7 +872,7 @@ static char *find_branch_name(struct rev_info *rev)
 	int i, positive = -1;
 	unsigned char branch_sha1[20];
 	const unsigned char *tip_sha1;
-	const char *ref;
+	const char *ref, *v;
 	char *full_ref, *branch = NULL;
 
 	for (i = 0; i < rev->cmdline.nr; i++) {
@@ -888,9 +888,9 @@ static char *find_branch_name(struct rev_info *rev)
 	ref = rev->cmdline.rev[positive].name;
 	tip_sha1 = rev->cmdline.rev[positive].item->sha1;
 	if (dwim_ref(ref, strlen(ref), branch_sha1, &full_ref) &&
-	    starts_with(full_ref, "refs/heads/") &&
+	    skip_prefix(full_ref, "refs/heads/", &v) &&
 	    !hashcmp(tip_sha1, branch_sha1))
-		branch = xstrdup(full_ref + strlen("refs/heads/"));
+		branch = xstrdup(v);
 	free(full_ref);
 	return branch;
 }
@@ -1394,10 +1394,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 
 		if (check_head) {
 			unsigned char sha1[20];
-			const char *ref;
+			const char *ref, *v;
 			ref = resolve_ref_unsafe("HEAD", sha1, 1, NULL);
-			if (ref && starts_with(ref, "refs/heads/"))
-				branch_name = xstrdup(ref + strlen("refs/heads/"));
+			if (ref && skip_prefix(ref, "refs/heads/", &v))
+				branch_name = xstrdup(v);
 			else
 				branch_name = xstrdup(""); /* no branch */
 		}
diff --git a/diff.c b/diff.c
index 97db932..2378ae4 100644
--- a/diff.c
+++ b/diff.c
@@ -3395,12 +3395,10 @@ int parse_long_opt(const char *opt, const char **argv,
 		   const char **optarg)
 {
 	const char *arg = argv[0];
-	if (arg[0] != '-' || arg[1] != '-')
+	if (!skip_prefix(arg, "--", &arg))
 		return 0;
-	arg += strlen("--");
-	if (!starts_with(arg, opt))
+	if (!skip_prefix(arg, opt, &arg))
 		return 0;
-	arg += strlen(opt);
 	if (*arg == '=') { /* stuck form: --option=value */
 		*optarg = arg + 1;
 		return 1;
@@ -3429,8 +3427,7 @@ static int stat_opt(struct diff_options *options, const char **av)
 
 	switch (*arg) {
 	case '-':
-		if (starts_with(arg, "-width")) {
-			arg += strlen("-width");
+		if (skip_prefix(arg, "-width", &arg)) {
 			if (*arg == '=')
 				width = strtoul(arg + 1, &end, 10);
 			else if (!*arg && !av[1])
@@ -3439,8 +3436,7 @@ static int stat_opt(struct diff_options *options, const char **av)
 				width = strtoul(av[1], &end, 10);
 				argcount = 2;
 			}
-		} else if (starts_with(arg, "-name-width")) {
-			arg += strlen("-name-width");
+		} else if (skip_prefix(arg, "-name-width", &arg)) {
 			if (*arg == '=')
 				name_width = strtoul(arg + 1, &end, 10);
 			else if (!*arg && !av[1])
@@ -3449,8 +3445,7 @@ static int stat_opt(struct diff_options *options, const char **av)
 				name_width = strtoul(av[1], &end, 10);
 				argcount = 2;
 			}
-		} else if (starts_with(arg, "-graph-width")) {
-			arg += strlen("-graph-width");
+		} else if (skip_prefix(arg, "-graph-width", &arg)) {
 			if (*arg == '=')
 				graph_width = strtoul(arg + 1, &end, 10);
 			else if (!*arg && !av[1])
@@ -3459,8 +3454,7 @@ static int stat_opt(struct diff_options *options, const char **av)
 				graph_width = strtoul(av[1], &end, 10);
 				argcount = 2;
 			}
-		} else if (starts_with(arg, "-count")) {
-			arg += strlen("-count");
+		} else if (skip_prefix(arg, "-count", &arg)) {
 			if (*arg == '=')
 				count = strtoul(arg + 1, &end, 10);
 			else if (!*arg && !av[1])
@@ -3905,16 +3899,13 @@ static int diff_scoreopt_parse(const char *opt)
 	cmd = *opt++;
 	if (cmd == '-') {
 		/* convert the long-form arguments into short-form versions */
-		if (starts_with(opt, "break-rewrites")) {
-			opt += strlen("break-rewrites");
+		if (skip_prefix(opt, "break-rewrites", &opt)) {
 			if (*opt == 0 || *opt++ == '=')
 				cmd = 'B';
-		} else if (starts_with(opt, "find-copies")) {
-			opt += strlen("find-copies");
+		} else if (skip_prefix(opt, "find-copies", &opt)) {
 			if (*opt == 0 || *opt++ == '=')
 				cmd = 'C';
-		} else if (starts_with(opt, "find-renames")) {
-			opt += strlen("find-renames");
+		} else if (skip_prefix(opt, "find-renames", &opt)) {
 			if (*opt == 0 || *opt++ == '=')
 				cmd = 'M';
 		}
diff --git a/help.c b/help.c
index b0f1a69..79e8007 100644
--- a/help.c
+++ b/help.c
@@ -414,11 +414,12 @@ static int append_similar_ref(const char *refname, const unsigned char *sha1,
 {
 	struct similar_ref_cb *cb = (struct similar_ref_cb *)(cb_data);
 	char *branch = strrchr(refname, '/') + 1;
+	const char *remote;
+
 	/* A remote branch of the same name is deemed similar */
-	if (starts_with(refname, "refs/remotes/") &&
+	if (skip_prefix(refname, "refs/remotes/", &remote) &&
 	    !strcmp(branch, cb->base_ref))
-		string_list_append(cb->similar_refs,
-				   refname + strlen("refs/remotes/"));
+		string_list_append(cb->similar_refs, remote);
 	return 0;
 }
 
diff --git a/merge-recursive.c b/merge-recursive.c
index cab16fa..f848001 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2063,6 +2063,8 @@ void init_merge_options(struct merge_options *o)
 
 int parse_merge_opt(struct merge_options *o, const char *s)
 {
+	const char *arg;
+
 	if (!s || !*s)
 		return -1;
 	if (!strcmp(s, "ours"))
@@ -2071,14 +2073,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 (skip_prefix(s, "subtree=", &arg))
+		o->subtree_shift = arg;
 	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 (skip_prefix(s, "diff-algorithm=", &arg)) {
+		long value = parse_algorithm_value(arg);
 		if (value < 0)
 			return -1;
 		/* clear out previous settings */
@@ -2096,9 +2098,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=");
-		if ((o->rename_score = parse_rename_score(&score)) == -1 || *score != 0)
+	else if (skip_prefix(s, "rename-threshold=", &arg)) {
+		if ((o->rename_score = parse_rename_score(&arg)) == -1 || *arg != 0)
 			return -1;
 	}
 	else
diff --git a/pretty.c b/pretty.c
index 4f51287..f24752a 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 (!skip_prefix(var, "pretty.", &name))
 		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/remote-curl.c b/remote-curl.c
index 4493b38..cdcca29 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -791,9 +791,9 @@ static void parse_fetch(struct strbuf *buf)
 	int alloc_heads = 0, nr_heads = 0;
 
 	do {
-		if (starts_with(buf->buf, "fetch ")) {
-			char *p = buf->buf + strlen("fetch ");
-			char *name;
+		const char *p;
+		if (skip_prefix(buf->buf, "fetch ", &p)) {
+			const char *name;
 			struct ref *ref;
 			unsigned char old_sha1[20];
 
@@ -968,6 +968,8 @@ int main(int argc, const char **argv)
 	http_init(remote, url.buf, 0);
 
 	do {
+		const char *arg;
+
 		if (strbuf_getline(&buf, stdin, '\n') == EOF) {
 			if (ferror(stdin))
 				fprintf(stderr, "Error reading command stream\n");
@@ -989,9 +991,8 @@ int main(int argc, const char **argv)
 		} else if (starts_with(buf.buf, "push ")) {
 			parse_push(&buf);
 
-		} else if (starts_with(buf.buf, "option ")) {
-			char *name = buf.buf + strlen("option ");
-			char *value = strchr(name, ' ');
+		} else if (skip_prefix(buf.buf, "option ", &arg)) {
+			char *value = strchr(arg, ' ');
 			int result;
 
 			if (value)
@@ -999,7 +1000,7 @@ int main(int argc, const char **argv)
 			else
 				value = "true";
 
-			result = set_option(name, value);
+			result = set_option(arg, value);
 			if (!result)
 				printf("ok\n");
 			else if (result < 0)
diff --git a/remote.c b/remote.c
index 0e9459c..30d2829 100644
--- a/remote.c
+++ b/remote.c
@@ -488,9 +488,8 @@ static void read_config(void)
 	current_branch = NULL;
 	head_ref = resolve_ref_unsafe("HEAD", sha1, 0, &flag);
 	if (head_ref && (flag & REF_ISSYMREF) &&
-	    starts_with(head_ref, "refs/heads/")) {
-		current_branch =
-			make_branch(head_ref + strlen("refs/heads/"), 0);
+	    skip_prefix(head_ref, "refs/heads/", &head_ref)) {
+		current_branch = make_branch(head_ref, 0);
 	}
 	git_config(handle_config, NULL);
 	if (branch_pushremote_name) {
diff --git a/sha1_name.c b/sha1_name.c
index 2b6322f..72e6ac6 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -911,10 +911,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 (skip_prefix(message, "checkout: moving from ", &match))
 		target = strstr(match, " to ");
-	}
 
 	if (!match || !target)
 		return 0;
-- 
2.0.0.566.gfe3e6b2

[PATCH 10/16] fast-import: use skip_prefix for parsing input

[Jeff King]

Fast-import does a lot of parsing of commands and
dispatching to sub-functions. For example, given "option
foo", we might recognize "option " using starts_with, and
then hand it off to parse_option() to do the rest.

However, we do not let parse_option know that we have parsed
the first part already. It gets the full buffer, and has to
skip past the uninteresting bits. Some functions simply add
a magic constant:

  char *option = command_buf.buf + 7;

Others use strlen:

  char *option = command_buf.buf + strlen("option ");

And others use strchr:

  char *option = strchr(command_buf.buf, ' ') + 1;

All of these are brittle and easy to get wrong (especially
given that the starts_with call and the code that assumes
the presence of the prefix are far apart). Instead, we can
use skip_prefix, and just pass each handler a pointer to its
arguments.

Signed-off-by: Jeff King <peff@peff.net>
---
 fast-import.c | 125 +++++++++++++++++++++++++---------------------------------
 1 file changed, 53 insertions(+), 72 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index a3ffe30..5f17adb 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -371,8 +371,8 @@ static volatile sig_atomic_t checkpoint_requested;
 static int cat_blob_fd = STDOUT_FILENO;
 
 static void parse_argv(void);
-static void parse_cat_blob(void);
-static void parse_ls(struct branch *b);
+static void parse_cat_blob(const char *p);
+static void parse_ls(const char *p, struct branch *b);
 
 static void write_branch_report(FILE *rpt, struct branch *b)
 {
@@ -1861,6 +1861,8 @@ static int read_next_command(void)
 	}
 
 	for (;;) {
+		const char *p;
+
 		if (unread_command_buf) {
 			unread_command_buf = 0;
 		} else {
@@ -1893,8 +1895,8 @@ static int read_next_command(void)
 			rc->prev->next = rc;
 			cmd_tail = rc;
 		}
-		if (starts_with(command_buf.buf, "cat-blob ")) {
-			parse_cat_blob();
+		if (skip_prefix(command_buf.buf, "cat-blob ", &p)) {
+			parse_cat_blob(p);
 			continue;
 		}
 		if (command_buf.buf[0] == '#')
@@ -2273,9 +2275,8 @@ static uintmax_t parse_mark_ref_space(const char **p)
 	return mark;
 }
 
-static void file_change_m(struct branch *b)
+static void file_change_m(const char *p, struct branch *b)
 {
-	const char *p = command_buf.buf + 2;
 	static struct strbuf uq = STRBUF_INIT;
 	const char *endp;
 	struct object_entry *oe;
@@ -2376,9 +2377,8 @@ static void file_change_m(struct branch *b)
 	tree_content_set(&b->branch_tree, p, sha1, mode, NULL);
 }
 
-static void file_change_d(struct branch *b)
+static void file_change_d(const char *p, struct branch *b)
 {
-	const char *p = command_buf.buf + 2;
 	static struct strbuf uq = STRBUF_INIT;
 	const char *endp;
 
@@ -2391,15 +2391,14 @@ static void file_change_d(struct branch *b)
 	tree_content_remove(&b->branch_tree, p, NULL, 1);
 }
 
-static void file_change_cr(struct branch *b, int rename)
+static void file_change_cr(const char *s, struct branch *b, int rename)
 {
-	const char *s, *d;
+	const char *d;
 	static struct strbuf s_uq = STRBUF_INIT;
 	static struct strbuf d_uq = STRBUF_INIT;
 	const char *endp;
 	struct tree_entry leaf;
 
-	s = command_buf.buf + 2;
 	strbuf_reset(&s_uq);
 	if (!unquote_c_style(&s_uq, s, &endp)) {
 		if (*endp != ' ')
@@ -2444,9 +2443,8 @@ static void file_change_cr(struct branch *b, int rename)
 		leaf.tree);
 }
 
-static void note_change_n(struct branch *b, unsigned char *old_fanout)
+static void note_change_n(const char *p, struct branch *b, unsigned char *old_fanout)
 {
-	const char *p = command_buf.buf + 2;
 	static struct strbuf uq = STRBUF_INIT;
 	struct object_entry *oe;
 	struct branch *s;
@@ -2587,7 +2585,7 @@ static int parse_from(struct branch *b)
 	const char *from;
 	struct branch *s;
 
-	if (!starts_with(command_buf.buf, "from "))
+	if (!skip_prefix(command_buf.buf, "from ", &from))
 		return 0;
 
 	if (b->branch_tree.tree) {
@@ -2595,7 +2593,6 @@ static int parse_from(struct branch *b)
 		b->branch_tree.tree = NULL;
 	}
 
-	from = strchr(command_buf.buf, ' ') + 1;
 	s = lookup_branch(from);
 	if (b == s)
 		die("Can't create a branch from itself: %s", b->name);
@@ -2636,8 +2633,7 @@ static struct hash_list *parse_merge(unsigned int *count)
 	struct branch *s;
 
 	*count = 0;
-	while (starts_with(command_buf.buf, "merge ")) {
-		from = strchr(command_buf.buf, ' ') + 1;
+	while (skip_prefix(command_buf.buf, "merge ", &from)) {
 		n = xmalloc(sizeof(*n));
 		s = lookup_branch(from);
 		if (s)
@@ -2668,11 +2664,10 @@ static struct hash_list *parse_merge(unsigned int *count)
 	return list;
 }
 
-static void parse_new_commit(void)
+static void parse_new_commit(const char *arg)
 {
 	static struct strbuf msg = STRBUF_INIT;
 	struct branch *b;
-	char *sp;
 	char *author = NULL;
 	char *committer = NULL;
 	struct hash_list *merge_list = NULL;
@@ -2680,11 +2675,9 @@ static void parse_new_commit(void)
 	unsigned char prev_fanout, new_fanout;
 	const char *v;
 
-	/* Obtain the branch name from the rest of our command */
-	sp = strchr(command_buf.buf, ' ') + 1;
-	b = lookup_branch(sp);
+	b = lookup_branch(arg);
 	if (!b)
-		b = new_branch(sp);
+		b = new_branch(arg);
 
 	read_next_command();
 	parse_mark();
@@ -2713,20 +2706,22 @@ static void parse_new_commit(void)
 
 	/* file_change* */
 	while (command_buf.len > 0) {
-		if (starts_with(command_buf.buf, "M "))
-			file_change_m(b);
-		else if (starts_with(command_buf.buf, "D "))
-			file_change_d(b);
-		else if (starts_with(command_buf.buf, "R "))
-			file_change_cr(b, 1);
-		else if (starts_with(command_buf.buf, "C "))
-			file_change_cr(b, 0);
-		else if (starts_with(command_buf.buf, "N "))
-			note_change_n(b, &prev_fanout);
+		const char *v;
+
+		if (skip_prefix(command_buf.buf, "M ", &v))
+			file_change_m(v, b);
+		else if (skip_prefix(command_buf.buf, "D ", &v))
+			file_change_d(v, b);
+		else if (skip_prefix(command_buf.buf, "R ", &v))
+			file_change_cr(v, b, 1);
+		else if (skip_prefix(command_buf.buf, "C ", &v))
+			file_change_cr(v, b, 0);
+		else if (skip_prefix(command_buf.buf, "N ", &v))
+			note_change_n(v, b, &prev_fanout);
 		else if (!strcmp("deleteall", command_buf.buf))
 			file_change_deleteall(b);
-		else if (starts_with(command_buf.buf, "ls "))
-			parse_ls(b);
+		else if (skip_prefix(command_buf.buf, "ls ", &v))
+			parse_ls(v, b);
 		else {
 			unread_command_buf = 1;
 			break;
@@ -2769,10 +2764,9 @@ static void parse_new_commit(void)
 	b->last_commit = object_count_by_type[OBJ_COMMIT];
 }
 
-static void parse_new_tag(void)
+static void parse_new_tag(const char *arg)
 {
 	static struct strbuf msg = STRBUF_INIT;
-	char *sp;
 	const char *from;
 	char *tagger;
 	struct branch *s;
@@ -2782,11 +2776,9 @@ static void parse_new_tag(void)
 	enum object_type type;
 	const char *v;
 
-	/* Obtain the new tag name from the rest of our command */
-	sp = strchr(command_buf.buf, ' ') + 1;
 	t = pool_alloc(sizeof(struct tag));
 	memset(t, 0, sizeof(struct tag));
-	t->name = pool_strdup(sp);
+	t->name = pool_strdup(arg);
 	if (last_tag)
 		last_tag->next_tag = t;
 	else
@@ -2795,9 +2787,8 @@ static void parse_new_tag(void)
 	read_next_command();
 
 	/* from ... */
-	if (!starts_with(command_buf.buf, "from "))
+	if (!skip_prefix(command_buf.buf, "from ", &from))
 		die("Expected from command, got %s", command_buf.buf);
-	from = strchr(command_buf.buf, ' ') + 1;
 	s = lookup_branch(from);
 	if (s) {
 		if (is_null_sha1(s->sha1))
@@ -2853,14 +2844,11 @@ static void parse_new_tag(void)
 		t->pack_id = pack_id;
 }
 
-static void parse_reset_branch(void)
+static void parse_reset_branch(const char *arg)
 {
 	struct branch *b;
-	char *sp;
 
-	/* Obtain the branch name from the rest of our command */
-	sp = strchr(command_buf.buf, ' ') + 1;
-	b = lookup_branch(sp);
+	b = lookup_branch(arg);
 	if (b) {
 		hashclr(b->sha1);
 		hashclr(b->branch_tree.versions[0].sha1);
@@ -2871,7 +2859,7 @@ static void parse_reset_branch(void)
 		}
 	}
 	else
-		b = new_branch(sp);
+		b = new_branch(arg);
 	read_next_command();
 	parse_from(b);
 	if (command_buf.len > 0)
@@ -2929,14 +2917,12 @@ static void cat_blob(struct object_entry *oe, unsigned char sha1[20])
 		free(buf);
 }
 
-static void parse_cat_blob(void)
+static void parse_cat_blob(const char *p)
 {
-	const char *p;
 	struct object_entry *oe = oe;
 	unsigned char sha1[20];
 
 	/* cat-blob SP <object> LF */
-	p = command_buf.buf + strlen("cat-blob ");
 	if (*p == ':') {
 		oe = find_mark(parse_mark_ref_eol(p));
 		if (!oe)
@@ -3053,14 +3039,12 @@ static void print_ls(int mode, const unsigned char *sha1, const char *path)
 	cat_blob_write(line.buf, line.len);
 }
 
-static void parse_ls(struct branch *b)
+static void parse_ls(const char *p, struct branch *b)
 {
-	const char *p;
 	struct tree_entry *root = NULL;
 	struct tree_entry leaf = {NULL};
 
 	/* ls SP (<tree-ish> SP)? <path> */
-	p = command_buf.buf + strlen("ls ");
 	if (*p == '"') {
 		if (!b)
 			die("Not in a commit: %s", command_buf.buf);
@@ -3276,10 +3260,8 @@ static int parse_one_feature(const char *feature, int from_stream)
 	return 1;
 }
 
-static void parse_feature(void)
+static void parse_feature(const char *feature)
 {
-	char *feature = command_buf.buf + 8;
-
 	if (seen_data_command)
 		die("Got feature command '%s' after data command", feature);
 
@@ -3289,10 +3271,8 @@ static void parse_feature(void)
 	die("This version of fast-import does not support feature %s.", feature);
 }
 
-static void parse_option(void)
+static void parse_option(const char *option)
 {
-	char *option = command_buf.buf + 11;
-
 	if (seen_data_command)
 		die("Got option command '%s' after data command", option);
 
@@ -3408,26 +3388,27 @@ int main(int argc, char **argv)
 	set_die_routine(die_nicely);
 	set_checkpoint_signal();
 	while (read_next_command() != EOF) {
+		const char *v;
 		if (!strcmp("blob", command_buf.buf))
 			parse_new_blob();
-		else if (starts_with(command_buf.buf, "ls "))
-			parse_ls(NULL);
-		else if (starts_with(command_buf.buf, "commit "))
-			parse_new_commit();
-		else if (starts_with(command_buf.buf, "tag "))
-			parse_new_tag();
-		else if (starts_with(command_buf.buf, "reset "))
-			parse_reset_branch();
+		else if (skip_prefix(command_buf.buf, "ls ", &v))
+			parse_ls(v, NULL);
+		else if (skip_prefix(command_buf.buf, "commit ", &v))
+			parse_new_commit(v);
+		else if (skip_prefix(command_buf.buf, "tag ", &v))
+			parse_new_tag(v);
+		else if (skip_prefix(command_buf.buf, "reset ", &v))
+			parse_reset_branch(v);
 		else if (!strcmp("checkpoint", command_buf.buf))
 			parse_checkpoint();
 		else if (!strcmp("done", command_buf.buf))
 			break;
 		else if (starts_with(command_buf.buf, "progress "))
 			parse_progress();
-		else if (starts_with(command_buf.buf, "feature "))
-			parse_feature();
-		else if (starts_with(command_buf.buf, "option git "))
-			parse_option();
+		else if (skip_prefix(command_buf.buf, "feature ", &v))
+			parse_feature(v);
+		else if (skip_prefix(command_buf.buf, "option git ", &v))
+			parse_option(v);
 		else if (starts_with(command_buf.buf, "option "))
 			/* ignore non-git options*/;
 		else
-- 
2.0.0.566.gfe3e6b2

[PATCH 11/16] daemon: use skip_prefix to avoid magic numbers

[Jeff King]

Like earlier cases, we can use skip_prefix to avoid magic
numbers that must match the length of starts_with prefixes.
However, the numbers are a little more complicated here, as
we keep parsing past the prefix. We can solve it by keeping
a running pointer as we parse; its final value is the
location we want.

Signed-off-by: Jeff King <peff@peff.net>
---
 daemon.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/daemon.c b/daemon.c
index 6d25828..1eb6631 100644
--- a/daemon.c
+++ b/daemon.c
@@ -626,15 +626,16 @@ static int execute(void)
 
 	for (i = 0; i < ARRAY_SIZE(daemon_service); i++) {
 		struct daemon_service *s = &(daemon_service[i]);
-		int namelen = strlen(s->name);
-		if (starts_with(line, "git-") &&
-		    !strncmp(s->name, line + 4, namelen) &&
-		    line[namelen + 4] == ' ') {
+		const char *arg;
+
+		if (skip_prefix(line, "git-", &arg) &&
+		    skip_prefix(arg, s->name, &arg) &&
+		    *arg++ == ' ') {
 			/*
 			 * Note: The directory here is probably context sensitive,
 			 * and might depend on the actual service being performed.
 			 */
-			return run_service(line + namelen + 5, s);
+			return run_service(arg, s);
 		}
 	}
 
-- 
2.0.0.566.gfe3e6b2

[PATCH 12/16] stat_opt: check extra strlen call

[Jeff King]

As in earlier commits, the diff option parser uses
starts_with to find that an argument starts with "--stat-",
and then adds strlen("stat-") to find the rest of the
option.

However, in this case the starts_with and the strlen are
separated across functions, making it easy to call the
latter without the former. Let's use skip_prefix instead of
raw pointer arithmetic to catch such a case.

Signed-off-by: Jeff King <peff@peff.net>
---
Another possibility would be for stat_opt to take only the
prefix-skipped part of the string. But that would involve refactoring
its use of "av" (it needs the whole array because it may need to grab a
follow-on argument).

 diff.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index 2378ae4..06bdfb8 100644
--- a/diff.c
+++ b/diff.c
@@ -3422,7 +3422,8 @@ static int stat_opt(struct diff_options *options, const char **av)
 	int count = options->stat_count;
 	int argcount = 1;
 
-	arg += strlen("--stat");
+	if (!skip_prefix(arg, "--stat", &arg))
+		die("BUG: stat option does not begin with --stat: %s", arg);
 	end = (char *)arg;
 
 	switch (*arg) {
-- 
2.0.0.566.gfe3e6b2

[PATCH 13/16] fast-import: refactor parsing of spaces

[Jeff King]

When we see a file change in a commit, we expect one of:

  1. A mark.

  2. An "inline" keyword.

  3. An object sha1.

The handling of spaces is inconsistent between the three
options. Option 1 calls a sub-function which checks for the
space, but doesn't parse past it. Option 2 parses the space,
then deliberately avoids moving the pointer past it. Option
3 detects the space locally but doesn't move past it.

This is confusing, because it looks like option 1 forgets to
check for the space (it's just buried). And option 2 checks
for "inline ", but only moves strlen("inline") characters
forward, which looks like a bug but isn't.

We can make this more clear by just having each branch move
past the space as it is checked (and we can replace the
doubled use of "inline" with a call to skip_prefix).

Signed-off-by: Jeff King <peff@peff.net>
---
 fast-import.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 5f17adb..55ca7d8 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2269,7 +2269,7 @@ static uintmax_t parse_mark_ref_space(const char **p)
 	char *end;
 
 	mark = parse_mark_ref(*p, &end);
-	if (*end != ' ')
+	if (*end++ != ' ')
 		die("Missing space after mark: %s", command_buf.buf);
 	*p = end;
 	return mark;
@@ -2304,20 +2304,17 @@ static void file_change_m(const char *p, struct branch *b)
 	if (*p == ':') {
 		oe = find_mark(parse_mark_ref_space(&p));
 		hashcpy(sha1, oe->idx.sha1);
-	} else if (starts_with(p, "inline ")) {
+	} else if (skip_prefix(p, "inline ", &p)) {
 		inline_data = 1;
 		oe = NULL; /* not used with inline_data, but makes gcc happy */
-		p += strlen("inline");  /* advance to space */
 	} else {
 		if (get_sha1_hex(p, sha1))
 			die("Invalid dataref: %s", command_buf.buf);
 		oe = find_object(sha1);
 		p += 40;
-		if (*p != ' ')
+		if (*p++ != ' ')
 			die("Missing space after SHA1: %s", command_buf.buf);
 	}
-	assert(*p == ' ');
-	p++;  /* skip space */
 
 	strbuf_reset(&uq);
 	if (!unquote_c_style(&uq, p, &endp)) {
@@ -2474,20 +2471,17 @@ static void note_change_n(const char *p, struct branch *b, unsigned char *old_fa
 	if (*p == ':') {
 		oe = find_mark(parse_mark_ref_space(&p));
 		hashcpy(sha1, oe->idx.sha1);
-	} else if (starts_with(p, "inline ")) {
+	} else if (skip_prefix(p, "inline ", &p)) {
 		inline_data = 1;
 		oe = NULL; /* not used with inline_data, but makes gcc happy */
-		p += strlen("inline");  /* advance to space */
 	} else {
 		if (get_sha1_hex(p, sha1))
 			die("Invalid dataref: %s", command_buf.buf);
 		oe = find_object(sha1);
 		p += 40;
-		if (*p != ' ')
+		if (*p++ != ' ')
 			die("Missing space after SHA1: %s", command_buf.buf);
 	}
-	assert(*p == ' ');
-	p++;  /* skip space */
 
 	/* <commit-ish> */
 	s = lookup_branch(p);
@@ -3005,6 +2999,8 @@ static struct object_entry *parse_treeish_dataref(const char **p)
 			die("Invalid dataref: %s", command_buf.buf);
 		e = find_object(sha1);
 		*p += 40;
+		if (*(*p)++ != ' ')
+			die("Missing space after tree-ish: %s", command_buf.buf);
 	}
 
 	while (!e || e->type != OBJ_TREE)
@@ -3056,8 +3052,6 @@ static void parse_ls(const char *p, struct branch *b)
 		if (!is_null_sha1(root->versions[1].sha1))
 			root->versions[1].mode = S_IFDIR;
 		load_tree(root);
-		if (*p++ != ' ')
-			die("Missing space after tree-ish: %s", command_buf.buf);
 	}
 	if (*p == '"') {
 		static struct strbuf uq = STRBUF_INIT;
-- 
2.0.0.566.gfe3e6b2

[PATCH 14/16] fetch-pack: refactor parsing in get_ack

[Jeff King]

There are several uses of the magic number "line+45" when
parsing ACK lines from the server, and it's rather unclear
why 45 is the correct number. We can make this more clear by
keeping a running pointer as we parse, using skip_prefix to
jump past the first "ACK ", then adding 40 to jump past
get_sha1_hex (which is still magical, but hopefully 40 is
less magical to readers of git code).

Note that this actually puts us at line+44. The original
required some character between the sha1 and further ACK
flags (it is supposed to be a space, but we never enforced
that). We start our search for flags at line+44, which
meanas we are slightly more liberal than the old code.

Signed-off-by: Jeff King <peff@peff.net>
---
I actually think we could tighten this even more and drop the strstrs,
too, like:

  arg += 40;
  if (*arg++ != ' ')
	return ACK;
  if (!strcmp(arg, "continue"))
	return ACK_continue;

and so on. But I wasn't sure if there was a reason for the use of
strstr. According to pack-protocol.txt, we would only get one at a time,
and always with a single space between them.

 fetch-pack.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index 3de3bd5..72ec520 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -189,20 +189,23 @@ static enum ack_type get_ack(int fd, unsigned char *result_sha1)
 {
 	int len;
 	char *line = packet_read_line(fd, &len);
+	const char *arg;
 
 	if (!len)
 		die("git fetch-pack: expected ACK/NAK, got EOF");
 	if (!strcmp(line, "NAK"))
 		return NAK;
-	if (starts_with(line, "ACK ")) {
-		if (!get_sha1_hex(line+4, result_sha1)) {
-			if (len < 45)
+	if (skip_prefix(line, "ACK ", &arg)) {
+		if (!get_sha1_hex(arg, result_sha1)) {
+			arg += 40;
+			len -= arg - line;
+			if (len < 1)
 				return ACK;
-			if (strstr(line+45, "continue"))
+			if (strstr(arg, "continue"))
 				return ACK_continue;
-			if (strstr(line+45, "common"))
+			if (strstr(arg, "common"))
 				return ACK_common;
-			if (strstr(line+45, "ready"))
+			if (strstr(arg, "ready"))
 				return ACK_ready;
 			return ACK;
 		}
-- 
2.0.0.566.gfe3e6b2

[PATCH 15/16] git: avoid magic number with skip_prefix

[Jeff King]

After handling options, any leftover arguments should be
commands. However, we pass through "--help" and "--version",
so that we convert them into "git help" and "git version"
respectively.

This is a straightforward use of skip_prefix to avoid a
magic number, but while we are there, it is worth adding a
comment to explain this otherwise confusing behavior.

Signed-off-by: Jeff King <peff@peff.net>
---
Another option would be to teach handle_options to convert "--version"
into "version" itself. That's more disruptive, but I think would be
less confusing.

 git.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git.c b/git.c
index b2bb09e..1537f00 100644
--- a/git.c
+++ b/git.c
@@ -588,8 +588,8 @@ int main(int argc, char **av)
 	argc--;
 	handle_options(&argv, &argc, NULL);
 	if (argc > 0) {
-		if (starts_with(argv[0], "--"))
-			argv[0] += 2;
+		/* translate --help and --version into commands */
+		skip_prefix(argv[0], "--", &argv[0]);
 	} else {
 		/* The user didn't specify a command; give them help */
 		commit_pager_choice();
-- 
2.0.0.566.gfe3e6b2

[PATCH 16/16] use skip_prefix to avoid repeated calculations

[Jeff King]

In some cases, we use starts_with to check for a prefix, and
then use an already-calculated prefix length to advance a
pointer past the prefix. There are no magic numbers or
duplicated strings here, but we can still make the code
simpler and more obvious by using skip_prefix.

Signed-off-by: Jeff King <peff@peff.net>
---
 help.c | 11 +++++------
 http.c |  3 +--
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/help.c b/help.c
index 79e8007..f31f29a 100644
--- a/help.c
+++ b/help.c
@@ -129,7 +129,6 @@ static void list_commands_in_dir(struct cmdnames *cmds,
 					 const char *path,
 					 const char *prefix)
 {
-	int prefix_len;
 	DIR *dir = opendir(path);
 	struct dirent *de;
 	struct strbuf buf = STRBUF_INIT;
@@ -139,15 +138,15 @@ static void list_commands_in_dir(struct cmdnames *cmds,
 		return;
 	if (!prefix)
 		prefix = "git-";
-	prefix_len = strlen(prefix);
 
 	strbuf_addf(&buf, "%s/", path);
 	len = buf.len;
 
 	while ((de = readdir(dir)) != NULL) {
+		const char *ent;
 		int entlen;
 
-		if (!starts_with(de->d_name, prefix))
+		if (!skip_prefix(de->d_name, prefix, &ent))
 			continue;
 
 		strbuf_setlen(&buf, len);
@@ -155,11 +154,11 @@ static void list_commands_in_dir(struct cmdnames *cmds,
 		if (!is_executable(buf.buf))
 			continue;
 
-		entlen = strlen(de->d_name) - prefix_len;
-		if (has_extension(de->d_name, ".exe"))
+		entlen = strlen(ent);
+		if (has_extension(ent, ".exe"))
 			entlen -= 4;
 
-		add_cmdname(cmds, de->d_name + prefix_len, entlen);
+		add_cmdname(cmds, ent, entlen);
 	}
 	closedir(dir);
 	strbuf_release(&buf);
diff --git a/http.c b/http.c
index 2b4f6a3..f04621e 100644
--- a/http.c
+++ b/http.c
@@ -1087,11 +1087,10 @@ static int update_url_from_redirect(struct strbuf *base,
 	if (!strcmp(asked, got->buf))
 		return 0;
 
-	if (!starts_with(asked, base->buf))
+	if (!skip_prefix(asked, base->buf, &tail))
 		die("BUG: update_url_from_redirect: %s is not a superset of %s",
 		    asked, base->buf);
 
-	tail = asked + base->len;
 	tail_len = strlen(tail);
 
 	if (got->len < tail_len ||
-- 
2.0.0.566.gfe3e6b2

Re: [PATCH 0/16] skip_prefix refactoring and cleanups

[Tanay Abhra]

Hi,

I was about to send a patch like this series today, I am attaching it below.

-- >8 --
Subject: [PATCH] imap-send: use skip_prefix instead of using magic numbers

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 imap-send.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 83a6ed2..524fbab 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1328,13 +1328,9 @@ static char *imap_folder;
 
 static int git_imap_config(const char *key, const char *val, void *cb)
 {
-	char imap_key[] = "imap.";
-
-	if (strncmp(key, imap_key, sizeof imap_key - 1))
+	if (!skip_prefix(key, "imap.", &key))
 		return 0;
 
-	key += sizeof imap_key - 1;
-
 	/* check booleans first, and barf on others */
 	if (!strcmp("sslverify", key))
 		server.ssl_verify = git_config_bool(key, val);
-- 
1.9.0.GIT

[PATCH 17/16] http-push: refactor parsing of remote object names

[Jeff King]

We get loose object names like "objects/??/..." from the
remote side, and need to convert them to their hex
representation.

The code to do so is rather hard to follow, as it uses some
calculated lengths whose origins are hard to understand and
verify (e.g., the path must be exactly 49 characters long.
why? Why doesn't the strcpy overflow obj_hex, which is the
same length as path?).

We can simplify this a bit by using skip_prefix, using standard
40- and 20-character buffers for hex and binary sha1s, and
adding some comments.

We also drop a totally bogus comment that claims strlcpy
cannot be used because "path" is not NUL-terminated. Right
between a call to strlen(path) and strcpy(path).

Signed-off-by: Jeff King <peff@peff.net>
---
I found this one while doing the xstrfmt series, but it actually doesn't
need xstrfmt, and does need skip_prefix, so it probably goes better on
the skip-prefix topic.

It's still a little more magical than I would like, but I think this is
the best we can do while still building on get_sha1_hex. Parsing it
left-to-right would be better, but we would essentially end up
reimplementing get_sha1_hex.

 http-push.c | 38 +++++++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/http-push.c b/http-push.c
index 26dfa67..c5c95e8 100644
--- a/http-push.c
+++ b/http-push.c
@@ -719,14 +719,10 @@ static int fetch_indices(void)
 	return ret;
 }
 
-static void one_remote_object(const char *hex)
+static void one_remote_object(const unsigned char *sha1)
 {
-	unsigned char sha1[20];
 	struct object *obj;
 
-	if (get_sha1_hex(hex, sha1) != 0)
-		return;
-
 	obj = lookup_object(sha1);
 	if (!obj)
 		obj = parse_object(sha1);
@@ -1020,26 +1016,38 @@ static void remote_ls(const char *path, int flags,
 		      void (*userFunc)(struct remote_ls_ctx *ls),
 		      void *userData);
 
+/* extract hex from sharded "xx/x{40}" filename */
+static int get_sha1_hex_from_objpath(const char *path, unsigned char *sha1)
+{
+	char hex[40];
+
+	if (strlen(path) != 41)
+		return -1;
+
+	memcpy(hex, path, 2);
+	path += 2;
+	path++; /* skip '/' */
+	memcpy(hex, path, 38);
+
+	return get_sha1_hex(hex, sha1);
+}
+
 static void process_ls_object(struct remote_ls_ctx *ls)
 {
 	unsigned int *parent = (unsigned int *)ls->userData;
-	char *path = ls->dentry_name;
-	char *obj_hex;
+	const char *path = ls->dentry_name;
+	unsigned char sha1[20];
 
 	if (!strcmp(ls->path, ls->dentry_name) && (ls->flags & IS_DIR)) {
 		remote_dir_exists[*parent] = 1;
 		return;
 	}
 
-	if (strlen(path) != 49)
+	if (!skip_prefix(path, "objects/", &path) ||
+	    get_sha1_hex_from_objpath(path, sha1))
 		return;
-	path += 8;
-	obj_hex = xmalloc(strlen(path));
-	/* NB: path is not null-terminated, can not use strlcpy here */
-	memcpy(obj_hex, path, 2);
-	strcpy(obj_hex + 2, path + 3);
-	one_remote_object(obj_hex);
-	free(obj_hex);
+
+	one_remote_object(sha1);
 }
 
 static void process_ls_ref(struct remote_ls_ctx *ls)
-- 
2.0.0.566.gfe3e6b2

Re: [PATCH 17/16] http-push: refactor parsing of remote object names

[Jeff King]

On Thu, Jun 19, 2014 at 05:58:10PM -0400, Jeff King wrote:

> It's still a little more magical than I would like, but I think this is
> the best we can do while still building on get_sha1_hex. Parsing it
> left-to-right would be better, but we would essentially end up
> reimplementing get_sha1_hex.

Just for fun, that would look something like this (on top):

diff --git a/http-push.c b/http-push.c
index c5c95e8..2425c61 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1016,20 +1016,34 @@ static void remote_ls(const char *path, int flags,
 		      void (*userFunc)(struct remote_ls_ctx *ls),
 		      void *userData);
 
+static inline int parse_one_hex(unsigned char **to, const char **from)
+{
+	unsigned int val;
+
+	/* avoid reading past end-of-string for from[1] */
+	if (!(*from)[0])
+		return -1;
+	val = (hexval((*from)[0]) << 4) | hexval((*from)[1]);
+	if (val & ~0xff)
+		return -1;
+	*(*to)++ = val;
+	(*from) += 2;
+	return 0;
+}
+
 /* extract hex from sharded "xx/x{40}" filename */
 static int get_sha1_hex_from_objpath(const char *path, unsigned char *sha1)
 {
-	char hex[40];
+	int i;
 
-	if (strlen(path) != 41)
+	if (parse_one_hex(&sha1, &path) < 0)
 		return -1;
-
-	memcpy(hex, path, 2);
-	path += 2;
-	path++; /* skip '/' */
-	memcpy(hex, path, 38);
-
-	return get_sha1_hex(hex, sha1);
+	if (*path++ != '/')
+		return -1;
+	for (i = 1; i < 20; i++)
+		if (parse_one_hex(&sha1, &path) < 0)
+			return -1;
+	return 0;
 }
 
 static void process_ls_object(struct remote_ls_ctx *ls)

In theory we could factor parse_one_hex to share with get_sha1_hex, but
I am hesitant to touch get_sha1_hex, as it is used in a lot of tight
loops.

-Peff

Re: [PATCH 04/16] refactor skip_prefix to return a boolean

[Eric Sunshine]

On Wed, Jun 18, 2014 at 3:44 PM, Jeff King <peff@peff.net> wrote:
> The skip_prefix function returns a pointer to the content
> past the prefix, or NULL if the prefix was not found. While
> this is nice and simple, in practice it makes it hard to use
> for two reasons:
>
>   1. When you want to conditionally skip or keep the string
>      as-is, you have to introduce a second temporary
>      variable. For example:
>
>        tmp = skip_prefix(buf, "foo");
>        if (tmp)
>                buf = tmp;
>
>   2. It is verbose to check the outcome in a conditional, as
>      you need extra parentheses to silence compiler
>      warnings. For example:
>
>        if ((cp = skip_prefix(buf, "foo"))
>                /* do something with cp */
>
> Both of these make it harder to use for long if-chains, and
> we tend to use starts_with instead. However, the first line
> of "do something" is often to then skip forward in buf past
> the prefix, either using a magic constant or with an extra
> strlen (which is generally computed at compile time, but
> means we are repeating ourselves).
>
> This patch refactors skip_prefix to return a simple boolean,
> and to provide the pointer value as an out-parameter. If the
> prefix is not found, the out-parameter is untouched. This
> lets you write:
>
>   if (skip_prefix(arg, "foo ", &arg))
>           do_foo(arg);
>   else if (skip_prefix(arg, "bar ", &arg))
>           do_bar(arg);
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> diff --git a/git-compat-util.h b/git-compat-util.h
> index b6f03b3..556c839 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -349,13 +349,31 @@ extern void set_die_is_recursing_routine(int (*routine)(void));
>  extern int starts_with(const char *str, const char *prefix);
>  extern int ends_with(const char *str, const char *suffix);
>
> -static inline const char *skip_prefix(const char *str, const char *prefix)
> +/*
> + * If "str" begins with "prefix", return 1. If out is non-NULL,
> + * it it set to str + strlen(prefix) (i.e., the prefix is skipped).

The documentation claims that 'out' can be NULL, however, the code
does not respect this. NULL 'out' seems rather pointless (unless you
want an alias for starts_with()), so presumably the documentation is
incorrect.

> + *
> + * Otherwise, returns 0 and out is left untouched.
> + *
> + * Examples:
> + *
> + *   [extract branch name, fail if not a branch]
> + *   if (!skip_prefix(ref, "refs/heads/", &branch)
> + *     return -1;
> + *
> + *   [skip prefix if present, otherwise use whole string]
> + *   skip_prefix(name, "refs/heads/", &name);
> + */
> +static inline int skip_prefix(const char *str, const char *prefix,
> +                             const char **out)
>  {
>         do {
> -               if (!*prefix)
> -                       return str;
> +               if (!*prefix) {
> +                       *out = str;
> +                       return 1;
> +               }
>         } while (*str++ == *prefix++);
> -       return NULL;
> +       return 0;
>  }
>
>  #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)

Re: [PATCH 04/16] refactor skip_prefix to return a boolean

[Jeff King]

On Thu, Jun 19, 2014 at 09:59:39PM -0400, Eric Sunshine wrote:

> > diff --git a/git-compat-util.h b/git-compat-util.h
> > index b6f03b3..556c839 100644
> > --- a/git-compat-util.h
> > +++ b/git-compat-util.h
> > @@ -349,13 +349,31 @@ extern void set_die_is_recursing_routine(int (*routine)(void));
> >  extern int starts_with(const char *str, const char *prefix);
> >  extern int ends_with(const char *str, const char *suffix);
> >
> > -static inline const char *skip_prefix(const char *str, const char *prefix)
> > +/*
> > + * If "str" begins with "prefix", return 1. If out is non-NULL,
> > + * it it set to str + strlen(prefix) (i.e., the prefix is skipped).
> 
> The documentation claims that 'out' can be NULL, however, the code
> does not respect this. NULL 'out' seems rather pointless (unless you
> want an alias for starts_with()), so presumably the documentation is
> incorrect.

Thanks for catching. My original version (before I sent to the list) did
allow for a conditional NULL out, but I realized there was exactly one
caller who wanted this, and that they would be better off using
starts_with. I added patch 3 ("avoid using skip_prefix as a boolean")
and stripped the conditional from skip_prefix, but forgot to update the
comment.

I think we'd just want to squash the patch below (I also took the
opportunity to fix a typo and clarify the text a bit):

diff --git a/git-compat-util.h b/git-compat-util.h
index 556c839..1187e1a 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -350,8 +350,9 @@ extern int starts_with(const char *str, const char *prefix);
 extern int ends_with(const char *str, const char *suffix);
 
 /*
- * If "str" begins with "prefix", return 1. If out is non-NULL,
- * it it set to str + strlen(prefix) (i.e., the prefix is skipped).
+ * If the string "str" begins with the string found in "prefix", return 1.
+ * The "out" parameter is set to "str + strlen(prefix)" (i.e., to the point in
+ * the string right after the prefix).
  *
  * Otherwise, returns 0 and out is left untouched.
  *

-Peff

Re: [PATCH 04/16] refactor skip_prefix to return a boolean

[Eric Sunshine]

On Thu, Jun 19, 2014 at 10:08 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Jun 19, 2014 at 09:59:39PM -0400, Eric Sunshine wrote:
>
>> > diff --git a/git-compat-util.h b/git-compat-util.h
>> > index b6f03b3..556c839 100644
>> > --- a/git-compat-util.h
>> > +++ b/git-compat-util.h
>> > @@ -349,13 +349,31 @@ extern void set_die_is_recursing_routine(int (*routine)(void));
>> >  extern int starts_with(const char *str, const char *prefix);
>> >  extern int ends_with(const char *str, const char *suffix);
>> >
>> > -static inline const char *skip_prefix(const char *str, const char *prefix)
>> > +/*
>> > + * If "str" begins with "prefix", return 1. If out is non-NULL,
>> > + * it it set to str + strlen(prefix) (i.e., the prefix is skipped).
>>
>> The documentation claims that 'out' can be NULL, however, the code
>> does not respect this. NULL 'out' seems rather pointless (unless you
>> want an alias for starts_with()), so presumably the documentation is
>> incorrect.
>
> Thanks for catching. My original version (before I sent to the list) did
> allow for a conditional NULL out, but I realized there was exactly one
> caller who wanted this, and that they would be better off using
> starts_with. I added patch 3 ("avoid using skip_prefix as a boolean")
> and stripped the conditional from skip_prefix, but forgot to update the
> comment.
>
> I think we'd just want to squash the patch below (I also took the
> opportunity to fix a typo and clarify the text a bit):
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 556c839..1187e1a 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -350,8 +350,9 @@ extern int starts_with(const char *str, const char *prefix);
>  extern int ends_with(const char *str, const char *suffix);
>
>  /*
> - * If "str" begins with "prefix", return 1. If out is non-NULL,
> - * it it set to str + strlen(prefix) (i.e., the prefix is skipped).
> + * If the string "str" begins with the string found in "prefix", return 1.
> + * The "out" parameter is set to "str + strlen(prefix)" (i.e., to the point in
> + * the string right after the prefix).
>   *
>   * Otherwise, returns 0 and out is left untouched.

For consistency with the preceding paragraph:

    Otherwise, return 0 and leave "out" untouched.

>   *
>
> -Peff

Re: [PATCH 04/16] refactor skip_prefix to return a boolean

[Jeff King]

On Thu, Jun 19, 2014 at 10:30:36PM -0400, Eric Sunshine wrote:

> > diff --git a/git-compat-util.h b/git-compat-util.h
> > index 556c839..1187e1a 100644
> > --- a/git-compat-util.h
> > +++ b/git-compat-util.h
> > @@ -350,8 +350,9 @@ extern int starts_with(const char *str, const char *prefix);
> >  extern int ends_with(const char *str, const char *suffix);
> >
> >  /*
> > - * If "str" begins with "prefix", return 1. If out is non-NULL,
> > - * it it set to str + strlen(prefix) (i.e., the prefix is skipped).
> > + * If the string "str" begins with the string found in "prefix", return 1.
> > + * The "out" parameter is set to "str + strlen(prefix)" (i.e., to the point in
> > + * the string right after the prefix).
> >   *
> >   * Otherwise, returns 0 and out is left untouched.
> 
> For consistency with the preceding paragraph:
> 
>     Otherwise, return 0 and leave "out" untouched.

Heh, I even noticed the verb tense mismatch but thought "nah, nobody
will care". I see I was wrong. :)

Here is the full squashable patch against v1, in case Junio picks it up
from here (or it will be in my re-roll if necessary).

diff --git a/git-compat-util.h b/git-compat-util.h
index 556c839..d29e1df 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -350,10 +350,11 @@ extern int starts_with(const char *str, const char *prefix);
 extern int ends_with(const char *str, const char *suffix);
 
 /*
- * If "str" begins with "prefix", return 1. If out is non-NULL,
- * it it set to str + strlen(prefix) (i.e., the prefix is skipped).
+ * If the string "str" begins with the string found in "prefix", return 1.
+ * The "out" parameter is set to "str + strlen(prefix)" (i.e., to the point in
+ * the string right after the prefix).
  *
- * Otherwise, returns 0 and out is left untouched.
+ * Otherwise, return 0 and leave "out" untouched.
  *
  * Examples:
  *

Re: [PATCH 10/16] fast-import: use skip_prefix for parsing input

[Eric Sunshine]

On Wed, Jun 18, 2014 at 3:49 PM, Jeff King <peff@peff.net> wrote:
> Fast-import does a lot of parsing of commands and
> dispatching to sub-functions. For example, given "option
> foo", we might recognize "option " using starts_with, and
> then hand it off to parse_option() to do the rest.
>
> However, we do not let parse_option know that we have parsed
> the first part already. It gets the full buffer, and has to
> skip past the uninteresting bits. Some functions simply add
> a magic constant:
>
>   char *option = command_buf.buf + 7;
>
> Others use strlen:
>
>   char *option = command_buf.buf + strlen("option ");
>
> And others use strchr:
>
>   char *option = strchr(command_buf.buf, ' ') + 1;
>
> All of these are brittle and easy to get wrong (especially
> given that the starts_with call and the code that assumes
> the presence of the prefix are far apart). Instead, we can
> use skip_prefix, and just pass each handler a pointer to its
> arguments.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> diff --git a/fast-import.c b/fast-import.c
> index a3ffe30..5f17adb 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -2713,20 +2706,22 @@ static void parse_new_commit(void)
>
>         /* file_change* */
>         while (command_buf.len > 0) {
> -               if (starts_with(command_buf.buf, "M "))
> -                       file_change_m(b);
> -               else if (starts_with(command_buf.buf, "D "))
> -                       file_change_d(b);
> -               else if (starts_with(command_buf.buf, "R "))
> -                       file_change_cr(b, 1);
> -               else if (starts_with(command_buf.buf, "C "))
> -                       file_change_cr(b, 0);
> -               else if (starts_with(command_buf.buf, "N "))
> -                       note_change_n(b, &prev_fanout);
> +               const char *v;

This declaration of 'v' shadows the 'v' added by patch 8/16 earlier in
the function.

> +               if (skip_prefix(command_buf.buf, "M ", &v))
> +                       file_change_m(v, b);
> +               else if (skip_prefix(command_buf.buf, "D ", &v))
> +                       file_change_d(v, b);
> +               else if (skip_prefix(command_buf.buf, "R ", &v))
> +                       file_change_cr(v, b, 1);
> +               else if (skip_prefix(command_buf.buf, "C ", &v))
> +                       file_change_cr(v, b, 0);
> +               else if (skip_prefix(command_buf.buf, "N ", &v))
> +                       note_change_n(v, b, &prev_fanout);
>                 else if (!strcmp("deleteall", command_buf.buf))
>                         file_change_deleteall(b);
> -               else if (starts_with(command_buf.buf, "ls "))
> -                       parse_ls(b);
> +               else if (skip_prefix(command_buf.buf, "ls ", &v))
> +                       parse_ls(v, b);
>                 else {
>                         unread_command_buf = 1;
>                         break;

Re: [PATCH 14/16] fetch-pack: refactor parsing in get_ack

[Eric Sunshine]

On Wed, Jun 18, 2014 at 3:56 PM, Jeff King <peff@peff.net> wrote:
> There are several uses of the magic number "line+45" when
> parsing ACK lines from the server, and it's rather unclear
> why 45 is the correct number. We can make this more clear by
> keeping a running pointer as we parse, using skip_prefix to
> jump past the first "ACK ", then adding 40 to jump past
> get_sha1_hex (which is still magical, but hopefully 40 is
> less magical to readers of git code).
>
> Note that this actually puts us at line+44. The original
> required some character between the sha1 and further ACK
> flags (it is supposed to be a space, but we never enforced
> that). We start our search for flags at line+44, which
> meanas we are slightly more liberal than the old code.

s/meanas/means/

> Signed-off-by: Jeff King <peff@peff.net>

Re: [PATCH 10/16] fast-import: use skip_prefix for parsing input

[Jeff King]

On Thu, Jun 19, 2014 at 11:19:09PM -0400, Eric Sunshine wrote:

> > -               if (starts_with(command_buf.buf, "M "))
> > -                       file_change_m(b);
> > -               else if (starts_with(command_buf.buf, "D "))
> > -                       file_change_d(b);
> > -               else if (starts_with(command_buf.buf, "R "))
> > -                       file_change_cr(b, 1);
> > -               else if (starts_with(command_buf.buf, "C "))
> > -                       file_change_cr(b, 0);
> > -               else if (starts_with(command_buf.buf, "N "))
> > -                       note_change_n(b, &prev_fanout);
> > +               const char *v;
> 
> This declaration of 'v' shadows the 'v' added by patch 8/16 earlier in
> the function.

Thanks.  I reordered the patches before sending, so when this one was
originally written, there was no "v" at the top-level of the function.
I think we can just drop this interior one. The point of the short "v"
is that it can be used as a temporary value for prefix matches, so I
think we can just reuse the same one.

I tried compiling with -Wshadow (which I don't usually do), but we're
not even close to compiling clean there. Some of them are legitimately
confusing (e.g., try figuring out "end" in parse_rev_note). But others
look just annoying (e.g., complaining that a local "usage" conflicts
with the global function). I don't know if we want to put effort into
being -Wshadow clean or not.

-Peff

Re: [PATCH 10/16] fast-import: use skip_prefix for parsing input

[Eric Sunshine]

On Fri, Jun 20, 2014 at 1:45 AM, Jeff King <peff@peff.net> wrote:
> On Thu, Jun 19, 2014 at 11:19:09PM -0400, Eric Sunshine wrote:
>
>> > -               if (starts_with(command_buf.buf, "M "))
>> > -                       file_change_m(b);
>> > -               else if (starts_with(command_buf.buf, "D "))
>> > -                       file_change_d(b);
>> > -               else if (starts_with(command_buf.buf, "R "))
>> > -                       file_change_cr(b, 1);
>> > -               else if (starts_with(command_buf.buf, "C "))
>> > -                       file_change_cr(b, 0);
>> > -               else if (starts_with(command_buf.buf, "N "))
>> > -                       note_change_n(b, &prev_fanout);
>> > +               const char *v;
>>
>> This declaration of 'v' shadows the 'v' added by patch 8/16 earlier in
>> the function.
>
> Thanks.  I reordered the patches before sending, so when this one was
> originally written, there was no "v" at the top-level of the function.
> I think we can just drop this interior one. The point of the short "v"
> is that it can be used as a temporary value for prefix matches, so I
> think we can just reuse the same one.

Agreed. The intended usage of 'v' is clear enough and the code simple
enough that confusion is unlikely.

> I tried compiling with -Wshadow (which I don't usually do), but we're
> not even close to compiling clean there. Some of them are legitimately
> confusing (e.g., try figuring out "end" in parse_rev_note). But others
> look just annoying (e.g., complaining that a local "usage" conflicts
> with the global function). I don't know if we want to put effort into
> being -Wshadow clean or not.

I just happened to notice the shadowing declaration while reading the
patch, but don't feel strongly about existing cases. It makes sense to
clean up confusing cases, such 'end' in parse_rev_note(), when working
on that code (just as with style cleanups), but thus far nobody has
been complaining about existing shadowed variables, so global cleanup
would likely be considered churn.

Re: [PATCH 04/16] refactor skip_prefix to return a boolean

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> diff --git a/builtin/clone.c b/builtin/clone.c
> index b12989d..a5b2d9d 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -703,9 +703,12 @@ 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) {
> +				const char *head = remote_head_points_at->name;
> +				if (!skip_prefix(head, "refs/heads/", &head))
> +					die("BUG: remote HEAD points at non-head?");
> +
>  				strbuf_addf(&value, "+%s:%s%s", remote_head_points_at->name,
> -						branch_top->buf,
> -						skip_prefix(remote_head_points_at->name, "refs/heads/"));
> +						branch_top->buf, head);
>  			}
>  			/*
>  			 * otherwise, the next "git fetch" will

I was re-reading this and noticed another possible bug.

 builtin/clone.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index b12989d..df659dd 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -696,7 +696,7 @@ static void write_refspec_config(const char* src_ref_prefix,
 	if (option_mirror || !option_bare) {
 		if (option_single_branch && !option_mirror) {
 			if (option_branch) {
-				if (strstr(our_head_points_at->name, "refs/tags/"))
+				if (starts_with(our_head_points_at->name, "refs/tags/"))
 					strbuf_addf(&value, "+%s:%s", our_head_points_at->name,
 						our_head_points_at->name);
 				else

Because the pattern is not anchored to the left with a slash, it is
clear that the original cannot even claim that it was trying to
munge "foo/refs/tags/" as well.

Which means this is trivially correct, but at the same time I wonder
what it means for our-head to point at a ref in refs/tags/ hierarchy.

Re: [PATCH 04/16] refactor skip_prefix to return a boolean

[Jeff King]

On Mon, Jun 23, 2014 at 11:50:06AM -0700, Junio C Hamano wrote:

> I was re-reading this and noticed another possible bug.
> 
>  builtin/clone.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/builtin/clone.c b/builtin/clone.c
> index b12989d..df659dd 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -696,7 +696,7 @@ static void write_refspec_config(const char* src_ref_prefix,
>  	if (option_mirror || !option_bare) {
>  		if (option_single_branch && !option_mirror) {
>  			if (option_branch) {
> -				if (strstr(our_head_points_at->name, "refs/tags/"))
> +				if (starts_with(our_head_points_at->name, "refs/tags/"))
>  					strbuf_addf(&value, "+%s:%s", our_head_points_at->name,
>  						our_head_points_at->name);
>  				else
> 
> Because the pattern is not anchored to the left with a slash, it is
> clear that the original cannot even claim that it was trying to
> munge "foo/refs/tags/" as well.

Yeah, the strstr seems very wrong there. Even with the "/", why would
you want to match "refs/heads/refs/tags/"?

> Which means this is trivially correct, but at the same time I wonder
> what it means for our-head to point at a ref in refs/tags/ hierarchy.

I think it is for "git clone --branch=v1.0". We create a refspec pulling
v1.0 to its local tag in that case (as opposed to to something in
"refs/remotes/origin/").  So I really think this does want to be
starts_with.

-Peff

[PATCH] builtin/clone.c: detect a clone starting at a tag correctly

[Junio C Hamano]

31b808a0 (clone --single: limit the fetch refspec to fetched branch,
2012-09-20) tried to see if the given "branch" to follow is actually
a tag at the remote repository by checking with "refs/tags/" but it
incorrectly used strstr(3); it is actively wrong to treat a "branch"
"refs/heads/refs/tags/foo" and use the logic for the "refs/tags/"
ref hierarchy.  What the code really wanted to do is to see if it
starts with "refs/tags/".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

  Jeff King <peff@peff.net> writes:

  >> Because the pattern is not anchored to the left with a slash, it is
  >> clear that the original cannot even claim that it was trying to
  >> munge "foo/refs/tags/" as well.
  >
  > Yeah, the strstr seems very wrong there. Even with the "/", why would
  > you want to match "refs/heads/refs/tags/"?
  >
  >> Which means this is trivially correct, but at the same time I wonder
  >> what it means for our-head to point at a ref in refs/tags/ hierarchy.
  >
  > I think it is for "git clone --branch=v1.0". We create a refspec pulling
  > v1.0 to its local tag in that case (as opposed to to something in
  > "refs/remotes/origin/").  So I really think this does want to be
  > starts_with.

  Thanks; here is what I'm gonna queue.

 builtin/clone.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 9b3c04d..545105a 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -695,7 +695,7 @@ static void write_refspec_config(const char* src_ref_prefix,
 	if (option_mirror || !option_bare) {
 		if (option_single_branch && !option_mirror) {
 			if (option_branch) {
-				if (strstr(our_head_points_at->name, "refs/tags/"))
+				if (starts_with(our_head_points_at->name, "refs/tags/"))
 					strbuf_addf(&value, "+%s:%s", our_head_points_at->name,
 						our_head_points_at->name);
 				else
-- 
2.0.0-637-g8ac8cc9

Re: [PATCH 08/16] use skip_prefix to avoid magic numbers

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> diff --git a/connect.c b/connect.c
> index 94a6650..37ff018 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -140,12 +141,12 @@ 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 (len > 4 && skip_prefix(buffer, "ERR ", &arg))
> +			die("remote error: %s", arg);

Makes one wonder if we should do something special to a line with
only "ERR " and nothing else on it, which the other end may have
meant us to give a blank line to make the output more readable.

A fix, if one turns out to be needed, is outside the scope of this
patch, though, I think.

Re: [PATCH 08/16] use skip_prefix to avoid magic numbers

[Jeff King]

On Mon, Jun 23, 2014 at 02:44:23PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > diff --git a/connect.c b/connect.c
> > index 94a6650..37ff018 100644
> > --- a/connect.c
> > +++ b/connect.c
> > @@ -140,12 +141,12 @@ 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 (len > 4 && skip_prefix(buffer, "ERR ", &arg))
> > +			die("remote error: %s", arg);
> 
> Makes one wonder if we should do something special to a line with
> only "ERR " and nothing else on it, which the other end may have
> meant us to give a blank line to make the output more readable.

I don't think that would buy us much. We have always accepted only a
single ERR line and died immediately. So any changes of that nature
would have to be made in the client, and then servers would have to wait
N time units before it was safe to start using the feature (otherwise
old clients just get the blank line!).

I also don't think blank lines by themselves are useful. You'd want them
in addition to being able to handle multiple lines. So a nicer fix is
more along the lines of "accept multiple ERR lines, including blank
lines, followed by a terminating line ("ERRDONE" or something).

Then servers can do:

  ERR unable to access foo.git: Printer on fire
  ERR
  ERR You may have misspelled the repository name. Did you mean:
  ERR
  ERR  foobar.git
  ERRDONE

Old clients would see the first line and die. Newer clients would print
the helpful hint. Servers would just need to make sure that the first
line stands on its own to cover both cases.

> A fix, if one turns out to be needed, is outside the scope of this
> patch, though, I think.

Yeah, definitely a separate topic.

It is not something I think anybody has asked for, but I can imagine a
site like GitHub making use of it (we already show custom errors for
http, but there's no room beyond the single ERR line). And teaching the
clients now expands the options for servers later. So it might be worth
doing just as a potential feature.

-Peff