{"thread":{"id":"36953","subject":"[PATCH 0/16] skip_prefix refactoring and cleanups","startedAt":"2014-06-18T19:41:18Z","lastAt":"2014-07-01T17:35:02Z","messageCount":33,"participants":["Jeff King","Tanay Abhra","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":16},"messages":[{"id":"244561","messageId":"20140618194117.GA22269@sigill.intra.peff.net","threadId":"36953","inReplyTo":null,"subject":"[PATCH 0/16] skip_prefix refactoring and cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-18T19:41:18Z","receivedAt":"2014-06-18T19:41:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"A while ago[1] we discussed refactoring skip_prefix (or adding something\nlike it) to make it more natural to call. This morning I decided to take\na look at doing this, and went down a rabbit hole of cleanups. This is\npart one of the result.\n\nThe short of it is that skip_prefix can now be used like this:\n\n if (skip_prefix(arg, \"--foo=\", &value)\n\thandle_foo(value);\n\nor even:\n\n /* arg remains valid if we didn't match! */\n if (skip_prefix(arg, \"--foo=\", &arg))\n\thandle_foo(arg);\n else if (skip_prefix(arg, \"--bar\", &arg))\n\thandle_bar(arg);\n\nthough the latter form is not always useful if the conditional code\nwants to see all of \"arg\".\n\n [01/16]: parse_diff_color_slot: drop ofs parameter\n [02/16]: daemon: mark some strings as const\n [03/16]: avoid using skip_prefix as a boolean\n\n These ones are preparatory cleanup.\n\n [04/16]: refactor skip_prefix to return a boolean\n\n The actual refactoring; it changes the existing callers[2] at the\n same time.\n\n [05/16]: apply: use skip_prefix instead of raw addition\n [06/16]: fast-import: fix read of uninitialized argv memory\n [07/16]: transport-helper: avoid reading past end-of-string\n\n These three are conversions that actually fix bugs.\n\n [08/16]: use skip_prefix to avoid magic numbers\n [09/16]: use skip_prefix to avoid repeating strings\n\n These are the straightforward conversions lumped together by the\n problem they are solving.\n\n [10/16]: fast-import: use skip_prefix for parsing input\n [11/16]: daemon: use skip_prefix to avoid magic numbers\n [12/16]: stat_opt: check extra strlen call\n [13/16]: fast-import: refactor parsing of spaces\n [14/16]: fetch-pack: refactor parsing in get_ack\n [15/16]: git: avoid magic number with skip_prefix\n\n These ones are variants of the above two that needed extra\n discussion or attention for various reasons.\n\n [16/16]: use skip_prefix to avoid repeated calculations\n\n These ones don't solve a specific problem as the patches above do,\n but I think the code ends up more readable.\n\nMy conversions are by now means exhaustive. After grepping through all\nof the starts_with up to about http.c, I decided to call it a day. But\nwe can easily convert more as time goes on.\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/239438/focus=239564\n\n I started from scratch this morning, oblivious to the fact that René\n posted something very similar to patch 4 in that thread.\n\n[2] I test-merged with 'pu'. There is a minor textual conflict with\n jk/commit-buffer-length that should be easy to resolve. There's also\n one new caller of skip_prefix added in cc/interpret-trailers. It\n needs this fix when merged:\n\ndiff --git a/trailer.c b/trailer.c\nindex eaf692b..987fa29 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -377,8 +377,7 @@ static int git_trailer_config(const char *conf_key, const char *value, void *cb)\n \tenum trailer_info_type type;\n \tint i;\n \n-\ttrailer_item = skip_prefix(conf_key, \"trailer.\");\n-\tif (!trailer_item)\n+\tif (!skip_prefix(conf_key, \"trailer.\", &trailer_item))\n \t\treturn 0;\n \n \tvariable_name = strrchr(trailer_item, '.');\n"},{"id":"244562","messageId":"20140618194149.GA22622@sigill.intra.peff.net","threadId":"36953","inReplyTo":"20140618194117.GA22269@sigill.intra.peff.net","subject":"[PATCH 01/16] parse_diff_color_slot: drop ofs parameter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-18T19:41:50Z","receivedAt":"2014-06-18T19:41:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This function originally took a whole config variable name\n(\"var\") and an offset (\"ofs\"). It checked \"var+ofs\" against\neach color slot, but reported errors using the whole \"var\".\n\nHowever, since 8b8e862 (ignore unknown color configuration,\n2009-12-12), it returns -1 rather than printing its own\nerror, and therefore only cares about var+ofs. We can drop\nthe ofs parameter and teach its sole caller to derive the\npointer itself.\n\nSigned-off-by: Jeff King \n---\n diff.c | 20 ++++++++++----------\n 1 file changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex bba9a55..77c5eb4 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -52,23 +52,23 @@ static char diff_colors[][COLOR_MAXLEN] = {\n \tGIT_COLOR_NORMAL,\t/* FUNCINFO */\n };\n \n-static int parse_diff_color_slot(const char *var, int ofs)\n+static int parse_diff_color_slot(const char *var)\n {\n-\tif (!strcasecmp(var+ofs, \"plain\"))\n+\tif (!strcasecmp(var, \"plain\"))\n \t\treturn DIFF_PLAIN;\n-\tif (!strcasecmp(var+ofs, \"meta\"))\n+\tif (!strcasecmp(var, \"meta\"))\n \t\treturn DIFF_METAINFO;\n-\tif (!strcasecmp(var+ofs, \"frag\"))\n+\tif (!strcasecmp(var, \"frag\"))\n \t\treturn DIFF_FRAGINFO;\n-\tif (!strcasecmp(var+ofs, \"old\"))\n+\tif (!strcasecmp(var, \"old\"))\n \t\treturn DIFF_FILE_OLD;\n-\tif (!strcasecmp(var+ofs, \"new\"))\n+\tif (!strcasecmp(var, \"new\"))\n \t\treturn DIFF_FILE_NEW;\n-\tif (!strcasecmp(var+ofs, \"commit\"))\n+\tif (!strcasecmp(var, \"commit\"))\n \t\treturn DIFF_COMMIT;\n-\tif (!strcasecmp(var+ofs, \"whitespace\"))\n+\tif (!strcasecmp(var, \"whitespace\"))\n \t\treturn DIFF_WHITESPACE;\n-\tif (!strcasecmp(var+ofs, \"func\"))\n+\tif (!strcasecmp(var, \"func\"))\n \t\treturn DIFF_FUNCINFO;\n \treturn -1;\n }\n@@ -240,7 +240,7 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)\n \t\treturn -1;\n \n \tif (starts_with(var, \"diff.color.\") || starts_with(var, \"color.diff.\")) {\n-\t\tint slot = parse_diff_color_slot(var, 11);\n+\t\tint slot = parse_diff_color_slot(var + 11);\n \t\tif (slot < 0)\n \t\t\treturn 0;\n \t\tif (!value)\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244563","messageId":"20140618194158.GB22622@sigill.intra.peff.net","threadId":"36953","inReplyTo":"20140618194117.GA22269@sigill.intra.peff.net","subject":"[PATCH 02/16] daemon: mark some strings as const","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-18T19:41:58Z","receivedAt":"2014-06-18T19:41:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"None of these strings is modified; marking them as const\nwill help later refactoring.\n\nSigned-off-by: Jeff King \n---\n daemon.c | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex f9c63e9..18818c3 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -39,8 +39,8 @@ static int strict_paths;\n static int export_all_trees;\n \n /* Take all paths relative to this one if non-NULL */\n-static char *base_path;\n-static char *interpolated_path;\n+static const char *base_path;\n+static const char *interpolated_path;\n static int base_path_relaxed;\n \n /* Flag indicating client sent extra args. */\n@@ -106,12 +106,12 @@ static void NORETURN daemon_die(const char *err, va_list params)\n \texit(1);\n }\n \n-static const char *path_ok(char *directory)\n+static const char *path_ok(const char *directory)\n {\n \tstatic char rpath[PATH_MAX];\n \tstatic char interp_path[PATH_MAX];\n \tconst char *path;\n-\tchar *dir;\n+\tconst char *dir;\n \n \tdir = directory;\n \n@@ -131,7 +131,7 @@ static const char *path_ok(char *directory)\n \t\t\t * \"~alice/%s/foo\".\n \t\t\t */\n \t\t\tint namlen, restlen = strlen(dir);\n-\t\t\tchar *slash = strchr(dir, '/');\n+\t\t\tconst char *slash = strchr(dir, '/');\n \t\t\tif (!slash)\n \t\t\t\tslash = dir + restlen;\n \t\t\tnamlen = slash - dir;\n@@ -253,7 +253,7 @@ static int daemon_error(const char *dir, const char *msg)\n \treturn -1;\n }\n \n-static char *access_hook;\n+static const char *access_hook;\n \n static int run_access_hook(struct daemon_service *service, const char *dir, const char *path)\n {\n@@ -318,7 +318,7 @@ error_return:\n \treturn -1;\n }\n \n-static int run_service(char *dir, struct daemon_service *service)\n+static int run_service(const char *dir, struct daemon_service *service)\n {\n \tconst char *path;\n \tint enabled = service->enabled;\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244564","messageId":"20140618194214.GC22622@sigill.intra.peff.net","threadId":"36953","inReplyTo":"20140618194117.GA22269@sigill.intra.peff.net","subject":"[PATCH 03/16] avoid using skip_prefix as a boolean","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-18T19:42:14Z","receivedAt":"2014-06-18T19:42:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There's no point in using:\n\n if (skip_prefix(buf, \"foo\"))\n\nover\n\n if (starts_with(buf, \"foo\"))\n\nas the point of skip_prefix is to return a pointer to the\ndata after the prefix. Using starts_with is more readable,\nand will make refactoring skip_prefix easier.\n\nSigned-off-by: Jeff King \n---\n builtin/fmt-merge-msg.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c\nindex 3906eda..72378e6 100644\n--- a/builtin/fmt-merge-msg.c\n+++ b/builtin/fmt-merge-msg.c\n@@ -298,7 +298,7 @@ static void credit_people(struct strbuf *out,\n \t (them->nr == 1 &&\n \t me &&\n \t (me = skip_prefix(me, them->items->string)) != NULL &&\n-\t skip_prefix(me, \" <\")))\n+\t starts_with(me, \" <\")))\n \t\treturn;\n \tstrbuf_addf(out, \"\\n%c %s \", comment_line_char, label);\n \tadd_people_count(out, them);\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244565","messageId":"20140618194417.GD22622@sigill.intra.peff.net","threadId":"36953","inReplyTo":"20140618194117.GA22269@sigill.intra.peff.net","subject":"[PATCH 04/16] refactor skip_prefix to return a boolean","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-18T19:44:19Z","receivedAt":"2014-06-18T19:44:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The skip_prefix function returns a pointer to the content\npast the prefix, or NULL if the prefix was not found. While\nthis is nice and simple, in practice it makes it hard to use\nfor two reasons:\n\n 1. When you want to conditionally skip or keep the string\n as-is, you have to introduce a second temporary\n variable. For example:\n\n tmp = skip_prefix(buf, \"foo\");\n if (tmp)\n\t buf = tmp;\n\n 2. It is verbose to check the outcome in a conditional, as\n you need extra parentheses to silence compiler\n warnings. For example:\n\n if ((cp = skip_prefix(buf, \"foo\"))\n\t /* do something with cp */\n\nBoth of these make it harder to use for long if-chains, and\nwe tend to use starts_with instead. However, the first line\nof \"do something\" is often to then skip forward in buf past\nthe prefix, either using a magic constant or with an extra\nstrlen (which is generally computed at compile time, but\nmeans we are repeating ourselves).\n\nThis patch refactors skip_prefix to return a simple boolean,\nand to provide the pointer value as an out-parameter. If the\nprefix is not found, the out-parameter is untouched. This\nlets you write:\n\n if (skip_prefix(arg, \"foo \", &arg))\n\t do_foo(arg);\n else if (skip_prefix(arg, \"bar \", &arg))\n\t do_bar(arg);\n\nSigned-off-by: Jeff King \n---\nThe diffstat is misleading. We actually save lines, except that I added\ndocumentation for the function. :)\n\n advice.c | 5 ++++-\n branch.c | 4 ++--\n builtin/branch.c | 6 +++---\n builtin/clone.c | 11 +++++++----\n builtin/commit.c | 5 ++---\n builtin/fmt-merge-msg.c | 2 +-\n builtin/push.c | 7 +++----\n builtin/remote.c | 4 +---\n column.c | 5 +++--\n commit.c | 6 ++----\n config.c | 3 +--\n credential-cache--daemon.c | 6 ++----\n credential.c | 3 +--\n fsck.c | 14 +++++---------\n git-compat-util.h | 26 ++++++++++++++++++++++----\n parse-options.c | 16 +++++++++-------\n transport.c | 4 +++-\n urlmatch.c | 3 +--\n 18 files changed, 72 insertions(+), 58 deletions(-)\n\ndiff --git a/advice.c b/advice.c\nindex c50ebdf..9b42033 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -61,9 +61,12 @@ void advise(const char *advice, ...)\n \n int git_default_advice_config(const char *var, const char *value)\n {\n-\tconst char *k = skip_prefix(var, \"advice.\");\n+\tconst char *k;\n \tint i;\n \n+\tif (!skip_prefix(var, \"advice.\", &k))\n+\t\treturn 0;\n+\n \tfor (i = 0; i < ARRAY_SIZE(advice_config); i++) {\n \t\tif (strcmp(k, advice_config[i].name))\n \t\t\tcontinue;\ndiff --git a/branch.c b/branch.c\nindex 660097b..46e8aa8 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -50,11 +50,11 @@ static int should_setup_rebase(const char *origin)\n \n void install_branch_config(int flag, const char *local, const char *origin, const char *remote)\n {\n-\tconst char *shortname = skip_prefix(remote, \"refs/heads/\");\n+\tconst char *shortname = NULL;\n \tstruct strbuf key = STRBUF_INIT;\n \tint rebasing = should_setup_rebase(origin);\n \n-\tif (shortname\n+\tif (skip_prefix(remote, \"refs/heads/\", &shortname)\n \t && !strcmp(local, shortname)\n \t && !origin) {\n \t\twarning(_(\"Not setting branch %s as its own upstream.\"),\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 652b1d2..0591b22 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -294,13 +294,13 @@ static char *resolve_symref(const char *src, const char *prefix)\n {\n \tunsigned char sha1[20];\n \tint flag;\n-\tconst char *dst, *cp;\n+\tconst char *dst;\n \n \tdst = resolve_ref_unsafe(src, sha1, 0, &flag);\n \tif (!(dst && (flag & REF_ISSYMREF)))\n \t\treturn NULL;\n-\tif (prefix && (cp = skip_prefix(dst, prefix)))\n-\t\tdst = cp;\n+\tif (prefix)\n+\t\tskip_prefix(dst, prefix, &dst);\n \treturn xstrdup(dst);\n }\n \ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex b12989d..a5b2d9d 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -584,11 +584,11 @@ static void update_remote_refs(const struct ref *refs,\n static void update_head(const struct ref *our, const struct ref *remote,\n \t\t\tconst char *msg)\n {\n-\tif (our && starts_with(our->name, \"refs/heads/\")) {\n+\tconst char *head;\n+\tif (our && skip_prefix(our->name, \"refs/heads/\", &head)) {\n \t\t/* Local default branch link */\n \t\tcreate_symref(\"HEAD\", our->name, NULL);\n \t\tif (!option_bare) {\n-\t\t\tconst char *head = skip_prefix(our->name, \"refs/heads/\");\n \t\t\tupdate_ref(msg, \"HEAD\", our->old_sha1, NULL, 0,\n \t\t\t\t UPDATE_REFS_DIE_ON_ERR);\n \t\t\tinstall_branch_config(0, head, option_origin, our->name);\n@@ -703,9 +703,12 @@ static void write_refspec_config(const char* src_ref_prefix,\n \t\t\t\t\tstrbuf_addf(&value, \"+%s:%s%s\", our_head_points_at->name,\n \t\t\t\t\t\tbranch_top->buf, option_branch);\n \t\t\t} else if (remote_head_points_at) {\n+\t\t\t\tconst char *head = remote_head_points_at->name;\n+\t\t\t\tif (!skip_prefix(head, \"refs/heads/\", &head))\n+\t\t\t\t\tdie(\"BUG: remote HEAD points at non-head?\");\n+\n \t\t\t\tstrbuf_addf(&value, \"+%s:%s%s\", remote_head_points_at->name,\n-\t\t\t\t\t\tbranch_top->buf,\n-\t\t\t\t\t\tskip_prefix(remote_head_points_at->name, \"refs/heads/\"));\n+\t\t\t\t\t\tbranch_top->buf, head);\n \t\t\t}\n \t\t\t/*\n \t\t\t * otherwise, the next \"git fetch\" will\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 5e2221c..ec75341 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1020,7 +1020,7 @@ static int message_is_empty(struct strbuf *sb)\n static int template_untouched(struct strbuf *sb)\n {\n \tstruct strbuf tmpl = STRBUF_INIT;\n-\tchar *start;\n+\tconst char *start;\n \n \tif (cleanup_mode == CLEANUP_NONE && sb->len)\n \t\treturn 0;\n@@ -1029,8 +1029,7 @@ static int template_untouched(struct strbuf *sb)\n \t\treturn 0;\n \n \tstripspace(&tmpl, cleanup_mode == CLEANUP_ALL);\n-\tstart = (char *)skip_prefix(sb->buf, tmpl.buf);\n-\tif (!start)\n+\tif (!skip_prefix(sb->buf, tmpl.buf, &start))\n \t\tstart = sb->buf;\n \tstrbuf_release(&tmpl);\n \treturn rest_is_empty(sb, start - sb->buf);\ndiff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c\nindex 72378e6..3c19241 100644\n--- a/builtin/fmt-merge-msg.c\n+++ b/builtin/fmt-merge-msg.c\n@@ -297,7 +297,7 @@ static void credit_people(struct strbuf *out,\n \tif (!them->nr ||\n \t (them->nr == 1 &&\n \t me &&\n-\t (me = skip_prefix(me, them->items->string)) != NULL &&\n+\t skip_prefix(me, them->items->string, &me) &&\n \t starts_with(me, \" <\")))\n \t\treturn;\n \tstrbuf_addf(out, \"\\n%c %s \", comment_line_char, label);\ndiff --git a/builtin/push.c b/builtin/push.c\nindex f8dfea4..f50e3d5 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -127,11 +127,10 @@ static NORETURN int die_push_simple(struct branch *branch, struct remote *remote\n \t * them the big ugly fully qualified ref.\n \t */\n \tconst char *advice_maybe = \"\";\n-\tconst char *short_upstream =\n-\t\tskip_prefix(branch->merge[0]->src, \"refs/heads/\");\n+\tconst char *short_upstream = branch->merge[0]->src;\n+\n+\tskip_prefix(short_upstream, \"refs/heads/\", &short_upstream);\n \n-\tif (!short_upstream)\n-\t\tshort_upstream = branch->merge[0]->src;\n \t/*\n \t * Don't show advice for people who explicitly set\n \t * push.default.\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex c9102e8..a8efe3d 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -250,9 +250,7 @@ static struct string_list branch_list;\n \n static const char *abbrev_ref(const char *name, const char *prefix)\n {\n-\tconst char *abbrev = skip_prefix(name, prefix);\n-\tif (abbrev)\n-\t\treturn abbrev;\n+\tskip_prefix(name, prefix, &name);\n \treturn name;\n }\n #define abbrev_branch(name) abbrev_ref((name), \"refs/heads/\")\ndiff --git a/column.c b/column.c\nindex 1a468de..ca878bc 100644\n--- a/column.c\n+++ b/column.c\n@@ -336,8 +336,9 @@ static int column_config(const char *var, const char *value,\n int git_column_config(const char *var, const char *value,\n \t\t const char *command, unsigned int *colopts)\n {\n-\tconst char *it = skip_prefix(var, \"column.\");\n-\tif (!it)\n+\tconst char *it;\n+\n+\tif (!skip_prefix(var, \"column.\", &it))\n \t\treturn 0;\n \n \tif (!strcmp(it, \"ui\"))\ndiff --git a/commit.c b/commit.c\nindex 881be3b..dfc0eb0 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -556,8 +556,7 @@ static void record_author_date(struct author_date_slab *author_date,\n \t buf;\n \t buf = line_end + 1) {\n \t\tline_end = strchrnul(buf, '\\n');\n-\t\tident_line = skip_prefix(buf, \"author \");\n-\t\tif (!ident_line) {\n+\t\tif (!skip_prefix(buf, \"author \", &ident_line)) {\n \t\t\tif (!line_end[0] || line_end[1] == '\\n')\n \t\t\t\treturn; /* end of header */\n \t\t\tcontinue;\n@@ -1183,8 +1182,7 @@ static void parse_gpg_output(struct signature_check *sigc)\n \tfor (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {\n \t\tconst char *found, *next;\n \n-\t\tfound = skip_prefix(buf, sigcheck_gpg_status[i].check + 1);\n-\t\tif (!found) {\n+\t\tif (!skip_prefix(buf, sigcheck_gpg_status[i].check + 1, &found)) {\n \t\t\tfound = strstr(buf, sigcheck_gpg_status[i].check);\n \t\t\tif (!found)\n \t\t\t\tcontinue;\ndiff --git a/config.c b/config.c\nindex a1aef1c..ba882a1 100644\n--- a/config.c\n+++ b/config.c\n@@ -138,8 +138,7 @@ int git_config_include(const char *var, const char *value, void *data)\n \tif (ret < 0)\n \t\treturn ret;\n \n-\ttype = skip_prefix(var, \"include.\");\n-\tif (!type)\n+\tif (!skip_prefix(var, \"include.\", &type))\n \t\treturn ret;\n \n \tif (!strcmp(type, \"path\"))\ndiff --git a/credential-cache--daemon.c b/credential-cache--daemon.c\nindex 390f194..3b370ca 100644\n--- a/credential-cache--daemon.c\n+++ b/credential-cache--daemon.c\n@@ -109,14 +109,12 @@ static int read_request(FILE *fh, struct credential *c,\n \tconst char *p;\n \n \tstrbuf_getline(&item, fh, '\\n');\n-\tp = skip_prefix(item.buf, \"action=\");\n-\tif (!p)\n+\tif (!skip_prefix(item.buf, \"action=\", &p))\n \t\treturn error(\"client sent bogus action line: %s\", item.buf);\n \tstrbuf_addstr(action, p);\n \n \tstrbuf_getline(&item, fh, '\\n');\n-\tp = skip_prefix(item.buf, \"timeout=\");\n-\tif (!p)\n+\tif (!skip_prefix(item.buf, \"timeout=\", &p))\n \t\treturn error(\"client sent bogus timeout line: %s\", item.buf);\n \t*timeout = atoi(p);\n \ndiff --git a/credential.c b/credential.c\nindex e54753c..4d79d32 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -40,8 +40,7 @@ static int credential_config_callback(const char *var, const char *value,\n \tstruct credential *c = data;\n \tconst char *key, *dot;\n \n-\tkey = skip_prefix(var, \"credential.\");\n-\tif (!key)\n+\tif (!skip_prefix(var, \"credential.\", &key))\n \t\treturn 0;\n \n \tif (!value)\ndiff --git a/fsck.c b/fsck.c\nindex abed62b..bdbea2b 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -278,20 +278,18 @@ static int fsck_ident(const char **ident, struct object *obj, fsck_error error_f\n \n static int fsck_commit(struct commit *commit, fsck_error error_func)\n {\n-\tconst char *buffer = commit->buffer, *tmp;\n+\tconst char *buffer = commit->buffer;\n \tunsigned char tree_sha1[20], sha1[20];\n \tstruct commit_graft *graft;\n \tint parents = 0;\n \tint err;\n \n-\tbuffer = skip_prefix(buffer, \"tree \");\n-\tif (!buffer)\n+\tif (!skip_prefix(buffer, \"tree \", &buffer))\n \t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid format - expected 'tree' line\");\n \tif (get_sha1_hex(buffer, tree_sha1) || buffer[40] != '\\n')\n \t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid 'tree' line format - bad sha1\");\n \tbuffer += 41;\n-\twhile ((tmp = skip_prefix(buffer, \"parent \"))) {\n-\t\tbuffer = tmp;\n+\twhile (skip_prefix(buffer, \"parent \", &buffer)) {\n \t\tif (get_sha1_hex(buffer, sha1) || buffer[40] != '\\n')\n \t\t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid 'parent' line format - bad sha1\");\n \t\tbuffer += 41;\n@@ -318,14 +316,12 @@ static int fsck_commit(struct commit *commit, fsck_error error_func)\n \t\tif (p || parents)\n \t\t\treturn error_func(&commit->object, FSCK_ERROR, \"parent objects missing\");\n \t}\n-\tbuffer = skip_prefix(buffer, \"author \");\n-\tif (!buffer)\n+\tif (!skip_prefix(buffer, \"author \", &buffer))\n \t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid format - expected 'author' line\");\n \terr = fsck_ident(&buffer, &commit->object, error_func);\n \tif (err)\n \t\treturn err;\n-\tbuffer = skip_prefix(buffer, \"committer \");\n-\tif (!buffer)\n+\tif (!skip_prefix(buffer, \"committer \", &buffer))\n \t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid format - expected 'committer' line\");\n \terr = fsck_ident(&buffer, &commit->object, error_func);\n \tif (err)\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex b6f03b3..556c839 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -349,13 +349,31 @@ extern void set_die_is_recursing_routine(int (*routine)(void));\n extern int starts_with(const char *str, const char *prefix);\n extern int ends_with(const char *str, const char *suffix);\n \n-static inline const char *skip_prefix(const char *str, const char *prefix)\n+/*\n+ * If \"str\" begins with \"prefix\", return 1. If out is non-NULL,\n+ * it it set to str + strlen(prefix) (i.e., the prefix is skipped).\n+ *\n+ * Otherwise, returns 0 and out is left untouched.\n+ *\n+ * Examples:\n+ *\n+ * [extract branch name, fail if not a branch]\n+ * if (!skip_prefix(ref, \"refs/heads/\", &branch)\n+ *\treturn -1;\n+ *\n+ * [skip prefix if present, otherwise use whole string]\n+ * skip_prefix(name, \"refs/heads/\", &name);\n+ */\n+static inline int skip_prefix(const char *str, const char *prefix,\n+\t\t\t const char **out)\n {\n \tdo {\n-\t\tif (!*prefix)\n-\t\t\treturn str;\n+\t\tif (!*prefix) {\n+\t\t\t*out = str;\n+\t\t\treturn 1;\n+\t\t}\n \t} while (*str++ == *prefix++);\n-\treturn NULL;\n+\treturn 0;\n }\n \n #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)\ndiff --git a/parse-options.c b/parse-options.c\nindex b536896..e7dafa8 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -231,7 +231,8 @@ static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg,\n \t\t\tcontinue;\n \n again:\n-\t\trest = skip_prefix(arg, long_name);\n+\t\tif (!skip_prefix(arg, long_name, &rest))\n+\t\t\trest = NULL;\n \t\tif (options->type == OPTION_ARGUMENT) {\n \t\t\tif (!rest)\n \t\t\t\tcontinue;\n@@ -280,12 +281,13 @@ is_abbreviated:\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tflags |= OPT_UNSET;\n-\t\t\trest = skip_prefix(arg + 3, long_name);\n-\t\t\t/* abbreviated and negated? */\n-\t\t\tif (!rest && starts_with(long_name, arg + 3))\n-\t\t\t\tgoto is_abbreviated;\n-\t\t\tif (!rest)\n-\t\t\t\tcontinue;\n+\t\t\tif (!skip_prefix(arg + 3, long_name, &rest)) {\n+\t\t\t\t/* abbreviated and negated? */\n+\t\t\t\tif (starts_with(long_name, arg + 3))\n+\t\t\t\t\tgoto is_abbreviated;\n+\t\t\t\telse\n+\t\t\t\t\tcontinue;\n+\t\t\t}\n \t\t}\n \t\tif (*rest) {\n \t\t\tif (*rest != '=')\ndiff --git a/transport.c b/transport.c\nindex 325f03e..59c9727 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -192,7 +192,9 @@ static void set_upstreams(struct transport *transport, struct ref *refs,\n \n static const char *rsync_url(const char *url)\n {\n-\treturn !starts_with(url, \"rsync://\") ? skip_prefix(url, \"rsync:\") : url;\n+\tif (!starts_with(url, \"rsync://\"))\n+\t\tskip_prefix(url, \"rsync:\", &url);\n+\treturn url;\n }\n \n static struct ref *get_refs_via_rsync(struct transport *transport, int for_push)\ndiff --git a/urlmatch.c b/urlmatch.c\nindex ec87cba..3d4c54b 100644\n--- a/urlmatch.c\n+++ b/urlmatch.c\n@@ -483,8 +483,7 @@ int urlmatch_config_entry(const char *var, const char *value, void *cb)\n \tint user_matched = 0;\n \tint retval;\n \n-\tkey = skip_prefix(var, collect->section);\n-\tif (!key || *(key++) != '.') {\n+\tif (!skip_prefix(var, collect->section, &key) || *(key++) != '.') {\n \t\tif (collect->cascade_fn)\n \t\t\treturn collect->cascade_fn(var, value, cb);\n \t\treturn 0; /* not interested */\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244566","messageId":"20140618194534.GE22622@sigill.intra.peff.net","threadId":"36953","inReplyTo":"20140618194117.GA22269@sigill.intra.peff.net","subject":"[PATCH 05/16] apply: use skip_prefix instead of raw addition","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-18T19:45:34Z","receivedAt":"2014-06-18T19:45:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"A submodule diff generally has content like:\n\n -Subproject commit [0-9a-f]{40}\n +Subproject commit [0-9a-f]{40}\n\nWhen we are using \"git apply --index\" with a submodule, we\nfirst apply the textual diff, and then parse that result to\nfigure out the new sha1.\n\nIf the diff has bogus input like:\n\n -Subproject commit 1234567890123456789012345678901234567890\n +bogus\n\nwe will parse the \"bogus\" portion. Our parser assumes that\nthe buffer starts with \"Subproject commit\", and blindly\nskips past it using strlen(). This can cause us to read\nrandom memory after the buffer.\n\nThis problem was unlikely to have come up in practice (since\nit requires a malformed diff), and even when it did, we\nlikely noticed the problem anyway as the next operation was\nto call get_sha1_hex on the random memory.\n\nHowever, we can easily fix it by using skip_prefix to notice\nthe parsing error.\n\nSigned-off-by: Jeff King \n---\n builtin/apply.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 9c5724e..bc924ab 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -3847,9 +3847,10 @@ static void add_index_file(const char *path, unsigned mode, void *buf, unsigned\n \tce->ce_flags = create_ce_flags(0);\n \tce->ce_namelen = namelen;\n \tif (S_ISGITLINK(mode)) {\n-\t\tconst char *s = buf;\n+\t\tconst char *s;\n \n-\t\tif (get_sha1_hex(s + strlen(\"Subproject commit \"), ce->sha1))\n+\t\tif (!skip_prefix(buf, \"Subproject commit \", &s) ||\n+\t\t get_sha1_hex(s, ce->sha1))\n \t\t\tdie(_(\"corrupt patch for submodule %s\"), path);\n \t} else {\n \t\tif (!cached) {\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244567","messageId":"20140618194615.GF22622@sigill.intra.peff.net","threadId":"36953","inReplyTo":"20140618194117.GA22269@sigill.intra.peff.net","subject":"[PATCH 06/16] fast-import: fix read of uninitialized argv memory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-18T19:46:15Z","receivedAt":"2014-06-18T19:46:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Fast-import shares code between its command-line parser and\nthe \"option\" command. To do so, it strips the \"--\" from any\ncommand-line options and passes them to the option parser.\nHowever, it does not confirm that the option even begins\nwith \"--\" before blindly passing \"arg + 2\".\n\nIt does confirm that the option starts with \"-\", so the only\naffected case was:\n\n git fast-import -\n\nwhich would read uninitialized memory after the argument. We\ncan fix it by using skip_prefix and checking the result. As\na bonus, this gets rid of some magic numbers.\n\nSigned-off-by: Jeff King \n---\n fast-import.c | 13 ++++++++-----\n 1 file changed, 8 insertions(+), 5 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 6707a66..b2030cc 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -3342,18 +3342,21 @@ static void parse_argv(void)\n \t\tif (*a != '-' || !strcmp(a, \"--\"))\n \t\t\tbreak;\n \n-\t\tif (parse_one_option(a + 2))\n+\t\tif (!skip_prefix(a, \"--\", &a))\n+\t\t\tdie(\"unknown option %s\", a);\n+\n+\t\tif (parse_one_option(a))\n \t\t\tcontinue;\n \n-\t\tif (parse_one_feature(a + 2, 0))\n+\t\tif (parse_one_feature(a, 0))\n \t\t\tcontinue;\n \n-\t\tif (starts_with(a + 2, \"cat-blob-fd=\")) {\n-\t\t\toption_cat_blob_fd(a + 2 + strlen(\"cat-blob-fd=\"));\n+\t\tif (skip_prefix(a, \"cat-blob-fd=\", &a)) {\n+\t\t\toption_cat_blob_fd(a);\n \t\t\tcontinue;\n \t\t}\n \n-\t\tdie(\"unknown option %s\", a);\n+\t\tdie(\"unknown option --%s\", a);\n \t}\n \tif (i != global_argc)\n \t\tusage(fast_import_usage);\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244568","messageId":"20140618194717.GG22622@sigill.intra.peff.net","threadId":"36953","inReplyTo":"20140618194117.GA22269@sigill.intra.peff.net","subject":"[PATCH 07/16] transport-helper: avoid reading past end-of-string","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-18T19:47:17Z","receivedAt":"2014-06-18T19:47:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We detect the \"import-marks\" capability by looking for that\nstring, but _without_ a trailing space. Then we skip past it\nusing strlen(\"import-marks \"), with a space. So if a remote\nhelper gives us exactly \"import-marks\", we will read past\nthe end-of-string by one character.\n\nThis is unlikely to be a problem in practice, because such\ninput is malformed in the first place, and because there is\na good chance that the string has an extra NUL terminator\none character after the original (because it formerly had a\nnewline in it that we parsed off).\n\nWe can fix it by using skip_prefix with \"import-marks \",\nwith the space. The other form appears to be a typo from\na515ebe (transport-helper: implement marks location as\ncapability, 2011-07-16); \"import-marks\" has never existed\nwithout an argument, and it should match the \"export-marks\"\ndefinition above.\n\nSpeaking of which, we can also use skip_prefix in a few\nother places while we are in the function.\n\nSigned-off-by: Jeff King \n---\n transport-helper.c | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 84c616f..3d8fe7d 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -153,7 +153,7 @@ static struct child_process *get_helper(struct transport *transport)\n \twrite_constant(helper->in, \"capabilities\\n\");\n \n \twhile (1) {\n-\t\tconst char *capname;\n+\t\tconst char *capname, *arg;\n \t\tint mandatory = 0;\n \t\tif (recvline(data, &buf))\n \t\t\texit(128);\n@@ -183,19 +183,19 @@ static struct child_process *get_helper(struct transport *transport)\n \t\t\tdata->export = 1;\n \t\telse if (!strcmp(capname, \"check-connectivity\"))\n \t\t\tdata->check_connectivity = 1;\n-\t\telse if (!data->refspecs && starts_with(capname, \"refspec \")) {\n+\t\telse if (!data->refspecs && skip_prefix(capname, \"refspec \", &arg)) {\n \t\t\tALLOC_GROW(refspecs,\n \t\t\t\t refspec_nr + 1,\n \t\t\t\t refspec_alloc);\n-\t\t\trefspecs[refspec_nr++] = xstrdup(capname + strlen(\"refspec \"));\n+\t\t\trefspecs[refspec_nr++] = xstrdup(arg);\n \t\t} else if (!strcmp(capname, \"connect\")) {\n \t\t\tdata->connect = 1;\n \t\t} else if (!strcmp(capname, \"signed-tags\")) {\n \t\t\tdata->signed_tags = 1;\n-\t\t} else if (starts_with(capname, \"export-marks \")) {\n-\t\t\tdata->export_marks = xstrdup(capname + strlen(\"export-marks \"));\n-\t\t} else if (starts_with(capname, \"import-marks\")) {\n-\t\t\tdata->import_marks = xstrdup(capname + strlen(\"import-marks \"));\n+\t\t} else if (skip_prefix(capname, \"export-marks \", &arg)) {\n+\t\t\tdata->export_marks = xstrdup(arg);\n+\t\t} else if (skip_prefix(capname, \"import-marks \", &arg)) {\n+\t\t\tdata->import_marks = xstrdup(arg);\n \t\t} else if (starts_with(capname, \"no-private-update\")) {\n \t\t\tdata->no_private_update = 1;\n \t\t} else if (mandatory) {\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244569","messageId":"20140618194750.GH22622@sigill.intra.peff.net","threadId":"36953","inReplyTo":"20140618194117.GA22269@sigill.intra.peff.net","subject":"[PATCH 08/16] use skip_prefix to avoid magic numbers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-18T19:47:50Z","receivedAt":"2014-06-18T19:47:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"It's a common idiom to match a prefix and then skip past it\nwith a magic number, like:\n\n if (starts_with(foo, \"bar\"))\n\t foo += 3;\n\nThis is easy to get wrong, since you have to count the\nprefix string yourself, and there's no compiler check if the\nstring changes. We can use skip_prefix to avoid the magic\nnumbers here.\n\nNote that some of these conversions could be much shorter.\nFor example:\n\n if (starts_with(arg, \"--foo=\")) {\n\t bar = arg + 6;\n\t continue;\n }\n\ncould become:\n\n if (skip_prefix(arg, \"--foo=\", &bar))\n\t continue;\n\nHowever, I have left it as:\n\n if (skip_prefix(arg, \"--foo=\", &v)) {\n\t bar = v;\n\t continue;\n }\n\nto visually match nearby cases which need to actually\nprocess the string. Like:\n\n if (skip_prefix(arg, \"--foo=\", &v)) {\n\t bar = atoi(v);\n\t continue;\n }\n\nSigned-off-by: Jeff King \n---\n alias.c | 3 ++-\n connect.c | 11 +++++----\n convert.c | 4 ++--\n daemon.c | 73 ++++++++++++++++++++++++++++++----------------------------\n diff.c | 65 ++++++++++++++++++++++++++-------------------------\n fast-import.c | 69 +++++++++++++++++++++++++++++-------------------------\n fetch-pack.c | 9 ++++----\n git.c | 18 +++++++--------\n help.c | 6 +++--\n http-backend.c | 11 +++++----\n http-push.c | 11 +++++----\n 11 files changed, 149 insertions(+), 131 deletions(-)\n\ndiff --git a/alias.c b/alias.c\nindex 5efc3d6..758c867 100644\n--- a/alias.c\n+++ b/alias.c\n@@ -5,7 +5,8 @@ static char *alias_val;\n \n static int alias_lookup_cb(const char *k, const char *v, void *cb)\n {\n-\tif (starts_with(k, \"alias.\") && !strcmp(k + 6, alias_key)) {\n+\tconst char *name;\n+\tif (skip_prefix(k, \"alias.\", &name) && !strcmp(name, alias_key)) {\n \t\tif (!v)\n \t\t\treturn config_error_nonbool(k);\n \t\talias_val = xstrdup(v);\ndiff --git a/connect.c b/connect.c\nindex 94a6650..37ff018 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -129,6 +129,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n \t\tchar *name;\n \t\tint len, name_len;\n \t\tchar *buffer = packet_buffer;\n+\t\tconst char *arg;\n \n \t\tlen = packet_read(in, &src_buf, &src_len,\n \t\t\t\t packet_buffer, sizeof(packet_buffer),\n@@ -140,12 +141,12 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n \t\tif (!len)\n \t\t\tbreak;\n \n-\t\tif (len > 4 && starts_with(buffer, \"ERR \"))\n-\t\t\tdie(\"remote error: %s\", buffer + 4);\n+\t\tif (len > 4 && skip_prefix(buffer, \"ERR \", &arg))\n+\t\t\tdie(\"remote error: %s\", arg);\n \n-\t\tif (len == 48 && starts_with(buffer, \"shallow \")) {\n-\t\t\tif (get_sha1_hex(buffer + 8, old_sha1))\n-\t\t\t\tdie(\"protocol error: expected shallow sha-1, got '%s'\", buffer + 8);\n+\t\tif (len == 48 && skip_prefix(buffer, \"shallow \", &arg)) {\n+\t\t\tif (get_sha1_hex(arg, old_sha1))\n+\t\t\t\tdie(\"protocol error: expected shallow sha-1, got '%s'\", arg);\n \t\t\tif (!shallow_points)\n \t\t\t\tdie(\"repository on the other end cannot be shallow\");\n \t\t\tsha1_array_append(shallow_points, old_sha1);\ndiff --git a/convert.c b/convert.c\nindex ab80b72..cb5fbb4 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -1121,9 +1121,9 @@ static int is_foreign_ident(const char *str)\n {\n \tint i;\n \n-\tif (!starts_with(str, \"$Id: \"))\n+\tif (!skip_prefix(str, \"$Id: \", &str))\n \t\treturn 0;\n-\tfor (i = 5; str[i]; i++) {\n+\tfor (i = 0; str[i]; i++) {\n \t\tif (isspace(str[i]) && str[i+1] != '$')\n \t\t\treturn 1;\n \t}\ndiff --git a/daemon.c b/daemon.c\nindex 18818c3..6d25828 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -235,8 +235,10 @@ static int service_enabled;\n \n static int git_daemon_config(const char *var, const char *value, void *cb)\n {\n-\tif (starts_with(var, \"daemon.\") &&\n-\t !strcmp(var + 7, service_looking_at->config_name)) {\n+\tconst char *service;\n+\n+\tif (skip_prefix(var, \"daemon.\", &service) &&\n+\t !strcmp(service, service_looking_at->config_name)) {\n \t\tservice_enabled = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n@@ -1133,16 +1135,17 @@ int main(int argc, char **argv)\n \n \tfor (i = 1; i < argc; i++) {\n \t\tchar *arg = argv[i];\n+\t\tconst char *v;\n \n-\t\tif (starts_with(arg, \"--listen=\")) {\n-\t\t\tstring_list_append(&listen_addr, xstrdup_tolower(arg + 9));\n+\t\tif (skip_prefix(arg, \"--listen=\", &v)) {\n+\t\t\tstring_list_append(&listen_addr, xstrdup_tolower(v));\n \t\t\tcontinue;\n \t\t}\n-\t\tif (starts_with(arg, \"--port=\")) {\n+\t\tif (skip_prefix(arg, \"--port=\", &v)) {\n \t\t\tchar *end;\n \t\t\tunsigned long n;\n-\t\t\tn = strtoul(arg+7, &end, 0);\n-\t\t\tif (arg[7] && !*end) {\n+\t\t\tn = strtoul(v, &end, 0);\n+\t\t\tif (*v && !*end) {\n \t\t\t\tlisten_port = n;\n \t\t\t\tcontinue;\n \t\t\t}\n@@ -1168,20 +1171,20 @@ int main(int argc, char **argv)\n \t\t\texport_all_trees = 1;\n \t\t\tcontinue;\n \t\t}\n-\t\tif (starts_with(arg, \"--access-hook=\")) {\n-\t\t\taccess_hook = arg + 14;\n+\t\tif (skip_prefix(arg, \"--access-hook=\", &v)) {\n+\t\t\taccess_hook = v;\n \t\t\tcontinue;\n \t\t}\n-\t\tif (starts_with(arg, \"--timeout=\")) {\n-\t\t\ttimeout = atoi(arg+10);\n+\t\tif (skip_prefix(arg, \"--timeout=\", &v)) {\n+\t\t\ttimeout = atoi(v);\n \t\t\tcontinue;\n \t\t}\n-\t\tif (starts_with(arg, \"--init-timeout=\")) {\n-\t\t\tinit_timeout = atoi(arg+15);\n+\t\tif (skip_prefix(arg, \"--init-timeout=\", &v)) {\n+\t\t\tinit_timeout = atoi(v);\n \t\t\tcontinue;\n \t\t}\n-\t\tif (starts_with(arg, \"--max-connections=\")) {\n-\t\t\tmax_connections = atoi(arg+18);\n+\t\tif (skip_prefix(arg, \"--max-connections=\", &v)) {\n+\t\t\tmax_connections = atoi(v);\n \t\t\tif (max_connections < 0)\n \t\t\t\tmax_connections = 0;\t /* unlimited */\n \t\t\tcontinue;\n@@ -1190,16 +1193,16 @@ int main(int argc, char **argv)\n \t\t\tstrict_paths = 1;\n \t\t\tcontinue;\n \t\t}\n-\t\tif (starts_with(arg, \"--base-path=\")) {\n-\t\t\tbase_path = arg+12;\n+\t\tif (skip_prefix(arg, \"--base-path=\", &v)) {\n+\t\t\tbase_path = v;\n \t\t\tcontinue;\n \t\t}\n \t\tif (!strcmp(arg, \"--base-path-relaxed\")) {\n \t\t\tbase_path_relaxed = 1;\n \t\t\tcontinue;\n \t\t}\n-\t\tif (starts_with(arg, \"--interpolated-path=\")) {\n-\t\t\tinterpolated_path = arg+20;\n+\t\tif (skip_prefix(arg, \"--interpolated-path=\", &v)) {\n+\t\t\tinterpolated_path = v;\n \t\t\tcontinue;\n \t\t}\n \t\tif (!strcmp(arg, \"--reuseaddr\")) {\n@@ -1210,12 +1213,12 @@ int main(int argc, char **argv)\n \t\t\tuser_path = \"\";\n \t\t\tcontinue;\n \t\t}\n-\t\tif (starts_with(arg, \"--user-path=\")) {\n-\t\t\tuser_path = arg + 12;\n+\t\tif (skip_prefix(arg, \"--user-path=\", &v)) {\n+\t\t\tuser_path = v;\n \t\t\tcontinue;\n \t\t}\n-\t\tif (starts_with(arg, \"--pid-file=\")) {\n-\t\t\tpid_file = arg + 11;\n+\t\tif (skip_prefix(arg, \"--pid-file=\", &v)) {\n+\t\t\tpid_file = v;\n \t\t\tcontinue;\n \t\t}\n \t\tif (!strcmp(arg, \"--detach\")) {\n@@ -1223,28 +1226,28 @@ int main(int argc, char **argv)\n \t\t\tlog_syslog = 1;\n \t\t\tcontinue;\n \t\t}\n-\t\tif (starts_with(arg, \"--user=\")) {\n-\t\t\tuser_name = arg + 7;\n+\t\tif (skip_prefix(arg, \"--user=\", &v)) {\n+\t\t\tuser_name = v;\n \t\t\tcontinue;\n \t\t}\n-\t\tif (starts_with(arg, \"--group=\")) {\n-\t\t\tgroup_name = arg + 8;\n+\t\tif (skip_prefix(arg, \"--group=\", &v)) {\n+\t\t\tgroup_name = v;\n \t\t\tcontinue;\n \t\t}\n-\t\tif (starts_with(arg, \"--enable=\")) {\n-\t\t\tenable_service(arg + 9, 1);\n+\t\tif (skip_prefix(arg, \"--enable=\", &v)) {\n+\t\t\tenable_service(v, 1);\n \t\t\tcontinue;\n \t\t}\n-\t\tif (starts_with(arg, \"--disable=\")) {\n-\t\t\tenable_service(arg + 10, 0);\n+\t\tif (skip_prefix(arg, \"--disable=\", &v)) {\n+\t\t\tenable_service(v, 0);\n \t\t\tcontinue;\n \t\t}\n-\t\tif (starts_with(arg, \"--allow-override=\")) {\n-\t\t\tmake_service_overridable(arg + 17, 1);\n+\t\tif (skip_prefix(arg, \"--allow-override=\", &v)) {\n+\t\t\tmake_service_overridable(v, 1);\n \t\t\tcontinue;\n \t\t}\n-\t\tif (starts_with(arg, \"--forbid-override=\")) {\n-\t\t\tmake_service_overridable(arg + 18, 0);\n+\t\tif (skip_prefix(arg, \"--forbid-override=\", &v)) {\n+\t\t\tmake_service_overridable(v, 0);\n \t\t\tcontinue;\n \t\t}\n \t\tif (!strcmp(arg, \"--informative-errors\")) {\ndiff --git a/diff.c b/diff.c\nindex 77c5eb4..97db932 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -231,6 +231,8 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n \n int git_diff_basic_config(const char *var, const char *value, void *cb)\n {\n+\tconst char *name;\n+\n \tif (!strcmp(var, \"diff.renamelimit\")) {\n \t\tdiff_rename_limit_default = git_config_int(var, value);\n \t\treturn 0;\n@@ -239,8 +241,9 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)\n \tif (userdiff_config(var, value) < 0)\n \t\treturn -1;\n \n-\tif (starts_with(var, \"diff.color.\") || starts_with(var, \"color.diff.\")) {\n-\t\tint slot = parse_diff_color_slot(var + 11);\n+\tif (skip_prefix(var, \"diff.color.\", &name) ||\n+\t skip_prefix(var, \"color.diff.\", &name)) {\n+\t\tint slot = parse_diff_color_slot(name);\n \t\tif (slot < 0)\n \t\t\treturn 0;\n \t\tif (!value)\n@@ -2341,6 +2344,7 @@ static void builtin_diff(const char *name_a,\n \t} else {\n \t\t/* Crazy xdl interfaces.. */\n \t\tconst char *diffopts = getenv(\"GIT_DIFF_OPTS\");\n+\t\tconst char *v;\n \t\txpparam_t xpp;\n \t\txdemitconf_t xecfg;\n \t\tstruct emit_callback ecbdata;\n@@ -2379,10 +2383,10 @@ static void builtin_diff(const char *name_a,\n \t\t\txdiff_set_find_func(&xecfg, pe->pattern, pe->cflags);\n \t\tif (!diffopts)\n \t\t\t;\n-\t\telse if (starts_with(diffopts, \"--unified=\"))\n-\t\t\txecfg.ctxlen = strtoul(diffopts + 10, NULL, 10);\n-\t\telse if (starts_with(diffopts, \"-u\"))\n-\t\t\txecfg.ctxlen = strtoul(diffopts + 2, NULL, 10);\n+\t\telse if (skip_prefix(diffopts, \"--unified=\", &v))\n+\t\t\txecfg.ctxlen = strtoul(v, NULL, 10);\n+\t\telse if (skip_prefix(diffopts, \"-u\", &v))\n+\t\t\txecfg.ctxlen = strtoul(v, NULL, 10);\n \t\tif (o->word_diff)\n \t\t\tinit_diff_words_data(&ecbdata, o, one, two);\n \t\txdi_diff_outf(&mf1, &mf2, fn_out_consume, &ecbdata,\n@@ -3609,17 +3613,17 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n \t\toptions->output_format |= DIFF_FORMAT_SHORTSTAT;\n \telse if (!strcmp(arg, \"-X\") || !strcmp(arg, \"--dirstat\"))\n \t\treturn parse_dirstat_opt(options, \"\");\n-\telse if (starts_with(arg, \"-X\"))\n-\t\treturn parse_dirstat_opt(options, arg + 2);\n-\telse if (starts_with(arg, \"--dirstat=\"))\n-\t\treturn parse_dirstat_opt(options, arg + 10);\n+\telse if (skip_prefix(arg, \"-X\", &arg))\n+\t\treturn parse_dirstat_opt(options, arg);\n+\telse if (skip_prefix(arg, \"--dirstat=\", &arg))\n+\t\treturn parse_dirstat_opt(options, arg);\n \telse if (!strcmp(arg, \"--cumulative\"))\n \t\treturn parse_dirstat_opt(options, \"cumulative\");\n \telse if (!strcmp(arg, \"--dirstat-by-file\"))\n \t\treturn parse_dirstat_opt(options, \"files\");\n-\telse if (starts_with(arg, \"--dirstat-by-file=\")) {\n+\telse if (skip_prefix(arg, \"--dirstat-by-file=\", &arg)) {\n \t\tparse_dirstat_opt(options, \"files\");\n-\t\treturn parse_dirstat_opt(options, arg + 18);\n+\t\treturn parse_dirstat_opt(options, arg);\n \t}\n \telse if (!strcmp(arg, \"--check\"))\n \t\toptions->output_format |= DIFF_FORMAT_CHECKDIFF;\n@@ -3669,9 +3673,9 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n \t\tDIFF_OPT_CLR(options, RENAME_EMPTY);\n \telse if (!strcmp(arg, \"--relative\"))\n \t\tDIFF_OPT_SET(options, RELATIVE_NAME);\n-\telse if (starts_with(arg, \"--relative=\")) {\n+\telse if (skip_prefix(arg, \"--relative=\", &arg)) {\n \t\tDIFF_OPT_SET(options, RELATIVE_NAME);\n-\t\toptions->prefix = arg + 11;\n+\t\toptions->prefix = arg;\n \t}\n \n \t/* xdiff options */\n@@ -3722,8 +3726,8 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n \t\tDIFF_OPT_CLR(options, FOLLOW_RENAMES);\n \telse if (!strcmp(arg, \"--color\"))\n \t\toptions->use_color = 1;\n-\telse if (starts_with(arg, \"--color=\")) {\n-\t\tint value = git_config_colorbool(NULL, arg+8);\n+\telse if (skip_prefix(arg, \"--color=\", &arg)) {\n+\t\tint value = git_config_colorbool(NULL, arg);\n \t\tif (value < 0)\n \t\t\treturn error(\"option `color' expects \\\"always\\\", \\\"auto\\\", or \\\"never\\\"\");\n \t\toptions->use_color = value;\n@@ -3734,29 +3738,28 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n \t\toptions->use_color = 1;\n \t\toptions->word_diff = DIFF_WORDS_COLOR;\n \t}\n-\telse if (starts_with(arg, \"--color-words=\")) {\n+\telse if (skip_prefix(arg, \"--color-words=\", &arg)) {\n \t\toptions->use_color = 1;\n \t\toptions->word_diff = DIFF_WORDS_COLOR;\n-\t\toptions->word_regex = arg + 14;\n+\t\toptions->word_regex = arg;\n \t}\n \telse if (!strcmp(arg, \"--word-diff\")) {\n \t\tif (options->word_diff == DIFF_WORDS_NONE)\n \t\t\toptions->word_diff = DIFF_WORDS_PLAIN;\n \t}\n-\telse if (starts_with(arg, \"--word-diff=\")) {\n-\t\tconst char *type = arg + 12;\n-\t\tif (!strcmp(type, \"plain\"))\n+\telse if (skip_prefix(arg, \"--word-diff=\", &arg)) {\n+\t\tif (!strcmp(arg, \"plain\"))\n \t\t\toptions->word_diff = DIFF_WORDS_PLAIN;\n-\t\telse if (!strcmp(type, \"color\")) {\n+\t\telse if (!strcmp(arg, \"color\")) {\n \t\t\toptions->use_color = 1;\n \t\t\toptions->word_diff = DIFF_WORDS_COLOR;\n \t\t}\n-\t\telse if (!strcmp(type, \"porcelain\"))\n+\t\telse if (!strcmp(arg, \"porcelain\"))\n \t\t\toptions->word_diff = DIFF_WORDS_PORCELAIN;\n-\t\telse if (!strcmp(type, \"none\"))\n+\t\telse if (!strcmp(arg, \"none\"))\n \t\t\toptions->word_diff = DIFF_WORDS_NONE;\n \t\telse\n-\t\t\tdie(\"bad --word-diff argument: %s\", type);\n+\t\t\tdie(\"bad --word-diff argument: %s\", arg);\n \t}\n \telse if ((argcount = parse_long_opt(\"word-diff-regex\", av, &optarg))) {\n \t\tif (options->word_diff == DIFF_WORDS_NONE)\n@@ -3779,13 +3782,13 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n \telse if (!strcmp(arg, \"--ignore-submodules\")) {\n \t\tDIFF_OPT_SET(options, OVERRIDE_SUBMODULE_CONFIG);\n \t\thandle_ignore_submodules_arg(options, \"all\");\n-\t} else if (starts_with(arg, \"--ignore-submodules=\")) {\n+\t} else if (skip_prefix(arg, \"--ignore-submodules=\", &arg)) {\n \t\tDIFF_OPT_SET(options, OVERRIDE_SUBMODULE_CONFIG);\n-\t\thandle_ignore_submodules_arg(options, arg + 20);\n+\t\thandle_ignore_submodules_arg(options, arg);\n \t} else if (!strcmp(arg, \"--submodule\"))\n \t\tDIFF_OPT_SET(options, SUBMODULE_LOG);\n-\telse if (starts_with(arg, \"--submodule=\"))\n-\t\treturn parse_submodule_opt(options, arg + 12);\n+\telse if (skip_prefix(arg, \"--submodule=\", &arg))\n+\t\treturn parse_submodule_opt(options, arg);\n \n \t/* misc options */\n \telse if (!strcmp(arg, \"-z\"))\n@@ -3820,8 +3823,8 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n \t}\n \telse if (!strcmp(arg, \"--abbrev\"))\n \t\toptions->abbrev = DEFAULT_ABBREV;\n-\telse if (starts_with(arg, \"--abbrev=\")) {\n-\t\toptions->abbrev = strtoul(arg + 9, NULL, 10);\n+\telse if (skip_prefix(arg, \"--abbrev=\", &arg)) {\n+\t\toptions->abbrev = strtoul(arg, NULL, 10);\n \t\tif (options->abbrev < MINIMUM_ABBREV)\n \t\t\toptions->abbrev = MINIMUM_ABBREV;\n \t\telse if (40 < options->abbrev)\ndiff --git a/fast-import.c b/fast-import.c\nindex b2030cc..a3ffe30 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1912,8 +1912,9 @@ static void skip_optional_lf(void)\n \n static void parse_mark(void)\n {\n-\tif (starts_with(command_buf.buf, \"mark :\")) {\n-\t\tnext_mark = strtoumax(command_buf.buf + 6, NULL, 10);\n+\tconst char *v;\n+\tif (skip_prefix(command_buf.buf, \"mark :\", &v)) {\n+\t\tnext_mark = strtoumax(v, NULL, 10);\n \t\tread_next_command();\n \t}\n \telse\n@@ -1922,14 +1923,15 @@ static void parse_mark(void)\n \n static int parse_data(struct strbuf *sb, uintmax_t limit, uintmax_t *len_res)\n {\n+\tconst char *data;\n \tstrbuf_reset(sb);\n \n-\tif (!starts_with(command_buf.buf, \"data \"))\n+\tif (!skip_prefix(command_buf.buf, \"data \", &data))\n \t\tdie(\"Expected 'data n' command, found: %s\", command_buf.buf);\n \n-\tif (starts_with(command_buf.buf + 5, \"<<\")) {\n-\t\tchar *term = xstrdup(command_buf.buf + 5 + 2);\n-\t\tsize_t term_len = command_buf.len - 5 - 2;\n+\tif (skip_prefix(data, \"<<\", &data)) {\n+\t\tchar *term = xstrdup(data);\n+\t\tsize_t term_len = command_buf.len - (data - command_buf.buf);\n \n \t\tstrbuf_detach(&command_buf, NULL);\n \t\tfor (;;) {\n@@ -1944,7 +1946,7 @@ static int parse_data(struct strbuf *sb, uintmax_t limit, uintmax_t *len_res)\n \t\tfree(term);\n \t}\n \telse {\n-\t\tuintmax_t len = strtoumax(command_buf.buf + 5, NULL, 10);\n+\t\tuintmax_t len = strtoumax(data, NULL, 10);\n \t\tsize_t n = 0, length = (size_t)len;\n \n \t\tif (limit && limit < len) {\n@@ -2676,6 +2678,7 @@ static void parse_new_commit(void)\n \tstruct hash_list *merge_list = NULL;\n \tunsigned int merge_count;\n \tunsigned char prev_fanout, new_fanout;\n+\tconst char *v;\n \n \t/* Obtain the branch name from the rest of our command */\n \tsp = strchr(command_buf.buf, ' ') + 1;\n@@ -2685,12 +2688,12 @@ static void parse_new_commit(void)\n \n \tread_next_command();\n \tparse_mark();\n-\tif (starts_with(command_buf.buf, \"author \")) {\n-\t\tauthor = parse_ident(command_buf.buf + 7);\n+\tif (skip_prefix(command_buf.buf, \"author \", &v)) {\n+\t\tauthor = parse_ident(v);\n \t\tread_next_command();\n \t}\n-\tif (starts_with(command_buf.buf, \"committer \")) {\n-\t\tcommitter = parse_ident(command_buf.buf + 10);\n+\tif (skip_prefix(command_buf.buf, \"committer \", &v)) {\n+\t\tcommitter = parse_ident(v);\n \t\tread_next_command();\n \t}\n \tif (!committer)\n@@ -2777,6 +2780,7 @@ static void parse_new_tag(void)\n \tuintmax_t from_mark = 0;\n \tunsigned char sha1[20];\n \tenum object_type type;\n+\tconst char *v;\n \n \t/* Obtain the new tag name from the rest of our command */\n \tsp = strchr(command_buf.buf, ' ') + 1;\n@@ -2819,8 +2823,8 @@ static void parse_new_tag(void)\n \tread_next_command();\n \n \t/* tagger ... */\n-\tif (starts_with(command_buf.buf, \"tagger \")) {\n-\t\ttagger = parse_ident(command_buf.buf + 7);\n+\tif (skip_prefix(command_buf.buf, \"tagger \", &v)) {\n+\t\ttagger = parse_ident(v);\n \t\tread_next_command();\n \t} else\n \t\ttagger = NULL;\n@@ -3207,9 +3211,9 @@ static void option_export_pack_edges(const char *edges)\n \n static int parse_one_option(const char *option)\n {\n-\tif (starts_with(option, \"max-pack-size=\")) {\n+\tif (skip_prefix(option, \"max-pack-size=\", &option)) {\n \t\tunsigned long v;\n-\t\tif (!git_parse_ulong(option + 14, &v))\n+\t\tif (!git_parse_ulong(option, &v))\n \t\t\treturn 0;\n \t\tif (v < 8192) {\n \t\t\twarning(\"max-pack-size is now in bytes, assuming --max-pack-size=%lum\", v);\n@@ -3219,17 +3223,17 @@ static int parse_one_option(const char *option)\n \t\t\tv = 1024 * 1024;\n \t\t}\n \t\tmax_packsize = v;\n-\t} else if (starts_with(option, \"big-file-threshold=\")) {\n+\t} else if (skip_prefix(option, \"big-file-threshold=\", &option)) {\n \t\tunsigned long v;\n-\t\tif (!git_parse_ulong(option + 19, &v))\n+\t\tif (!git_parse_ulong(option, &v))\n \t\t\treturn 0;\n \t\tbig_file_threshold = v;\n-\t} else if (starts_with(option, \"depth=\")) {\n-\t\toption_depth(option + 6);\n-\t} else if (starts_with(option, \"active-branches=\")) {\n-\t\toption_active_branches(option + 16);\n-\t} else if (starts_with(option, \"export-pack-edges=\")) {\n-\t\toption_export_pack_edges(option + 18);\n+\t} else if (skip_prefix(option, \"depth=\", &option)) {\n+\t\toption_depth(option);\n+\t} else if (skip_prefix(option, \"active-branches=\", &option)) {\n+\t\toption_active_branches(option);\n+\t} else if (skip_prefix(option, \"export-pack-edges=\", &option)) {\n+\t\toption_export_pack_edges(option);\n \t} else if (starts_with(option, \"quiet\")) {\n \t\tshow_stats = 0;\n \t} else if (starts_with(option, \"stats\")) {\n@@ -3243,15 +3247,16 @@ static int parse_one_option(const char *option)\n \n static int parse_one_feature(const char *feature, int from_stream)\n {\n-\tif (starts_with(feature, \"date-format=\")) {\n-\t\toption_date_format(feature + 12);\n-\t} else if (starts_with(feature, \"import-marks=\")) {\n-\t\toption_import_marks(feature + 13, from_stream, 0);\n-\t} else if (starts_with(feature, \"import-marks-if-exists=\")) {\n-\t\toption_import_marks(feature + strlen(\"import-marks-if-exists=\"),\n-\t\t\t\t\tfrom_stream, 1);\n-\t} else if (starts_with(feature, \"export-marks=\")) {\n-\t\toption_export_marks(feature + 13);\n+\tconst char *arg;\n+\n+\tif (skip_prefix(feature, \"date-format=\", &arg)) {\n+\t\toption_date_format(arg);\n+\t} else if (skip_prefix(feature, \"import-marks=\", &arg)) {\n+\t\toption_import_marks(arg, from_stream, 0);\n+\t} else if (skip_prefix(feature, \"import-marks-if-exists=\", &arg)) {\n+\t\toption_import_marks(arg, from_stream, 1);\n+\t} else if (skip_prefix(feature, \"export-marks=\", &arg)) {\n+\t\toption_export_marks(arg);\n \t} else if (!strcmp(feature, \"cat-blob\")) {\n \t\t; /* Don't die - this feature is supported */\n \t} else if (!strcmp(feature, \"relative-marks\")) {\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex eeee2bb..3de3bd5 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -319,18 +319,19 @@ static int find_common(struct fetch_pack_args *args,\n \n \tif (args->depth > 0) {\n \t\tchar *line;\n+\t\tconst char *arg;\n \t\tunsigned char sha1[20];\n \n \t\tsend_request(args, fd[1], &req_buf);\n \t\twhile ((line = packet_read_line(fd[0], NULL))) {\n-\t\t\tif (starts_with(line, \"shallow \")) {\n-\t\t\t\tif (get_sha1_hex(line + 8, sha1))\n+\t\t\tif (skip_prefix(line, \"shallow \", &arg)) {\n+\t\t\t\tif (get_sha1_hex(arg, sha1))\n \t\t\t\t\tdie(\"invalid shallow line: %s\", line);\n \t\t\t\tregister_shallow(sha1);\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\tif (starts_with(line, \"unshallow \")) {\n-\t\t\t\tif (get_sha1_hex(line + 10, sha1))\n+\t\t\tif (skip_prefix(line, \"unshallow \", &arg)) {\n+\t\t\t\tif (get_sha1_hex(arg, sha1))\n \t\t\t\t\tdie(\"invalid unshallow line: %s\", line);\n \t\t\t\tif (!lookup_object(sha1))\n \t\t\t\t\tdie(\"object not found: %s\", line);\ndiff --git a/git.c b/git.c\nindex 7780572..b2bb09e 100644\n--- a/git.c\n+++ b/git.c\n@@ -54,8 +54,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t/*\n \t\t * Check remaining flags.\n \t\t */\n-\t\tif (starts_with(cmd, \"--exec-path\")) {\n-\t\t\tcmd += 11;\n+\t\tif (skip_prefix(cmd, \"--exec-path\", &cmd)) {\n \t\t\tif (*cmd == '=')\n \t\t\t\tgit_set_argv_exec_path(cmd + 1);\n \t\t\telse {\n@@ -92,8 +91,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\t\t*envchanged = 1;\n \t\t\t(*argv)++;\n \t\t\t(*argc)--;\n-\t\t} else if (starts_with(cmd, \"--git-dir=\")) {\n-\t\t\tsetenv(GIT_DIR_ENVIRONMENT, cmd + 10, 1);\n+\t\t} else if (skip_prefix(cmd, \"--git-dir=\", &cmd)) {\n+\t\t\tsetenv(GIT_DIR_ENVIRONMENT, cmd, 1);\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n \t\t} else if (!strcmp(cmd, \"--namespace\")) {\n@@ -106,8 +105,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\t\t*envchanged = 1;\n \t\t\t(*argv)++;\n \t\t\t(*argc)--;\n-\t\t} else if (starts_with(cmd, \"--namespace=\")) {\n-\t\t\tsetenv(GIT_NAMESPACE_ENVIRONMENT, cmd + 12, 1);\n+\t\t} else if (skip_prefix(cmd, \"--namespace=\", &cmd)) {\n+\t\t\tsetenv(GIT_NAMESPACE_ENVIRONMENT, cmd, 1);\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n \t\t} else if (!strcmp(cmd, \"--work-tree\")) {\n@@ -120,8 +119,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\t\t*envchanged = 1;\n \t\t\t(*argv)++;\n \t\t\t(*argc)--;\n-\t\t} else if (starts_with(cmd, \"--work-tree=\")) {\n-\t\t\tsetenv(GIT_WORK_TREE_ENVIRONMENT, cmd + 12, 1);\n+\t\t} else if (skip_prefix(cmd, \"--work-tree=\", &cmd)) {\n+\t\t\tsetenv(GIT_WORK_TREE_ENVIRONMENT, cmd, 1);\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n \t\t} else if (!strcmp(cmd, \"--bare\")) {\n@@ -578,8 +577,7 @@ int main(int argc, char **av)\n \t * So we just directly call the builtin handler, and die if\n \t * that one cannot handle it.\n \t */\n-\tif (starts_with(cmd, \"git-\")) {\n-\t\tcmd += 4;\n+\tif (skip_prefix(cmd, \"git-\", &cmd)) {\n \t\targv[0] = cmd;\n \t\thandle_builtin(argc, argv);\n \t\tdie(\"cannot handle %s as a builtin\", cmd);\ndiff --git a/help.c b/help.c\nindex b266b09..b0f1a69 100644\n--- a/help.c\n+++ b/help.c\n@@ -251,11 +251,13 @@ static struct cmdnames aliases;\n \n static int git_unknown_cmd_config(const char *var, const char *value, void *cb)\n {\n+\tconst char *p;\n+\n \tif (!strcmp(var, \"help.autocorrect\"))\n \t\tautocorrect = git_config_int(var,value);\n \t/* Also use aliases for command lookup */\n-\tif (starts_with(var, \"alias.\"))\n-\t\tadd_cmdname(&aliases, var + 6, strlen(var + 6));\n+\tif (skip_prefix(var, \"alias.\", &p))\n+\t\tadd_cmdname(&aliases, p, strlen(p));\n \n \treturn git_default_config(var, value, cb);\n }\ndiff --git a/http-backend.c b/http-backend.c\nindex d2c0a62..57290d9 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -221,17 +221,19 @@ static void get_idx_file(char *name)\n \n static int http_config(const char *var, const char *value, void *cb)\n {\n+\tconst char *p;\n+\n \tif (!strcmp(var, \"http.getanyfile\")) {\n \t\tgetanyfile = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n \n-\tif (starts_with(var, \"http.\")) {\n+\tif (skip_prefix(var, \"http.\", &p)) {\n \t\tint i;\n \n \t\tfor (i = 0; i < ARRAY_SIZE(rpc_service); i++) {\n \t\t\tstruct rpc_service *svc = &rpc_service[i];\n-\t\t\tif (!strcmp(var + 5, svc->config_name)) {\n+\t\t\tif (!strcmp(p, svc->config_name)) {\n \t\t\t\tsvc->enabled = git_config_bool(var, value);\n \t\t\t\treturn 0;\n \t\t\t}\n@@ -244,15 +246,16 @@ static int http_config(const char *var, const char *value, void *cb)\n \n static struct rpc_service *select_service(const char *name)\n {\n+\tconst char *svc_name;\n \tstruct rpc_service *svc = NULL;\n \tint i;\n \n-\tif (!starts_with(name, \"git-\"))\n+\tif (!skip_prefix(name, \"git-\", &svc_name))\n \t\tforbidden(\"Unsupported service: '%s'\", name);\n \n \tfor (i = 0; i < ARRAY_SIZE(rpc_service); i++) {\n \t\tstruct rpc_service *s = &rpc_service[i];\n-\t\tif (!strcmp(s->name, name + 4)) {\n+\t\tif (!strcmp(s->name, svc_name)) {\n \t\t\tsvc = s;\n \t\t\tbreak;\n \t\t}\ndiff --git a/http-push.c b/http-push.c\nindex de00d16..26dfa67 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -770,9 +770,9 @@ static void handle_new_lock_ctx(struct xml_ctx *ctx, int tag_closed)\n \t\t\tlock->owner = xmalloc(strlen(ctx->cdata) + 1);\n \t\t\tstrcpy(lock->owner, ctx->cdata);\n \t\t} else if (!strcmp(ctx->name, DAV_ACTIVELOCK_TIMEOUT)) {\n-\t\t\tif (starts_with(ctx->cdata, \"Second-\"))\n-\t\t\t\tlock->timeout =\n-\t\t\t\t\tstrtol(ctx->cdata + 7, NULL, 10);\n+\t\t\tconst char *arg;\n+\t\t\tif (skip_prefix(ctx->cdata, \"Second-\", &arg))\n+\t\t\t\tlock->timeout = strtol(arg, NULL, 10);\n \t\t} else if (!strcmp(ctx->name, DAV_ACTIVELOCK_TOKEN)) {\n \t\t\tlock->token = xmalloc(strlen(ctx->cdata) + 1);\n \t\t\tstrcpy(lock->token, ctx->cdata);\n@@ -1561,6 +1561,7 @@ static void fetch_symref(const char *path, char **symref, unsigned char *sha1)\n {\n \tchar *url;\n \tstruct strbuf buffer = STRBUF_INIT;\n+\tconst char *name;\n \n \turl = xmalloc(strlen(repo->url) + strlen(path) + 1);\n \tsprintf(url, \"%s%s\", repo->url, path);\n@@ -1578,8 +1579,8 @@ static void fetch_symref(const char *path, char **symref, unsigned char *sha1)\n \t\treturn;\n \n \t/* If it's a symref, set the refname; otherwise try for a sha1 */\n-\tif (starts_with((char *)buffer.buf, \"ref: \")) {\n-\t\t*symref = xmemdupz((char *)buffer.buf + 5, buffer.len - 6);\n+\tif (skip_prefix(buffer.buf, \"ref: \", &name)) {\n+\t\t*symref = xmemdupz(name, buffer.len - (name - buffer.buf));\n \t} else {\n \t\tget_sha1_hex(buffer.buf, sha1);\n \t}\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244570","messageId":"20140618194829.GI22622@sigill.intra.peff.net","threadId":"36953","inReplyTo":"20140618194117.GA22269@sigill.intra.peff.net","subject":"[PATCH 09/16] use skip_prefix to avoid repeating strings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-18T19:48:29Z","receivedAt":"2014-06-18T19:48:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"It's a common idiom to match a prefix and then skip past it\nwith strlen, like:\n\n if (starts_with(foo, \"bar\"))\n\t foo += strlen(\"bar\");\n\nThis avoids magic numbers, but means we have to repeat the\nstring (and there is no compiler check that we didn't make a\ntypo in one of the strings).\n\nWe can use skip_prefix to handle this case without repeating\nourselves.\n\nSigned-off-by: Jeff King \n---\n builtin/checkout.c | 4 ++--\n builtin/fmt-merge-msg.c | 6 +++---\n builtin/log.c | 12 ++++++------\n diff.c | 27 +++++++++------------------\n help.c | 7 ++++---\n merge-recursive.c | 15 ++++++++-------\n pretty.c | 3 +--\n remote-curl.c | 15 ++++++++-------\n remote.c | 5 ++---\n sha1_name.c | 4 +---\n 10 files changed, 44 insertions(+), 54 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex f1dc56e..463cfee 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -776,8 +776,8 @@ static int switch_branches(const struct checkout_opts *opts,\n \tif (!(flag & REF_ISSYMREF))\n \t\told.path = NULL;\n \n-\tif (old.path && starts_with(old.path, \"refs/heads/\"))\n-\t\told.name = old.path + strlen(\"refs/heads/\");\n+\tif (old.path)\n+\t\tskip_prefix(old.path, \"refs/heads/\", &old.name);\n \n \tif (!new->name) {\n \t\tnew->name = \"HEAD\";\ndiff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c\nindex 3c19241..ad3bc58 100644\n--- a/builtin/fmt-merge-msg.c\n+++ b/builtin/fmt-merge-msg.c\n@@ -100,7 +100,8 @@ static int handle_line(char *line, struct merge_parents *merge_parents)\n {\n \tint i, len = strlen(line);\n \tstruct origin_data *origin_data;\n-\tchar *src, *origin;\n+\tchar *src;\n+\tconst char *origin;\n \tstruct src_data *src_data;\n \tstruct string_list_item *item;\n \tint pulling_head = 0;\n@@ -164,8 +165,7 @@ static int handle_line(char *line, struct merge_parents *merge_parents)\n \t\torigin = line;\n \t\tstring_list_append(&src_data->tag, origin + 4);\n \t\tsrc_data->head_status |= 2;\n-\t} else if (starts_with(line, \"remote-tracking branch \")) {\n-\t\torigin = line + strlen(\"remote-tracking branch \");\n+\t} else if (skip_prefix(line, \"remote-tracking branch \", &origin)) {\n \t\tstring_list_append(&src_data->r_branch, origin);\n \t\tsrc_data->head_status |= 2;\n \t} else {\ndiff --git a/builtin/log.c b/builtin/log.c\nindex a7ba211..0f59c25 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -872,7 +872,7 @@ static char *find_branch_name(struct rev_info *rev)\n \tint i, positive = -1;\n \tunsigned char branch_sha1[20];\n \tconst unsigned char *tip_sha1;\n-\tconst char *ref;\n+\tconst char *ref, *v;\n \tchar *full_ref, *branch = NULL;\n \n \tfor (i = 0; i < rev->cmdline.nr; i++) {\n@@ -888,9 +888,9 @@ static char *find_branch_name(struct rev_info *rev)\n \tref = rev->cmdline.rev[positive].name;\n \ttip_sha1 = rev->cmdline.rev[positive].item->sha1;\n \tif (dwim_ref(ref, strlen(ref), branch_sha1, &full_ref) &&\n-\t starts_with(full_ref, \"refs/heads/\") &&\n+\t skip_prefix(full_ref, \"refs/heads/\", &v) &&\n \t !hashcmp(tip_sha1, branch_sha1))\n-\t\tbranch = xstrdup(full_ref + strlen(\"refs/heads/\"));\n+\t\tbranch = xstrdup(v);\n \tfree(full_ref);\n \treturn branch;\n }\n@@ -1394,10 +1394,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \n \t\tif (check_head) {\n \t\t\tunsigned char sha1[20];\n-\t\t\tconst char *ref;\n+\t\t\tconst char *ref, *v;\n \t\t\tref = resolve_ref_unsafe(\"HEAD\", sha1, 1, NULL);\n-\t\t\tif (ref && starts_with(ref, \"refs/heads/\"))\n-\t\t\t\tbranch_name = xstrdup(ref + strlen(\"refs/heads/\"));\n+\t\t\tif (ref && skip_prefix(ref, \"refs/heads/\", &v))\n+\t\t\t\tbranch_name = xstrdup(v);\n \t\t\telse\n \t\t\t\tbranch_name = xstrdup(\"\"); /* no branch */\n \t\t}\ndiff --git a/diff.c b/diff.c\nindex 97db932..2378ae4 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3395,12 +3395,10 @@ int parse_long_opt(const char *opt, const char **argv,\n \t\t const char **optarg)\n {\n \tconst char *arg = argv[0];\n-\tif (arg[0] != '-' || arg[1] != '-')\n+\tif (!skip_prefix(arg, \"--\", &arg))\n \t\treturn 0;\n-\targ += strlen(\"--\");\n-\tif (!starts_with(arg, opt))\n+\tif (!skip_prefix(arg, opt, &arg))\n \t\treturn 0;\n-\targ += strlen(opt);\n \tif (*arg == '=') { /* stuck form: --option=value */\n \t\t*optarg = arg + 1;\n \t\treturn 1;\n@@ -3429,8 +3427,7 @@ static int stat_opt(struct diff_options *options, const char **av)\n \n \tswitch (*arg) {\n \tcase '-':\n-\t\tif (starts_with(arg, \"-width\")) {\n-\t\t\targ += strlen(\"-width\");\n+\t\tif (skip_prefix(arg, \"-width\", &arg)) {\n \t\t\tif (*arg == '=')\n \t\t\t\twidth = strtoul(arg + 1, &end, 10);\n \t\t\telse if (!*arg && !av[1])\n@@ -3439,8 +3436,7 @@ static int stat_opt(struct diff_options *options, const char **av)\n \t\t\t\twidth = strtoul(av[1], &end, 10);\n \t\t\t\targcount = 2;\n \t\t\t}\n-\t\t} else if (starts_with(arg, \"-name-width\")) {\n-\t\t\targ += strlen(\"-name-width\");\n+\t\t} else if (skip_prefix(arg, \"-name-width\", &arg)) {\n \t\t\tif (*arg == '=')\n \t\t\t\tname_width = strtoul(arg + 1, &end, 10);\n \t\t\telse if (!*arg && !av[1])\n@@ -3449,8 +3445,7 @@ static int stat_opt(struct diff_options *options, const char **av)\n \t\t\t\tname_width = strtoul(av[1], &end, 10);\n \t\t\t\targcount = 2;\n \t\t\t}\n-\t\t} else if (starts_with(arg, \"-graph-width\")) {\n-\t\t\targ += strlen(\"-graph-width\");\n+\t\t} else if (skip_prefix(arg, \"-graph-width\", &arg)) {\n \t\t\tif (*arg == '=')\n \t\t\t\tgraph_width = strtoul(arg + 1, &end, 10);\n \t\t\telse if (!*arg && !av[1])\n@@ -3459,8 +3454,7 @@ static int stat_opt(struct diff_options *options, const char **av)\n \t\t\t\tgraph_width = strtoul(av[1], &end, 10);\n \t\t\t\targcount = 2;\n \t\t\t}\n-\t\t} else if (starts_with(arg, \"-count\")) {\n-\t\t\targ += strlen(\"-count\");\n+\t\t} else if (skip_prefix(arg, \"-count\", &arg)) {\n \t\t\tif (*arg == '=')\n \t\t\t\tcount = strtoul(arg + 1, &end, 10);\n \t\t\telse if (!*arg && !av[1])\n@@ -3905,16 +3899,13 @@ static int diff_scoreopt_parse(const char *opt)\n \tcmd = *opt++;\n \tif (cmd == '-') {\n \t\t/* convert the long-form arguments into short-form versions */\n-\t\tif (starts_with(opt, \"break-rewrites\")) {\n-\t\t\topt += strlen(\"break-rewrites\");\n+\t\tif (skip_prefix(opt, \"break-rewrites\", &opt)) {\n \t\t\tif (*opt == 0 || *opt++ == '=')\n \t\t\t\tcmd = 'B';\n-\t\t} else if (starts_with(opt, \"find-copies\")) {\n-\t\t\topt += strlen(\"find-copies\");\n+\t\t} else if (skip_prefix(opt, \"find-copies\", &opt)) {\n \t\t\tif (*opt == 0 || *opt++ == '=')\n \t\t\t\tcmd = 'C';\n-\t\t} else if (starts_with(opt, \"find-renames\")) {\n-\t\t\topt += strlen(\"find-renames\");\n+\t\t} else if (skip_prefix(opt, \"find-renames\", &opt)) {\n \t\t\tif (*opt == 0 || *opt++ == '=')\n \t\t\t\tcmd = 'M';\n \t\t}\ndiff --git a/help.c b/help.c\nindex b0f1a69..79e8007 100644\n--- a/help.c\n+++ b/help.c\n@@ -414,11 +414,12 @@ static int append_similar_ref(const char *refname, const unsigned char *sha1,\n {\n \tstruct similar_ref_cb *cb = (struct similar_ref_cb *)(cb_data);\n \tchar *branch = strrchr(refname, '/') + 1;\n+\tconst char *remote;\n+\n \t/* A remote branch of the same name is deemed similar */\n-\tif (starts_with(refname, \"refs/remotes/\") &&\n+\tif (skip_prefix(refname, \"refs/remotes/\", &remote) &&\n \t !strcmp(branch, cb->base_ref))\n-\t\tstring_list_append(cb->similar_refs,\n-\t\t\t\t refname + strlen(\"refs/remotes/\"));\n+\t\tstring_list_append(cb->similar_refs, remote);\n \treturn 0;\n }\n \ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex cab16fa..f848001 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -2063,6 +2063,8 @@ void init_merge_options(struct merge_options *o)\n \n int parse_merge_opt(struct merge_options *o, const char *s)\n {\n+\tconst char *arg;\n+\n \tif (!s || !*s)\n \t\treturn -1;\n \tif (!strcmp(s, \"ours\"))\n@@ -2071,14 +2073,14 @@ int parse_merge_opt(struct merge_options *o, const char *s)\n \t\to->recursive_variant = MERGE_RECURSIVE_THEIRS;\n \telse if (!strcmp(s, \"subtree\"))\n \t\to->subtree_shift = \"\";\n-\telse if (starts_with(s, \"subtree=\"))\n-\t\to->subtree_shift = s + strlen(\"subtree=\");\n+\telse if (skip_prefix(s, \"subtree=\", &arg))\n+\t\to->subtree_shift = arg;\n \telse if (!strcmp(s, \"patience\"))\n \t\to->xdl_opts = DIFF_WITH_ALG(o, PATIENCE_DIFF);\n \telse if (!strcmp(s, \"histogram\"))\n \t\to->xdl_opts = DIFF_WITH_ALG(o, HISTOGRAM_DIFF);\n-\telse if (starts_with(s, \"diff-algorithm=\")) {\n-\t\tlong value = parse_algorithm_value(s + strlen(\"diff-algorithm=\"));\n+\telse if (skip_prefix(s, \"diff-algorithm=\", &arg)) {\n+\t\tlong value = parse_algorithm_value(arg);\n \t\tif (value < 0)\n \t\t\treturn -1;\n \t\t/* clear out previous settings */\n@@ -2096,9 +2098,8 @@ int parse_merge_opt(struct merge_options *o, const char *s)\n \t\to->renormalize = 1;\n \telse if (!strcmp(s, \"no-renormalize\"))\n \t\to->renormalize = 0;\n-\telse if (starts_with(s, \"rename-threshold=\")) {\n-\t\tconst char *score = s + strlen(\"rename-threshold=\");\n-\t\tif ((o->rename_score = parse_rename_score(&score)) == -1 || *score != 0)\n+\telse if (skip_prefix(s, \"rename-threshold=\", &arg)) {\n+\t\tif ((o->rename_score = parse_rename_score(&arg)) == -1 || *arg != 0)\n \t\t\treturn -1;\n \t}\n \telse\ndiff --git a/pretty.c b/pretty.c\nindex 4f51287..f24752a 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -40,10 +40,9 @@ static int git_pretty_formats_config(const char *var, const char *value, void *c\n \tconst char *fmt;\n \tint i;\n \n-\tif (!starts_with(var, \"pretty.\"))\n+\tif (!skip_prefix(var, \"pretty.\", &name))\n \t\treturn 0;\n \n-\tname = var + strlen(\"pretty.\");\n \tfor (i = 0; i < builtin_formats_len; i++) {\n \t\tif (!strcmp(commit_formats[i].name, name))\n \t\t\treturn 0;\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 4493b38..cdcca29 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -791,9 +791,9 @@ static void parse_fetch(struct strbuf *buf)\n \tint alloc_heads = 0, nr_heads = 0;\n \n \tdo {\n-\t\tif (starts_with(buf->buf, \"fetch \")) {\n-\t\t\tchar *p = buf->buf + strlen(\"fetch \");\n-\t\t\tchar *name;\n+\t\tconst char *p;\n+\t\tif (skip_prefix(buf->buf, \"fetch \", &p)) {\n+\t\t\tconst char *name;\n \t\t\tstruct ref *ref;\n \t\t\tunsigned char old_sha1[20];\n \n@@ -968,6 +968,8 @@ int main(int argc, const char **argv)\n \thttp_init(remote, url.buf, 0);\n \n \tdo {\n+\t\tconst char *arg;\n+\n \t\tif (strbuf_getline(&buf, stdin, '\\n') == EOF) {\n \t\t\tif (ferror(stdin))\n \t\t\t\tfprintf(stderr, \"Error reading command stream\\n\");\n@@ -989,9 +991,8 @@ int main(int argc, const char **argv)\n \t\t} else if (starts_with(buf.buf, \"push \")) {\n \t\t\tparse_push(&buf);\n \n-\t\t} else if (starts_with(buf.buf, \"option \")) {\n-\t\t\tchar *name = buf.buf + strlen(\"option \");\n-\t\t\tchar *value = strchr(name, ' ');\n+\t\t} else if (skip_prefix(buf.buf, \"option \", &arg)) {\n+\t\t\tchar *value = strchr(arg, ' ');\n \t\t\tint result;\n \n \t\t\tif (value)\n@@ -999,7 +1000,7 @@ int main(int argc, const char **argv)\n \t\t\telse\n \t\t\t\tvalue = \"true\";\n \n-\t\t\tresult = set_option(name, value);\n+\t\t\tresult = set_option(arg, value);\n \t\t\tif (!result)\n \t\t\t\tprintf(\"ok\\n\");\n \t\t\telse if (result < 0)\ndiff --git a/remote.c b/remote.c\nindex 0e9459c..30d2829 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -488,9 +488,8 @@ static void read_config(void)\n \tcurrent_branch = NULL;\n \thead_ref = resolve_ref_unsafe(\"HEAD\", sha1, 0, &flag);\n \tif (head_ref && (flag & REF_ISSYMREF) &&\n-\t starts_with(head_ref, \"refs/heads/\")) {\n-\t\tcurrent_branch =\n-\t\t\tmake_branch(head_ref + strlen(\"refs/heads/\"), 0);\n+\t skip_prefix(head_ref, \"refs/heads/\", &head_ref)) {\n+\t\tcurrent_branch = make_branch(head_ref, 0);\n \t}\n \tgit_config(handle_config, NULL);\n \tif (branch_pushremote_name) {\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 2b6322f..72e6ac6 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -911,10 +911,8 @@ static int grab_nth_branch_switch(unsigned char *osha1, unsigned char *nsha1,\n \tconst char *match = NULL, *target = NULL;\n \tsize_t len;\n \n-\tif (starts_with(message, \"checkout: moving from \")) {\n-\t\tmatch = message + strlen(\"checkout: moving from \");\n+\tif (skip_prefix(message, \"checkout: moving from \", &match))\n \t\ttarget = strstr(match, \" to \");\n-\t}\n \n \tif (!match || !target)\n \t\treturn 0;\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244571","messageId":"20140618194912.GJ22622@sigill.intra.peff.net","threadId":"36953","inReplyTo":"20140618194117.GA22269@sigill.intra.peff.net","subject":"[PATCH 10/16] fast-import: use skip_prefix for parsing input","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-18T19:49:12Z","receivedAt":"2014-06-18T19:49:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Fast-import does a lot of parsing of commands and\ndispatching to sub-functions. For example, given \"option\nfoo\", we might recognize \"option \" using starts_with, and\nthen hand it off to parse_option() to do the rest.\n\nHowever, we do not let parse_option know that we have parsed\nthe first part already. It gets the full buffer, and has to\nskip past the uninteresting bits. Some functions simply add\na magic constant:\n\n char *option = command_buf.buf + 7;\n\nOthers use strlen:\n\n char *option = command_buf.buf + strlen(\"option \");\n\nAnd others use strchr:\n\n char *option = strchr(command_buf.buf, ' ') + 1;\n\nAll of these are brittle and easy to get wrong (especially\ngiven that the starts_with call and the code that assumes\nthe presence of the prefix are far apart). Instead, we can\nuse skip_prefix, and just pass each handler a pointer to its\narguments.\n\nSigned-off-by: Jeff King \n---\n fast-import.c | 125 +++++++++++++++++++++++++---------------------------------\n 1 file changed, 53 insertions(+), 72 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex a3ffe30..5f17adb 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -371,8 +371,8 @@ static volatile sig_atomic_t checkpoint_requested;\n static int cat_blob_fd = STDOUT_FILENO;\n \n static void parse_argv(void);\n-static void parse_cat_blob(void);\n-static void parse_ls(struct branch *b);\n+static void parse_cat_blob(const char *p);\n+static void parse_ls(const char *p, struct branch *b);\n \n static void write_branch_report(FILE *rpt, struct branch *b)\n {\n@@ -1861,6 +1861,8 @@ static int read_next_command(void)\n \t}\n \n \tfor (;;) {\n+\t\tconst char *p;\n+\n \t\tif (unread_command_buf) {\n \t\t\tunread_command_buf = 0;\n \t\t} else {\n@@ -1893,8 +1895,8 @@ static int read_next_command(void)\n \t\t\trc->prev->next = rc;\n \t\t\tcmd_tail = rc;\n \t\t}\n-\t\tif (starts_with(command_buf.buf, \"cat-blob \")) {\n-\t\t\tparse_cat_blob();\n+\t\tif (skip_prefix(command_buf.buf, \"cat-blob \", &p)) {\n+\t\t\tparse_cat_blob(p);\n \t\t\tcontinue;\n \t\t}\n \t\tif (command_buf.buf[0] == '#')\n@@ -2273,9 +2275,8 @@ static uintmax_t parse_mark_ref_space(const char **p)\n \treturn mark;\n }\n \n-static void file_change_m(struct branch *b)\n+static void file_change_m(const char *p, struct branch *b)\n {\n-\tconst char *p = command_buf.buf + 2;\n \tstatic struct strbuf uq = STRBUF_INIT;\n \tconst char *endp;\n \tstruct object_entry *oe;\n@@ -2376,9 +2377,8 @@ static void file_change_m(struct branch *b)\n \ttree_content_set(&b->branch_tree, p, sha1, mode, NULL);\n }\n \n-static void file_change_d(struct branch *b)\n+static void file_change_d(const char *p, struct branch *b)\n {\n-\tconst char *p = command_buf.buf + 2;\n \tstatic struct strbuf uq = STRBUF_INIT;\n \tconst char *endp;\n \n@@ -2391,15 +2391,14 @@ static void file_change_d(struct branch *b)\n \ttree_content_remove(&b->branch_tree, p, NULL, 1);\n }\n \n-static void file_change_cr(struct branch *b, int rename)\n+static void file_change_cr(const char *s, struct branch *b, int rename)\n {\n-\tconst char *s, *d;\n+\tconst char *d;\n \tstatic struct strbuf s_uq = STRBUF_INIT;\n \tstatic struct strbuf d_uq = STRBUF_INIT;\n \tconst char *endp;\n \tstruct tree_entry leaf;\n \n-\ts = command_buf.buf + 2;\n \tstrbuf_reset(&s_uq);\n \tif (!unquote_c_style(&s_uq, s, &endp)) {\n \t\tif (*endp != ' ')\n@@ -2444,9 +2443,8 @@ static void file_change_cr(struct branch *b, int rename)\n \t\tleaf.tree);\n }\n \n-static void note_change_n(struct branch *b, unsigned char *old_fanout)\n+static void note_change_n(const char *p, struct branch *b, unsigned char *old_fanout)\n {\n-\tconst char *p = command_buf.buf + 2;\n \tstatic struct strbuf uq = STRBUF_INIT;\n \tstruct object_entry *oe;\n \tstruct branch *s;\n@@ -2587,7 +2585,7 @@ static int parse_from(struct branch *b)\n \tconst char *from;\n \tstruct branch *s;\n \n-\tif (!starts_with(command_buf.buf, \"from \"))\n+\tif (!skip_prefix(command_buf.buf, \"from \", &from))\n \t\treturn 0;\n \n \tif (b->branch_tree.tree) {\n@@ -2595,7 +2593,6 @@ static int parse_from(struct branch *b)\n \t\tb->branch_tree.tree = NULL;\n \t}\n \n-\tfrom = strchr(command_buf.buf, ' ') + 1;\n \ts = lookup_branch(from);\n \tif (b == s)\n \t\tdie(\"Can't create a branch from itself: %s\", b->name);\n@@ -2636,8 +2633,7 @@ static struct hash_list *parse_merge(unsigned int *count)\n \tstruct branch *s;\n \n \t*count = 0;\n-\twhile (starts_with(command_buf.buf, \"merge \")) {\n-\t\tfrom = strchr(command_buf.buf, ' ') + 1;\n+\twhile (skip_prefix(command_buf.buf, \"merge \", &from)) {\n \t\tn = xmalloc(sizeof(*n));\n \t\ts = lookup_branch(from);\n \t\tif (s)\n@@ -2668,11 +2664,10 @@ static struct hash_list *parse_merge(unsigned int *count)\n \treturn list;\n }\n \n-static void parse_new_commit(void)\n+static void parse_new_commit(const char *arg)\n {\n \tstatic struct strbuf msg = STRBUF_INIT;\n \tstruct branch *b;\n-\tchar *sp;\n \tchar *author = NULL;\n \tchar *committer = NULL;\n \tstruct hash_list *merge_list = NULL;\n@@ -2680,11 +2675,9 @@ static void parse_new_commit(void)\n \tunsigned char prev_fanout, new_fanout;\n \tconst char *v;\n \n-\t/* Obtain the branch name from the rest of our command */\n-\tsp = strchr(command_buf.buf, ' ') + 1;\n-\tb = lookup_branch(sp);\n+\tb = lookup_branch(arg);\n \tif (!b)\n-\t\tb = new_branch(sp);\n+\t\tb = new_branch(arg);\n \n \tread_next_command();\n \tparse_mark();\n@@ -2713,20 +2706,22 @@ static void parse_new_commit(void)\n \n \t/* file_change* */\n \twhile (command_buf.len > 0) {\n-\t\tif (starts_with(command_buf.buf, \"M \"))\n-\t\t\tfile_change_m(b);\n-\t\telse if (starts_with(command_buf.buf, \"D \"))\n-\t\t\tfile_change_d(b);\n-\t\telse if (starts_with(command_buf.buf, \"R \"))\n-\t\t\tfile_change_cr(b, 1);\n-\t\telse if (starts_with(command_buf.buf, \"C \"))\n-\t\t\tfile_change_cr(b, 0);\n-\t\telse if (starts_with(command_buf.buf, \"N \"))\n-\t\t\tnote_change_n(b, &prev_fanout);\n+\t\tconst char *v;\n+\n+\t\tif (skip_prefix(command_buf.buf, \"M \", &v))\n+\t\t\tfile_change_m(v, b);\n+\t\telse if (skip_prefix(command_buf.buf, \"D \", &v))\n+\t\t\tfile_change_d(v, b);\n+\t\telse if (skip_prefix(command_buf.buf, \"R \", &v))\n+\t\t\tfile_change_cr(v, b, 1);\n+\t\telse if (skip_prefix(command_buf.buf, \"C \", &v))\n+\t\t\tfile_change_cr(v, b, 0);\n+\t\telse if (skip_prefix(command_buf.buf, \"N \", &v))\n+\t\t\tnote_change_n(v, b, &prev_fanout);\n \t\telse if (!strcmp(\"deleteall\", command_buf.buf))\n \t\t\tfile_change_deleteall(b);\n-\t\telse if (starts_with(command_buf.buf, \"ls \"))\n-\t\t\tparse_ls(b);\n+\t\telse if (skip_prefix(command_buf.buf, \"ls \", &v))\n+\t\t\tparse_ls(v, b);\n \t\telse {\n \t\t\tunread_command_buf = 1;\n \t\t\tbreak;\n@@ -2769,10 +2764,9 @@ static void parse_new_commit(void)\n \tb->last_commit = object_count_by_type[OBJ_COMMIT];\n }\n \n-static void parse_new_tag(void)\n+static void parse_new_tag(const char *arg)\n {\n \tstatic struct strbuf msg = STRBUF_INIT;\n-\tchar *sp;\n \tconst char *from;\n \tchar *tagger;\n \tstruct branch *s;\n@@ -2782,11 +2776,9 @@ static void parse_new_tag(void)\n \tenum object_type type;\n \tconst char *v;\n \n-\t/* Obtain the new tag name from the rest of our command */\n-\tsp = strchr(command_buf.buf, ' ') + 1;\n \tt = pool_alloc(sizeof(struct tag));\n \tmemset(t, 0, sizeof(struct tag));\n-\tt->name = pool_strdup(sp);\n+\tt->name = pool_strdup(arg);\n \tif (last_tag)\n \t\tlast_tag->next_tag = t;\n \telse\n@@ -2795,9 +2787,8 @@ static void parse_new_tag(void)\n \tread_next_command();\n \n \t/* from ... */\n-\tif (!starts_with(command_buf.buf, \"from \"))\n+\tif (!skip_prefix(command_buf.buf, \"from \", &from))\n \t\tdie(\"Expected from command, got %s\", command_buf.buf);\n-\tfrom = strchr(command_buf.buf, ' ') + 1;\n \ts = lookup_branch(from);\n \tif (s) {\n \t\tif (is_null_sha1(s->sha1))\n@@ -2853,14 +2844,11 @@ static void parse_new_tag(void)\n \t\tt->pack_id = pack_id;\n }\n \n-static void parse_reset_branch(void)\n+static void parse_reset_branch(const char *arg)\n {\n \tstruct branch *b;\n-\tchar *sp;\n \n-\t/* Obtain the branch name from the rest of our command */\n-\tsp = strchr(command_buf.buf, ' ') + 1;\n-\tb = lookup_branch(sp);\n+\tb = lookup_branch(arg);\n \tif (b) {\n \t\thashclr(b->sha1);\n \t\thashclr(b->branch_tree.versions[0].sha1);\n@@ -2871,7 +2859,7 @@ static void parse_reset_branch(void)\n \t\t}\n \t}\n \telse\n-\t\tb = new_branch(sp);\n+\t\tb = new_branch(arg);\n \tread_next_command();\n \tparse_from(b);\n \tif (command_buf.len > 0)\n@@ -2929,14 +2917,12 @@ static void cat_blob(struct object_entry *oe, unsigned char sha1[20])\n \t\tfree(buf);\n }\n \n-static void parse_cat_blob(void)\n+static void parse_cat_blob(const char *p)\n {\n-\tconst char *p;\n \tstruct object_entry *oe = oe;\n \tunsigned char sha1[20];\n \n \t/* cat-blob SP LF */\n-\tp = command_buf.buf + strlen(\"cat-blob \");\n \tif (*p == ':') {\n \t\toe = find_mark(parse_mark_ref_eol(p));\n \t\tif (!oe)\n@@ -3053,14 +3039,12 @@ static void print_ls(int mode, const unsigned char *sha1, const char *path)\n \tcat_blob_write(line.buf, line.len);\n }\n \n-static void parse_ls(struct branch *b)\n+static void parse_ls(const char *p, struct branch *b)\n {\n-\tconst char *p;\n \tstruct tree_entry *root = NULL;\n \tstruct tree_entry leaf = {NULL};\n \n \t/* ls SP ( SP)? */\n-\tp = command_buf.buf + strlen(\"ls \");\n \tif (*p == '\"') {\n \t\tif (!b)\n \t\t\tdie(\"Not in a commit: %s\", command_buf.buf);\n@@ -3276,10 +3260,8 @@ static int parse_one_feature(const char *feature, int from_stream)\n \treturn 1;\n }\n \n-static void parse_feature(void)\n+static void parse_feature(const char *feature)\n {\n-\tchar *feature = command_buf.buf + 8;\n-\n \tif (seen_data_command)\n \t\tdie(\"Got feature command '%s' after data command\", feature);\n \n@@ -3289,10 +3271,8 @@ static void parse_feature(void)\n \tdie(\"This version of fast-import does not support feature %s.\", feature);\n }\n \n-static void parse_option(void)\n+static void parse_option(const char *option)\n {\n-\tchar *option = command_buf.buf + 11;\n-\n \tif (seen_data_command)\n \t\tdie(\"Got option command '%s' after data command\", option);\n \n@@ -3408,26 +3388,27 @@ int main(int argc, char **argv)\n \tset_die_routine(die_nicely);\n \tset_checkpoint_signal();\n \twhile (read_next_command() != EOF) {\n+\t\tconst char *v;\n \t\tif (!strcmp(\"blob\", command_buf.buf))\n \t\t\tparse_new_blob();\n-\t\telse if (starts_with(command_buf.buf, \"ls \"))\n-\t\t\tparse_ls(NULL);\n-\t\telse if (starts_with(command_buf.buf, \"commit \"))\n-\t\t\tparse_new_commit();\n-\t\telse if (starts_with(command_buf.buf, \"tag \"))\n-\t\t\tparse_new_tag();\n-\t\telse if (starts_with(command_buf.buf, \"reset \"))\n-\t\t\tparse_reset_branch();\n+\t\telse if (skip_prefix(command_buf.buf, \"ls \", &v))\n+\t\t\tparse_ls(v, NULL);\n+\t\telse if (skip_prefix(command_buf.buf, \"commit \", &v))\n+\t\t\tparse_new_commit(v);\n+\t\telse if (skip_prefix(command_buf.buf, \"tag \", &v))\n+\t\t\tparse_new_tag(v);\n+\t\telse if (skip_prefix(command_buf.buf, \"reset \", &v))\n+\t\t\tparse_reset_branch(v);\n \t\telse if (!strcmp(\"checkpoint\", command_buf.buf))\n \t\t\tparse_checkpoint();\n \t\telse if (!strcmp(\"done\", command_buf.buf))\n \t\t\tbreak;\n \t\telse if (starts_with(command_buf.buf, \"progress \"))\n \t\t\tparse_progress();\n-\t\telse if (starts_with(command_buf.buf, \"feature \"))\n-\t\t\tparse_feature();\n-\t\telse if (starts_with(command_buf.buf, \"option git \"))\n-\t\t\tparse_option();\n+\t\telse if (skip_prefix(command_buf.buf, \"feature \", &v))\n+\t\t\tparse_feature(v);\n+\t\telse if (skip_prefix(command_buf.buf, \"option git \", &v))\n+\t\t\tparse_option(v);\n \t\telse if (starts_with(command_buf.buf, \"option \"))\n \t\t\t/* ignore non-git options*/;\n \t\telse\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244572","messageId":"20140618194944.GK22622@sigill.intra.peff.net","threadId":"36953","inReplyTo":"20140618194117.GA22269@sigill.intra.peff.net","subject":"[PATCH 11/16] daemon: use skip_prefix to avoid magic numbers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-18T19:49:44Z","receivedAt":"2014-06-18T19:49:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Like earlier cases, we can use skip_prefix to avoid magic\nnumbers that must match the length of starts_with prefixes.\nHowever, the numbers are a little more complicated here, as\nwe keep parsing past the prefix. We can solve it by keeping\na running pointer as we parse; its final value is the\nlocation we want.\n\nSigned-off-by: Jeff King \n---\n daemon.c | 11 ++++++-----\n 1 file changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex 6d25828..1eb6631 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -626,15 +626,16 @@ static int execute(void)\n \n \tfor (i = 0; i < ARRAY_SIZE(daemon_service); i++) {\n \t\tstruct daemon_service *s = &(daemon_service[i]);\n-\t\tint namelen = strlen(s->name);\n-\t\tif (starts_with(line, \"git-\") &&\n-\t\t !strncmp(s->name, line + 4, namelen) &&\n-\t\t line[namelen + 4] == ' ') {\n+\t\tconst char *arg;\n+\n+\t\tif (skip_prefix(line, \"git-\", &arg) &&\n+\t\t skip_prefix(arg, s->name, &arg) &&\n+\t\t *arg++ == ' ') {\n \t\t\t/*\n \t\t\t * Note: The directory here is probably context sensitive,\n \t\t\t * and might depend on the actual service being performed.\n \t\t\t */\n-\t\t\treturn run_service(line + namelen + 5, s);\n+\t\t\treturn run_service(arg, s);\n \t\t}\n \t}\n \n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244573","messageId":"20140618195117.GL22622@sigill.intra.peff.net","threadId":"36953","inReplyTo":"20140618194117.GA22269@sigill.intra.peff.net","subject":"[PATCH 12/16] stat_opt: check extra strlen call","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-18T19:51:17Z","receivedAt":"2014-06-18T19:51:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"As in earlier commits, the diff option parser uses\nstarts_with to find that an argument starts with \"--stat-\",\nand then adds strlen(\"stat-\") to find the rest of the\noption.\n\nHowever, in this case the starts_with and the strlen are\nseparated across functions, making it easy to call the\nlatter without the former. Let's use skip_prefix instead of\nraw pointer arithmetic to catch such a case.\n\nSigned-off-by: Jeff King \n---\nAnother possibility would be for stat_opt to take only the\nprefix-skipped part of the string. But that would involve refactoring\nits use of \"av\" (it needs the whole array because it may need to grab a\nfollow-on argument).\n\n diff.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex 2378ae4..06bdfb8 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3422,7 +3422,8 @@ static int stat_opt(struct diff_options *options, const char **av)\n \tint count = options->stat_count;\n \tint argcount = 1;\n \n-\targ += strlen(\"--stat\");\n+\tif (!skip_prefix(arg, \"--stat\", &arg))\n+\t\tdie(\"BUG: stat option does not begin with --stat: %s\", arg);\n \tend = (char *)arg;\n \n \tswitch (*arg) {\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244574","messageId":"20140618195157.GM22622@sigill.intra.peff.net","threadId":"36953","inReplyTo":"20140618194117.GA22269@sigill.intra.peff.net","subject":"[PATCH 13/16] fast-import: refactor parsing of spaces","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-18T19:51:57Z","receivedAt":"2014-06-18T19:51:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we see a file change in a commit, we expect one of:\n\n 1. A mark.\n\n 2. An \"inline\" keyword.\n\n 3. An object sha1.\n\nThe handling of spaces is inconsistent between the three\noptions. Option 1 calls a sub-function which checks for the\nspace, but doesn't parse past it. Option 2 parses the space,\nthen deliberately avoids moving the pointer past it. Option\n3 detects the space locally but doesn't move past it.\n\nThis is confusing, because it looks like option 1 forgets to\ncheck for the space (it's just buried). And option 2 checks\nfor \"inline \", but only moves strlen(\"inline\") characters\nforward, which looks like a bug but isn't.\n\nWe can make this more clear by just having each branch move\npast the space as it is checked (and we can replace the\ndoubled use of \"inline\" with a call to skip_prefix).\n\nSigned-off-by: Jeff King \n---\n fast-import.c | 20 +++++++-------------\n 1 file changed, 7 insertions(+), 13 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 5f17adb..55ca7d8 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -2269,7 +2269,7 @@ static uintmax_t parse_mark_ref_space(const char **p)\n \tchar *end;\n \n \tmark = parse_mark_ref(*p, &end);\n-\tif (*end != ' ')\n+\tif (*end++ != ' ')\n \t\tdie(\"Missing space after mark: %s\", command_buf.buf);\n \t*p = end;\n \treturn mark;\n@@ -2304,20 +2304,17 @@ static void file_change_m(const char *p, struct branch *b)\n \tif (*p == ':') {\n \t\toe = find_mark(parse_mark_ref_space(&p));\n \t\thashcpy(sha1, oe->idx.sha1);\n-\t} else if (starts_with(p, \"inline \")) {\n+\t} else if (skip_prefix(p, \"inline \", &p)) {\n \t\tinline_data = 1;\n \t\toe = NULL; /* not used with inline_data, but makes gcc happy */\n-\t\tp += strlen(\"inline\"); /* advance to space */\n \t} else {\n \t\tif (get_sha1_hex(p, sha1))\n \t\t\tdie(\"Invalid dataref: %s\", command_buf.buf);\n \t\toe = find_object(sha1);\n \t\tp += 40;\n-\t\tif (*p != ' ')\n+\t\tif (*p++ != ' ')\n \t\t\tdie(\"Missing space after SHA1: %s\", command_buf.buf);\n \t}\n-\tassert(*p == ' ');\n-\tp++; /* skip space */\n \n \tstrbuf_reset(&uq);\n \tif (!unquote_c_style(&uq, p, &endp)) {\n@@ -2474,20 +2471,17 @@ static void note_change_n(const char *p, struct branch *b, unsigned char *old_fa\n \tif (*p == ':') {\n \t\toe = find_mark(parse_mark_ref_space(&p));\n \t\thashcpy(sha1, oe->idx.sha1);\n-\t} else if (starts_with(p, \"inline \")) {\n+\t} else if (skip_prefix(p, \"inline \", &p)) {\n \t\tinline_data = 1;\n \t\toe = NULL; /* not used with inline_data, but makes gcc happy */\n-\t\tp += strlen(\"inline\"); /* advance to space */\n \t} else {\n \t\tif (get_sha1_hex(p, sha1))\n \t\t\tdie(\"Invalid dataref: %s\", command_buf.buf);\n \t\toe = find_object(sha1);\n \t\tp += 40;\n-\t\tif (*p != ' ')\n+\t\tif (*p++ != ' ')\n \t\t\tdie(\"Missing space after SHA1: %s\", command_buf.buf);\n \t}\n-\tassert(*p == ' ');\n-\tp++; /* skip space */\n \n \t/* */\n \ts = lookup_branch(p);\n@@ -3005,6 +2999,8 @@ static struct object_entry *parse_treeish_dataref(const char **p)\n \t\t\tdie(\"Invalid dataref: %s\", command_buf.buf);\n \t\te = find_object(sha1);\n \t\t*p += 40;\n+\t\tif (*(*p)++ != ' ')\n+\t\t\tdie(\"Missing space after tree-ish: %s\", command_buf.buf);\n \t}\n \n \twhile (!e || e->type != OBJ_TREE)\n@@ -3056,8 +3052,6 @@ static void parse_ls(const char *p, struct branch *b)\n \t\tif (!is_null_sha1(root->versions[1].sha1))\n \t\t\troot->versions[1].mode = S_IFDIR;\n \t\tload_tree(root);\n-\t\tif (*p++ != ' ')\n-\t\t\tdie(\"Missing space after tree-ish: %s\", command_buf.buf);\n \t}\n \tif (*p == '\"') {\n \t\tstatic struct strbuf uq = STRBUF_INIT;\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244576","messageId":"20140618195603.GN22622@sigill.intra.peff.net","threadId":"36953","inReplyTo":"20140618194117.GA22269@sigill.intra.peff.net","subject":"[PATCH 14/16] fetch-pack: refactor parsing in get_ack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-18T19:56:03Z","receivedAt":"2014-06-18T19:56:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There are several uses of the magic number \"line+45\" when\nparsing ACK lines from the server, and it's rather unclear\nwhy 45 is the correct number. We can make this more clear by\nkeeping a running pointer as we parse, using skip_prefix to\njump past the first \"ACK \", then adding 40 to jump past\nget_sha1_hex (which is still magical, but hopefully 40 is\nless magical to readers of git code).\n\nNote that this actually puts us at line+44. The original\nrequired some character between the sha1 and further ACK\nflags (it is supposed to be a space, but we never enforced\nthat). We start our search for flags at line+44, which\nmeanas we are slightly more liberal than the old code.\n\nSigned-off-by: Jeff King \n---\nI actually think we could tighten this even more and drop the strstrs,\ntoo, like:\n\n arg += 40;\n if (*arg++ != ' ')\n\treturn ACK;\n if (!strcmp(arg, \"continue\"))\n\treturn ACK_continue;\n\nand so on. But I wasn't sure if there was a reason for the use of\nstrstr. According to pack-protocol.txt, we would only get one at a time,\nand always with a single space between them.\n\n fetch-pack.c | 15 +++++++++------\n 1 file changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 3de3bd5..72ec520 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -189,20 +189,23 @@ static enum ack_type get_ack(int fd, unsigned char *result_sha1)\n {\n \tint len;\n \tchar *line = packet_read_line(fd, &len);\n+\tconst char *arg;\n \n \tif (!len)\n \t\tdie(\"git fetch-pack: expected ACK/NAK, got EOF\");\n \tif (!strcmp(line, \"NAK\"))\n \t\treturn NAK;\n-\tif (starts_with(line, \"ACK \")) {\n-\t\tif (!get_sha1_hex(line+4, result_sha1)) {\n-\t\t\tif (len < 45)\n+\tif (skip_prefix(line, \"ACK \", &arg)) {\n+\t\tif (!get_sha1_hex(arg, result_sha1)) {\n+\t\t\targ += 40;\n+\t\t\tlen -= arg - line;\n+\t\t\tif (len < 1)\n \t\t\t\treturn ACK;\n-\t\t\tif (strstr(line+45, \"continue\"))\n+\t\t\tif (strstr(arg, \"continue\"))\n \t\t\t\treturn ACK_continue;\n-\t\t\tif (strstr(line+45, \"common\"))\n+\t\t\tif (strstr(arg, \"common\"))\n \t\t\t\treturn ACK_common;\n-\t\t\tif (strstr(line+45, \"ready\"))\n+\t\t\tif (strstr(arg, \"ready\"))\n \t\t\t\treturn ACK_ready;\n \t\t\treturn ACK;\n \t\t}\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244577","messageId":"20140618195648.GO22622@sigill.intra.peff.net","threadId":"36953","inReplyTo":"20140618194117.GA22269@sigill.intra.peff.net","subject":"[PATCH 15/16] git: avoid magic number with skip_prefix","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-18T19:56:48Z","receivedAt":"2014-06-18T19:56:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"After handling options, any leftover arguments should be\ncommands. However, we pass through \"--help\" and \"--version\",\nso that we convert them into \"git help\" and \"git version\"\nrespectively.\n\nThis is a straightforward use of skip_prefix to avoid a\nmagic number, but while we are there, it is worth adding a\ncomment to explain this otherwise confusing behavior.\n\nSigned-off-by: Jeff King \n---\nAnother option would be to teach handle_options to convert \"--version\"\ninto \"version\" itself. That's more disruptive, but I think would be\nless confusing.\n\n git.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex b2bb09e..1537f00 100644\n--- a/git.c\n+++ b/git.c\n@@ -588,8 +588,8 @@ int main(int argc, char **av)\n \targc--;\n \thandle_options(&argv, &argc, NULL);\n \tif (argc > 0) {\n-\t\tif (starts_with(argv[0], \"--\"))\n-\t\t\targv[0] += 2;\n+\t\t/* translate --help and --version into commands */\n+\t\tskip_prefix(argv[0], \"--\", &argv[0]);\n \t} else {\n \t\t/* The user didn't specify a command; give them help */\n \t\tcommit_pager_choice();\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244578","messageId":"20140618195717.GP22622@sigill.intra.peff.net","threadId":"36953","inReplyTo":"20140618194117.GA22269@sigill.intra.peff.net","subject":"[PATCH 16/16] use skip_prefix to avoid repeated calculations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-18T19:57:17Z","receivedAt":"2014-06-18T19:57:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In some cases, we use starts_with to check for a prefix, and\nthen use an already-calculated prefix length to advance a\npointer past the prefix. There are no magic numbers or\nduplicated strings here, but we can still make the code\nsimpler and more obvious by using skip_prefix.\n\nSigned-off-by: Jeff King \n---\n help.c | 11 +++++------\n http.c | 3 +--\n 2 files changed, 6 insertions(+), 8 deletions(-)\n\ndiff --git a/help.c b/help.c\nindex 79e8007..f31f29a 100644\n--- a/help.c\n+++ b/help.c\n@@ -129,7 +129,6 @@ static void list_commands_in_dir(struct cmdnames *cmds,\n \t\t\t\t\t const char *path,\n \t\t\t\t\t const char *prefix)\n {\n-\tint prefix_len;\n \tDIR *dir = opendir(path);\n \tstruct dirent *de;\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -139,15 +138,15 @@ static void list_commands_in_dir(struct cmdnames *cmds,\n \t\treturn;\n \tif (!prefix)\n \t\tprefix = \"git-\";\n-\tprefix_len = strlen(prefix);\n \n \tstrbuf_addf(&buf, \"%s/\", path);\n \tlen = buf.len;\n \n \twhile ((de = readdir(dir)) != NULL) {\n+\t\tconst char *ent;\n \t\tint entlen;\n \n-\t\tif (!starts_with(de->d_name, prefix))\n+\t\tif (!skip_prefix(de->d_name, prefix, &ent))\n \t\t\tcontinue;\n \n \t\tstrbuf_setlen(&buf, len);\n@@ -155,11 +154,11 @@ static void list_commands_in_dir(struct cmdnames *cmds,\n \t\tif (!is_executable(buf.buf))\n \t\t\tcontinue;\n \n-\t\tentlen = strlen(de->d_name) - prefix_len;\n-\t\tif (has_extension(de->d_name, \".exe\"))\n+\t\tentlen = strlen(ent);\n+\t\tif (has_extension(ent, \".exe\"))\n \t\t\tentlen -= 4;\n \n-\t\tadd_cmdname(cmds, de->d_name + prefix_len, entlen);\n+\t\tadd_cmdname(cmds, ent, entlen);\n \t}\n \tclosedir(dir);\n \tstrbuf_release(&buf);\ndiff --git a/http.c b/http.c\nindex 2b4f6a3..f04621e 100644\n--- a/http.c\n+++ b/http.c\n@@ -1087,11 +1087,10 @@ static int update_url_from_redirect(struct strbuf *base,\n \tif (!strcmp(asked, got->buf))\n \t\treturn 0;\n \n-\tif (!starts_with(asked, base->buf))\n+\tif (!skip_prefix(asked, base->buf, &tail))\n \t\tdie(\"BUG: update_url_from_redirect: %s is not a superset of %s\",\n \t\t asked, base->buf);\n \n-\ttail = asked + base->len;\n \ttail_len = strlen(tail);\n \n \tif (got->len < tail_len ||\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244625","messageId":"loom.20140619T065448-710@post.gmane.org","threadId":"36953","inReplyTo":"20140618194117.GA22269@sigill.intra.peff.net","subject":"Re: [PATCH 0/16] skip_prefix refactoring and cleanups","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-19T05:22:46Z","receivedAt":"2014-06-19T05:22:46Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Hi,\n\nI was about to send a patch like this series today, I am attaching it below.\n\n-- >8 --\nSubject: [PATCH] imap-send: use skip_prefix instead of using magic numbers\n\nSigned-off-by: Tanay Abhra \n---\n imap-send.c | 6 +-----\n 1 file changed, 1 insertion(+), 5 deletions(-)\n\ndiff --git a/imap-send.c b/imap-send.c\nindex 83a6ed2..524fbab 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -1328,13 +1328,9 @@ static char *imap_folder;\n \n static int git_imap_config(const char *key, const char *val, void *cb)\n {\n-\tchar imap_key[] = \"imap.\";\n-\n-\tif (strncmp(key, imap_key, sizeof imap_key - 1))\n+\tif (!skip_prefix(key, \"imap.\", &key))\n \t\treturn 0;\n \n-\tkey += sizeof imap_key - 1;\n-\n \t/* check booleans first, and barf on others */\n \tif (!strcmp(\"sslverify\", key))\n \t\tserver.ssl_verify = git_config_bool(key, val);\n-- \n1.9.0.GIT\n"},{"id":"244725","messageId":"20140619215810.GA29685@sigill.intra.peff.net","threadId":"36953","inReplyTo":"20140618194117.GA22269@sigill.intra.peff.net","subject":"[PATCH 17/16] http-push: refactor parsing of remote object names","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-19T21:58:10Z","receivedAt":"2014-06-19T21:58:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We get loose object names like \"objects/??/...\" from the\nremote side, and need to convert them to their hex\nrepresentation.\n\nThe code to do so is rather hard to follow, as it uses some\ncalculated lengths whose origins are hard to understand and\nverify (e.g., the path must be exactly 49 characters long.\nwhy? Why doesn't the strcpy overflow obj_hex, which is the\nsame length as path?).\n\nWe can simplify this a bit by using skip_prefix, using standard\n40- and 20-character buffers for hex and binary sha1s, and\nadding some comments.\n\nWe also drop a totally bogus comment that claims strlcpy\ncannot be used because \"path\" is not NUL-terminated. Right\nbetween a call to strlen(path) and strcpy(path).\n\nSigned-off-by: Jeff King \n---\nI found this one while doing the xstrfmt series, but it actually doesn't\nneed xstrfmt, and does need skip_prefix, so it probably goes better on\nthe skip-prefix topic.\n\nIt's still a little more magical than I would like, but I think this is\nthe best we can do while still building on get_sha1_hex. Parsing it\nleft-to-right would be better, but we would essentially end up\nreimplementing get_sha1_hex.\n\n http-push.c | 38 +++++++++++++++++++++++---------------\n 1 file changed, 23 insertions(+), 15 deletions(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex 26dfa67..c5c95e8 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -719,14 +719,10 @@ static int fetch_indices(void)\n \treturn ret;\n }\n \n-static void one_remote_object(const char *hex)\n+static void one_remote_object(const unsigned char *sha1)\n {\n-\tunsigned char sha1[20];\n \tstruct object *obj;\n \n-\tif (get_sha1_hex(hex, sha1) != 0)\n-\t\treturn;\n-\n \tobj = lookup_object(sha1);\n \tif (!obj)\n \t\tobj = parse_object(sha1);\n@@ -1020,26 +1016,38 @@ static void remote_ls(const char *path, int flags,\n \t\t void (*userFunc)(struct remote_ls_ctx *ls),\n \t\t void *userData);\n \n+/* extract hex from sharded \"xx/x{40}\" filename */\n+static int get_sha1_hex_from_objpath(const char *path, unsigned char *sha1)\n+{\n+\tchar hex[40];\n+\n+\tif (strlen(path) != 41)\n+\t\treturn -1;\n+\n+\tmemcpy(hex, path, 2);\n+\tpath += 2;\n+\tpath++; /* skip '/' */\n+\tmemcpy(hex, path, 38);\n+\n+\treturn get_sha1_hex(hex, sha1);\n+}\n+\n static void process_ls_object(struct remote_ls_ctx *ls)\n {\n \tunsigned int *parent = (unsigned int *)ls->userData;\n-\tchar *path = ls->dentry_name;\n-\tchar *obj_hex;\n+\tconst char *path = ls->dentry_name;\n+\tunsigned char sha1[20];\n \n \tif (!strcmp(ls->path, ls->dentry_name) && (ls->flags & IS_DIR)) {\n \t\tremote_dir_exists[*parent] = 1;\n \t\treturn;\n \t}\n \n-\tif (strlen(path) != 49)\n+\tif (!skip_prefix(path, \"objects/\", &path) ||\n+\t get_sha1_hex_from_objpath(path, sha1))\n \t\treturn;\n-\tpath += 8;\n-\tobj_hex = xmalloc(strlen(path));\n-\t/* NB: path is not null-terminated, can not use strlcpy here */\n-\tmemcpy(obj_hex, path, 2);\n-\tstrcpy(obj_hex + 2, path + 3);\n-\tone_remote_object(obj_hex);\n-\tfree(obj_hex);\n+\n+\tone_remote_object(sha1);\n }\n \n static void process_ls_ref(struct remote_ls_ctx *ls)\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244726","messageId":"20140619220811.GB29685@sigill.intra.peff.net","threadId":"36953","inReplyTo":"20140619215810.GA29685@sigill.intra.peff.net","subject":"Re: [PATCH 17/16] http-push: refactor parsing of remote object names","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-19T22:08:11Z","receivedAt":"2014-06-19T22:08:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 19, 2014 at 05:58:10PM -0400, Jeff King wrote:\n\n> It's still a little more magical than I would like, but I think this is\n> the best we can do while still building on get_sha1_hex. Parsing it\n> left-to-right would be better, but we would essentially end up\n> reimplementing get_sha1_hex.\n\nJust for fun, that would look something like this (on top):\n\ndiff --git a/http-push.c b/http-push.c\nindex c5c95e8..2425c61 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -1016,20 +1016,34 @@ static void remote_ls(const char *path, int flags,\n \t\t void (*userFunc)(struct remote_ls_ctx *ls),\n \t\t void *userData);\n \n+static inline int parse_one_hex(unsigned char **to, const char **from)\n+{\n+\tunsigned int val;\n+\n+\t/* avoid reading past end-of-string for from[1] */\n+\tif (!(*from)[0])\n+\t\treturn -1;\n+\tval = (hexval((*from)[0]) << 4) | hexval((*from)[1]);\n+\tif (val & ~0xff)\n+\t\treturn -1;\n+\t*(*to)++ = val;\n+\t(*from) += 2;\n+\treturn 0;\n+}\n+\n /* extract hex from sharded \"xx/x{40}\" filename */\n static int get_sha1_hex_from_objpath(const char *path, unsigned char *sha1)\n {\n-\tchar hex[40];\n+\tint i;\n \n-\tif (strlen(path) != 41)\n+\tif (parse_one_hex(&sha1, &path) < 0)\n \t\treturn -1;\n-\n-\tmemcpy(hex, path, 2);\n-\tpath += 2;\n-\tpath++; /* skip '/' */\n-\tmemcpy(hex, path, 38);\n-\n-\treturn get_sha1_hex(hex, sha1);\n+\tif (*path++ != '/')\n+\t\treturn -1;\n+\tfor (i = 1; i < 20; i++)\n+\t\tif (parse_one_hex(&sha1, &path) < 0)\n+\t\t\treturn -1;\n+\treturn 0;\n }\n \n static void process_ls_object(struct remote_ls_ctx *ls)\n\nIn theory we could factor parse_one_hex to share with get_sha1_hex, but\nI am hesitant to touch get_sha1_hex, as it is used in a lot of tight\nloops.\n\n-Peff\n"},{"id":"244733","messageId":"CAPig+cTKgrYx1hcO-hV3ZT2sm1rR17bWJC0ktZzFQB=TMr6Xuw@mail.gmail.com","threadId":"36953","inReplyTo":"20140618194417.GD22622@sigill.intra.peff.net","subject":"Re: [PATCH 04/16] refactor skip_prefix to return a boolean","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-06-20T01:59:39Z","receivedAt":"2014-06-20T01:59:39Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jun 18, 2014 at 3:44 PM, Jeff King wrote:\n> The skip_prefix function returns a pointer to the content\n> past the prefix, or NULL if the prefix was not found. While\n> this is nice and simple, in practice it makes it hard to use\n> for two reasons:\n>\n> 1. When you want to conditionally skip or keep the string\n> as-is, you have to introduce a second temporary\n> variable. For example:\n>\n> tmp = skip_prefix(buf, \"foo\");\n> if (tmp)\n> buf = tmp;\n>\n> 2. It is verbose to check the outcome in a conditional, as\n> you need extra parentheses to silence compiler\n> warnings. For example:\n>\n> if ((cp = skip_prefix(buf, \"foo\"))\n> /* do something with cp */\n>\n> Both of these make it harder to use for long if-chains, and\n> we tend to use starts_with instead. However, the first line\n> of \"do something\" is often to then skip forward in buf past\n> the prefix, either using a magic constant or with an extra\n> strlen (which is generally computed at compile time, but\n> means we are repeating ourselves).\n>\n> This patch refactors skip_prefix to return a simple boolean,\n> and to provide the pointer value as an out-parameter. If the\n> prefix is not found, the out-parameter is untouched. This\n> lets you write:\n>\n> if (skip_prefix(arg, \"foo \", &arg))\n> do_foo(arg);\n> else if (skip_prefix(arg, \"bar \", &arg))\n> do_bar(arg);\n>\n> Signed-off-by: Jeff King \n> ---\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index b6f03b3..556c839 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -349,13 +349,31 @@ extern void set_die_is_recursing_routine(int (*routine)(void));\n> extern int starts_with(const char *str, const char *prefix);\n> extern int ends_with(const char *str, const char *suffix);\n>\n> -static inline const char *skip_prefix(const char *str, const char *prefix)\n> +/*\n> + * If \"str\" begins with \"prefix\", return 1. If out is non-NULL,\n> + * it it set to str + strlen(prefix) (i.e., the prefix is skipped).\n\nThe documentation claims that 'out' can be NULL, however, the code\ndoes not respect this. NULL 'out' seems rather pointless (unless you\nwant an alias for starts_with()), so presumably the documentation is\nincorrect.\n\n> + *\n> + * Otherwise, returns 0 and out is left untouched.\n> + *\n> + * Examples:\n> + *\n> + * [extract branch name, fail if not a branch]\n> + * if (!skip_prefix(ref, \"refs/heads/\", &branch)\n> + * return -1;\n> + *\n> + * [skip prefix if present, otherwise use whole string]\n> + * skip_prefix(name, \"refs/heads/\", &name);\n> + */\n> +static inline int skip_prefix(const char *str, const char *prefix,\n> + const char **out)\n> {\n> do {\n> - if (!*prefix)\n> - return str;\n> + if (!*prefix) {\n> + *out = str;\n> + return 1;\n> + }\n> } while (*str++ == *prefix++);\n> - return NULL;\n> + return 0;\n> }\n>\n> #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)\n"},{"id":"244737","messageId":"20140620020824.GA31908@sigill.intra.peff.net","threadId":"36953","inReplyTo":"CAPig+cTKgrYx1hcO-hV3ZT2sm1rR17bWJC0ktZzFQB=TMr6Xuw@mail.gmail.com","subject":"Re: [PATCH 04/16] refactor skip_prefix to return a boolean","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-20T02:08:24Z","receivedAt":"2014-06-20T02:08:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 19, 2014 at 09:59:39PM -0400, Eric Sunshine wrote:\n\n> > diff --git a/git-compat-util.h b/git-compat-util.h\n> > index b6f03b3..556c839 100644\n> > --- a/git-compat-util.h\n> > +++ b/git-compat-util.h\n> > @@ -349,13 +349,31 @@ extern void set_die_is_recursing_routine(int (*routine)(void));\n> > extern int starts_with(const char *str, const char *prefix);\n> > extern int ends_with(const char *str, const char *suffix);\n> >\n> > -static inline const char *skip_prefix(const char *str, const char *prefix)\n> > +/*\n> > + * If \"str\" begins with \"prefix\", return 1. If out is non-NULL,\n> > + * it it set to str + strlen(prefix) (i.e., the prefix is skipped).\n> \n> The documentation claims that 'out' can be NULL, however, the code\n> does not respect this. NULL 'out' seems rather pointless (unless you\n> want an alias for starts_with()), so presumably the documentation is\n> incorrect.\n\nThanks for catching. My original version (before I sent to the list) did\nallow for a conditional NULL out, but I realized there was exactly one\ncaller who wanted this, and that they would be better off using\nstarts_with. I added patch 3 (\"avoid using skip_prefix as a boolean\")\nand stripped the conditional from skip_prefix, but forgot to update the\ncomment.\n\nI think we'd just want to squash the patch below (I also took the\nopportunity to fix a typo and clarify the text a bit):\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 556c839..1187e1a 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -350,8 +350,9 @@ extern int starts_with(const char *str, const char *prefix);\n extern int ends_with(const char *str, const char *suffix);\n \n /*\n- * If \"str\" begins with \"prefix\", return 1. If out is non-NULL,\n- * it it set to str + strlen(prefix) (i.e., the prefix is skipped).\n+ * If the string \"str\" begins with the string found in \"prefix\", return 1.\n+ * The \"out\" parameter is set to \"str + strlen(prefix)\" (i.e., to the point in\n+ * the string right after the prefix).\n *\n * Otherwise, returns 0 and out is left untouched.\n *\n\n-Peff\n"},{"id":"244739","messageId":"CAPig+cST9MW2fYgCD3C-3HJQbMLHzBSX7Vxr23ejmVFTmkCj7A@mail.gmail.com","threadId":"36953","inReplyTo":"20140620020824.GA31908@sigill.intra.peff.net","subject":"Re: [PATCH 04/16] refactor skip_prefix to return a boolean","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-06-20T02:30:36Z","receivedAt":"2014-06-20T02:30:36Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Jun 19, 2014 at 10:08 PM, Jeff King wrote:\n> On Thu, Jun 19, 2014 at 09:59:39PM -0400, Eric Sunshine wrote:\n>\n>> > diff --git a/git-compat-util.h b/git-compat-util.h\n>> > index b6f03b3..556c839 100644\n>> > --- a/git-compat-util.h\n>> > +++ b/git-compat-util.h\n>> > @@ -349,13 +349,31 @@ extern void set_die_is_recursing_routine(int (*routine)(void));\n>> > extern int starts_with(const char *str, const char *prefix);\n>> > extern int ends_with(const char *str, const char *suffix);\n>> >\n>> > -static inline const char *skip_prefix(const char *str, const char *prefix)\n>> > +/*\n>> > + * If \"str\" begins with \"prefix\", return 1. If out is non-NULL,\n>> > + * it it set to str + strlen(prefix) (i.e., the prefix is skipped).\n>>\n>> The documentation claims that 'out' can be NULL, however, the code\n>> does not respect this. NULL 'out' seems rather pointless (unless you\n>> want an alias for starts_with()), so presumably the documentation is\n>> incorrect.\n>\n> Thanks for catching. My original version (before I sent to the list) did\n> allow for a conditional NULL out, but I realized there was exactly one\n> caller who wanted this, and that they would be better off using\n> starts_with. I added patch 3 (\"avoid using skip_prefix as a boolean\")\n> and stripped the conditional from skip_prefix, but forgot to update the\n> comment.\n>\n> I think we'd just want to squash the patch below (I also took the\n> opportunity to fix a typo and clarify the text a bit):\n>\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index 556c839..1187e1a 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -350,8 +350,9 @@ extern int starts_with(const char *str, const char *prefix);\n> extern int ends_with(const char *str, const char *suffix);\n>\n> /*\n> - * If \"str\" begins with \"prefix\", return 1. If out is non-NULL,\n> - * it it set to str + strlen(prefix) (i.e., the prefix is skipped).\n> + * If the string \"str\" begins with the string found in \"prefix\", return 1.\n> + * The \"out\" parameter is set to \"str + strlen(prefix)\" (i.e., to the point in\n> + * the string right after the prefix).\n> *\n> * Otherwise, returns 0 and out is left untouched.\n\nFor consistency with the preceding paragraph:\n\n Otherwise, return 0 and leave \"out\" untouched.\n\n> *\n>\n> -Peff\n"},{"id":"244740","messageId":"20140620023844.GA1587@sigill.intra.peff.net","threadId":"36953","inReplyTo":"CAPig+cST9MW2fYgCD3C-3HJQbMLHzBSX7Vxr23ejmVFTmkCj7A@mail.gmail.com","subject":"Re: [PATCH 04/16] refactor skip_prefix to return a boolean","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-20T02:38:44Z","receivedAt":"2014-06-20T02:38:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 19, 2014 at 10:30:36PM -0400, Eric Sunshine wrote:\n\n> > diff --git a/git-compat-util.h b/git-compat-util.h\n> > index 556c839..1187e1a 100644\n> > --- a/git-compat-util.h\n> > +++ b/git-compat-util.h\n> > @@ -350,8 +350,9 @@ extern int starts_with(const char *str, const char *prefix);\n> > extern int ends_with(const char *str, const char *suffix);\n> >\n> > /*\n> > - * If \"str\" begins with \"prefix\", return 1. If out is non-NULL,\n> > - * it it set to str + strlen(prefix) (i.e., the prefix is skipped).\n> > + * If the string \"str\" begins with the string found in \"prefix\", return 1.\n> > + * The \"out\" parameter is set to \"str + strlen(prefix)\" (i.e., to the point in\n> > + * the string right after the prefix).\n> > *\n> > * Otherwise, returns 0 and out is left untouched.\n> \n> For consistency with the preceding paragraph:\n> \n> Otherwise, return 0 and leave \"out\" untouched.\n\nHeh, I even noticed the verb tense mismatch but thought \"nah, nobody\nwill care\". I see I was wrong. :)\n\nHere is the full squashable patch against v1, in case Junio picks it up\nfrom here (or it will be in my re-roll if necessary).\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 556c839..d29e1df 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -350,10 +350,11 @@ extern int starts_with(const char *str, const char *prefix);\n extern int ends_with(const char *str, const char *suffix);\n \n /*\n- * If \"str\" begins with \"prefix\", return 1. If out is non-NULL,\n- * it it set to str + strlen(prefix) (i.e., the prefix is skipped).\n+ * If the string \"str\" begins with the string found in \"prefix\", return 1.\n+ * The \"out\" parameter is set to \"str + strlen(prefix)\" (i.e., to the point in\n+ * the string right after the prefix).\n *\n- * Otherwise, returns 0 and out is left untouched.\n+ * Otherwise, return 0 and leave \"out\" untouched.\n *\n * Examples:\n *\n"},{"id":"244741","messageId":"CAPig+cTgz1s_68MVT5XgTsM9j=NZnCt3tNX3wBGddEq3nWZgyw@mail.gmail.com","threadId":"36953","inReplyTo":"20140618194912.GJ22622@sigill.intra.peff.net","subject":"Re: [PATCH 10/16] fast-import: use skip_prefix for parsing input","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-06-20T03:19:09Z","receivedAt":"2014-06-20T03:19:09Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jun 18, 2014 at 3:49 PM, Jeff King wrote:\n> Fast-import does a lot of parsing of commands and\n> dispatching to sub-functions. For example, given \"option\n> foo\", we might recognize \"option \" using starts_with, and\n> then hand it off to parse_option() to do the rest.\n>\n> However, we do not let parse_option know that we have parsed\n> the first part already. It gets the full buffer, and has to\n> skip past the uninteresting bits. Some functions simply add\n> a magic constant:\n>\n> char *option = command_buf.buf + 7;\n>\n> Others use strlen:\n>\n> char *option = command_buf.buf + strlen(\"option \");\n>\n> And others use strchr:\n>\n> char *option = strchr(command_buf.buf, ' ') + 1;\n>\n> All of these are brittle and easy to get wrong (especially\n> given that the starts_with call and the code that assumes\n> the presence of the prefix are far apart). Instead, we can\n> use skip_prefix, and just pass each handler a pointer to its\n> arguments.\n>\n> Signed-off-by: Jeff King \n> ---\n> diff --git a/fast-import.c b/fast-import.c\n> index a3ffe30..5f17adb 100644\n> --- a/fast-import.c\n> +++ b/fast-import.c\n> @@ -2713,20 +2706,22 @@ static void parse_new_commit(void)\n>\n> /* file_change* */\n> while (command_buf.len > 0) {\n> - if (starts_with(command_buf.buf, \"M \"))\n> - file_change_m(b);\n> - else if (starts_with(command_buf.buf, \"D \"))\n> - file_change_d(b);\n> - else if (starts_with(command_buf.buf, \"R \"))\n> - file_change_cr(b, 1);\n> - else if (starts_with(command_buf.buf, \"C \"))\n> - file_change_cr(b, 0);\n> - else if (starts_with(command_buf.buf, \"N \"))\n> - note_change_n(b, &prev_fanout);\n> + const char *v;\n\nThis declaration of 'v' shadows the 'v' added by patch 8/16 earlier in\nthe function.\n\n> + if (skip_prefix(command_buf.buf, \"M \", &v))\n> + file_change_m(v, b);\n> + else if (skip_prefix(command_buf.buf, \"D \", &v))\n> + file_change_d(v, b);\n> + else if (skip_prefix(command_buf.buf, \"R \", &v))\n> + file_change_cr(v, b, 1);\n> + else if (skip_prefix(command_buf.buf, \"C \", &v))\n> + file_change_cr(v, b, 0);\n> + else if (skip_prefix(command_buf.buf, \"N \", &v))\n> + note_change_n(v, b, &prev_fanout);\n> else if (!strcmp(\"deleteall\", command_buf.buf))\n> file_change_deleteall(b);\n> - else if (starts_with(command_buf.buf, \"ls \"))\n> - parse_ls(b);\n> + else if (skip_prefix(command_buf.buf, \"ls \", &v))\n> + parse_ls(v, b);\n> else {\n> unread_command_buf = 1;\n> break;\n"},{"id":"244742","messageId":"CAPig+cTBaDDigjSSeMLvJ=R701XNTtjVL1JKE8TWbHVTFC8BJA@mail.gmail.com","threadId":"36953","inReplyTo":"20140618195603.GN22622@sigill.intra.peff.net","subject":"Re: [PATCH 14/16] fetch-pack: refactor parsing in get_ack","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-06-20T05:15:20Z","receivedAt":"2014-06-20T05:15:20Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jun 18, 2014 at 3:56 PM, Jeff King wrote:\n> There are several uses of the magic number \"line+45\" when\n> parsing ACK lines from the server, and it's rather unclear\n> why 45 is the correct number. We can make this more clear by\n> keeping a running pointer as we parse, using skip_prefix to\n> jump past the first \"ACK \", then adding 40 to jump past\n> get_sha1_hex (which is still magical, but hopefully 40 is\n> less magical to readers of git code).\n>\n> Note that this actually puts us at line+44. The original\n> required some character between the sha1 and further ACK\n> flags (it is supposed to be a space, but we never enforced\n> that). We start our search for flags at line+44, which\n> meanas we are slightly more liberal than the old code.\n\ns/meanas/means/\n\n> Signed-off-by: Jeff King \n"},{"id":"244743","messageId":"20140620054549.GA4623@sigill.intra.peff.net","threadId":"36953","inReplyTo":"CAPig+cTgz1s_68MVT5XgTsM9j=NZnCt3tNX3wBGddEq3nWZgyw@mail.gmail.com","subject":"Re: [PATCH 10/16] fast-import: use skip_prefix for parsing input","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-20T05:45:49Z","receivedAt":"2014-06-20T05:45:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 19, 2014 at 11:19:09PM -0400, Eric Sunshine wrote:\n\n> > - if (starts_with(command_buf.buf, \"M \"))\n> > - file_change_m(b);\n> > - else if (starts_with(command_buf.buf, \"D \"))\n> > - file_change_d(b);\n> > - else if (starts_with(command_buf.buf, \"R \"))\n> > - file_change_cr(b, 1);\n> > - else if (starts_with(command_buf.buf, \"C \"))\n> > - file_change_cr(b, 0);\n> > - else if (starts_with(command_buf.buf, \"N \"))\n> > - note_change_n(b, &prev_fanout);\n> > + const char *v;\n> \n> This declaration of 'v' shadows the 'v' added by patch 8/16 earlier in\n> the function.\n\nThanks. I reordered the patches before sending, so when this one was\noriginally written, there was no \"v\" at the top-level of the function.\nI think we can just drop this interior one. The point of the short \"v\"\nis that it can be used as a temporary value for prefix matches, so I\nthink we can just reuse the same one.\n\nI tried compiling with -Wshadow (which I don't usually do), but we're\nnot even close to compiling clean there. Some of them are legitimately\nconfusing (e.g., try figuring out \"end\" in parse_rev_note). But others\nlook just annoying (e.g., complaining that a local \"usage\" conflicts\nwith the global function). I don't know if we want to put effort into\nbeing -Wshadow clean or not.\n\n-Peff\n"},{"id":"244750","messageId":"CAPig+cS6MoU0zDxY9ijMZmVGH04s_7RHLNoW8UVhQ9ff1OkiDA@mail.gmail.com","threadId":"36953","inReplyTo":"20140620054549.GA4623@sigill.intra.peff.net","subject":"Re: [PATCH 10/16] fast-import: use skip_prefix for parsing input","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-06-20T08:59:51Z","receivedAt":"2014-06-20T08:59:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jun 20, 2014 at 1:45 AM, Jeff King wrote:\n> On Thu, Jun 19, 2014 at 11:19:09PM -0400, Eric Sunshine wrote:\n>\n>> > - if (starts_with(command_buf.buf, \"M \"))\n>> > - file_change_m(b);\n>> > - else if (starts_with(command_buf.buf, \"D \"))\n>> > - file_change_d(b);\n>> > - else if (starts_with(command_buf.buf, \"R \"))\n>> > - file_change_cr(b, 1);\n>> > - else if (starts_with(command_buf.buf, \"C \"))\n>> > - file_change_cr(b, 0);\n>> > - else if (starts_with(command_buf.buf, \"N \"))\n>> > - note_change_n(b, &prev_fanout);\n>> > + const char *v;\n>>\n>> This declaration of 'v' shadows the 'v' added by patch 8/16 earlier in\n>> the function.\n>\n> Thanks. I reordered the patches before sending, so when this one was\n> originally written, there was no \"v\" at the top-level of the function.\n> I think we can just drop this interior one. The point of the short \"v\"\n> is that it can be used as a temporary value for prefix matches, so I\n> think we can just reuse the same one.\n\nAgreed. The intended usage of 'v' is clear enough and the code simple\nenough that confusion is unlikely.\n\n> I tried compiling with -Wshadow (which I don't usually do), but we're\n> not even close to compiling clean there. Some of them are legitimately\n> confusing (e.g., try figuring out \"end\" in parse_rev_note). But others\n> look just annoying (e.g., complaining that a local \"usage\" conflicts\n> with the global function). I don't know if we want to put effort into\n> being -Wshadow clean or not.\n\nI just happened to notice the shadowing declaration while reading the\npatch, but don't feel strongly about existing cases. It makes sense to\nclean up confusing cases, such 'end' in parse_rev_note(), when working\non that code (just as with style cleanups), but thus far nobody has\nbeen complaining about existing shadowed variables, so global cleanup\nwould likely be considered churn.\n"},{"id":"244889","messageId":"xmqq1tufcvfl.fsf@gitster.dls.corp.google.com","threadId":"36953","inReplyTo":"20140618194417.GD22622@sigill.intra.peff.net","subject":"Re: [PATCH 04/16] refactor skip_prefix to return a boolean","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-23T18:50:06Z","receivedAt":"2014-06-23T18:50:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index b12989d..a5b2d9d 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -703,9 +703,12 @@ static void write_refspec_config(const char* src_ref_prefix,\n> \t\t\t\t\tstrbuf_addf(&value, \"+%s:%s%s\", our_head_points_at->name,\n> \t\t\t\t\t\tbranch_top->buf, option_branch);\n> \t\t\t} else if (remote_head_points_at) {\n> +\t\t\t\tconst char *head = remote_head_points_at->name;\n> +\t\t\t\tif (!skip_prefix(head, \"refs/heads/\", &head))\n> +\t\t\t\t\tdie(\"BUG: remote HEAD points at non-head?\");\n> +\n> \t\t\t\tstrbuf_addf(&value, \"+%s:%s%s\", remote_head_points_at->name,\n> -\t\t\t\t\t\tbranch_top->buf,\n> -\t\t\t\t\t\tskip_prefix(remote_head_points_at->name, \"refs/heads/\"));\n> +\t\t\t\t\t\tbranch_top->buf, head);\n> \t\t\t}\n> \t\t\t/*\n> \t\t\t * otherwise, the next \"git fetch\" will\n\nI was re-reading this and noticed another possible bug.\n\n builtin/clone.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex b12989d..df659dd 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -696,7 +696,7 @@ static void write_refspec_config(const char* src_ref_prefix,\n \tif (option_mirror || !option_bare) {\n \t\tif (option_single_branch && !option_mirror) {\n \t\t\tif (option_branch) {\n-\t\t\t\tif (strstr(our_head_points_at->name, \"refs/tags/\"))\n+\t\t\t\tif (starts_with(our_head_points_at->name, \"refs/tags/\"))\n \t\t\t\t\tstrbuf_addf(&value, \"+%s:%s\", our_head_points_at->name,\n \t\t\t\t\t\tour_head_points_at->name);\n \t\t\t\telse\n\nBecause the pattern is not anchored to the left with a slash, it is\nclear that the original cannot even claim that it was trying to\nmunge \"foo/refs/tags/\" as well.\n\nWhich means this is trivially correct, but at the same time I wonder\nwhat it means for our-head to point at a ref in refs/tags/ hierarchy.\n"},{"id":"244893","messageId":"20140623210755.GA15766@sigill.intra.peff.net","threadId":"36953","inReplyTo":"xmqq1tufcvfl.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 04/16] refactor skip_prefix to return a boolean","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-23T21:07:55Z","receivedAt":"2014-06-23T21:07:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 23, 2014 at 11:50:06AM -0700, Junio C Hamano wrote:\n\n> I was re-reading this and noticed another possible bug.\n> \n> builtin/clone.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index b12989d..df659dd 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -696,7 +696,7 @@ static void write_refspec_config(const char* src_ref_prefix,\n> \tif (option_mirror || !option_bare) {\n> \t\tif (option_single_branch && !option_mirror) {\n> \t\t\tif (option_branch) {\n> -\t\t\t\tif (strstr(our_head_points_at->name, \"refs/tags/\"))\n> +\t\t\t\tif (starts_with(our_head_points_at->name, \"refs/tags/\"))\n> \t\t\t\t\tstrbuf_addf(&value, \"+%s:%s\", our_head_points_at->name,\n> \t\t\t\t\t\tour_head_points_at->name);\n> \t\t\t\telse\n> \n> Because the pattern is not anchored to the left with a slash, it is\n> clear that the original cannot even claim that it was trying to\n> munge \"foo/refs/tags/\" as well.\n\nYeah, the strstr seems very wrong there. Even with the \"/\", why would\nyou want to match \"refs/heads/refs/tags/\"?\n\n> Which means this is trivially correct, but at the same time I wonder\n> what it means for our-head to point at a ref in refs/tags/ hierarchy.\n\nI think it is for \"git clone --branch=v1.0\". We create a refspec pulling\nv1.0 to its local tag in that case (as opposed to to something in\n\"refs/remotes/origin/\"). So I really think this does want to be\nstarts_with.\n\n-Peff\n"},{"id":"244896","messageId":"xmqqk387b9bw.fsf_-_@gitster.dls.corp.google.com","threadId":"36953","inReplyTo":"20140623210755.GA15766@sigill.intra.peff.net","subject":"[PATCH] builtin/clone.c: detect a clone starting at a tag correctly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-23T21:32:51Z","receivedAt":"2014-06-23T21:32:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"31b808a0 (clone --single: limit the fetch refspec to fetched branch,\n2012-09-20) tried to see if the given \"branch\" to follow is actually\na tag at the remote repository by checking with \"refs/tags/\" but it\nincorrectly used strstr(3); it is actively wrong to treat a \"branch\"\n\"refs/heads/refs/tags/foo\" and use the logic for the \"refs/tags/\"\nref hierarchy. What the code really wanted to do is to see if it\nstarts with \"refs/tags/\".\n\nSigned-off-by: Junio C Hamano \n---\n\n Jeff King writes:\n\n >> Because the pattern is not anchored to the left with a slash, it is\n >> clear that the original cannot even claim that it was trying to\n >> munge \"foo/refs/tags/\" as well.\n >\n > Yeah, the strstr seems very wrong there. Even with the \"/\", why would\n > you want to match \"refs/heads/refs/tags/\"?\n >\n >> Which means this is trivially correct, but at the same time I wonder\n >> what it means for our-head to point at a ref in refs/tags/ hierarchy.\n >\n > I think it is for \"git clone --branch=v1.0\". We create a refspec pulling\n > v1.0 to its local tag in that case (as opposed to to something in\n > \"refs/remotes/origin/\"). So I really think this does want to be\n > starts_with.\n\n Thanks; here is what I'm gonna queue.\n\n builtin/clone.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 9b3c04d..545105a 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -695,7 +695,7 @@ static void write_refspec_config(const char* src_ref_prefix,\n \tif (option_mirror || !option_bare) {\n \t\tif (option_single_branch && !option_mirror) {\n \t\t\tif (option_branch) {\n-\t\t\t\tif (strstr(our_head_points_at->name, \"refs/tags/\"))\n+\t\t\t\tif (starts_with(our_head_points_at->name, \"refs/tags/\"))\n \t\t\t\t\tstrbuf_addf(&value, \"+%s:%s\", our_head_points_at->name,\n \t\t\t\t\t\tour_head_points_at->name);\n \t\t\t\telse\n-- \n2.0.0-637-g8ac8cc9\n"},{"id":"244897","messageId":"xmqqa993b8so.fsf@gitster.dls.corp.google.com","threadId":"36953","inReplyTo":"20140618194750.GH22622@sigill.intra.peff.net","subject":"Re: [PATCH 08/16] use skip_prefix to avoid magic numbers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-23T21:44:23Z","receivedAt":"2014-06-23T21:44:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> diff --git a/connect.c b/connect.c\n> index 94a6650..37ff018 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -140,12 +141,12 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n> \t\tif (!len)\n> \t\t\tbreak;\n> \n> -\t\tif (len > 4 && starts_with(buffer, \"ERR \"))\n> -\t\t\tdie(\"remote error: %s\", buffer + 4);\n> +\t\tif (len > 4 && skip_prefix(buffer, \"ERR \", &arg))\n> +\t\t\tdie(\"remote error: %s\", arg);\n\nMakes one wonder if we should do something special to a line with\nonly \"ERR \" and nothing else on it, which the other end may have\nmeant us to give a blank line to make the output more readable.\n\nA fix, if one turns out to be needed, is outside the scope of this\npatch, though, I think.\n"},{"id":"245243","messageId":"20140701173502.GA12777@sigill.intra.peff.net","threadId":"36953","inReplyTo":"xmqqa993b8so.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 08/16] use skip_prefix to avoid magic numbers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-01T17:35:02Z","receivedAt":"2014-07-01T17:35:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 23, 2014 at 02:44:23PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > diff --git a/connect.c b/connect.c\n> > index 94a6650..37ff018 100644\n> > --- a/connect.c\n> > +++ b/connect.c\n> > @@ -140,12 +141,12 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n> > \t\tif (!len)\n> > \t\t\tbreak;\n> > \n> > -\t\tif (len > 4 && starts_with(buffer, \"ERR \"))\n> > -\t\t\tdie(\"remote error: %s\", buffer + 4);\n> > +\t\tif (len > 4 && skip_prefix(buffer, \"ERR \", &arg))\n> > +\t\t\tdie(\"remote error: %s\", arg);\n> \n> Makes one wonder if we should do something special to a line with\n> only \"ERR \" and nothing else on it, which the other end may have\n> meant us to give a blank line to make the output more readable.\n\nI don't think that would buy us much. We have always accepted only a\nsingle ERR line and died immediately. So any changes of that nature\nwould have to be made in the client, and then servers would have to wait\nN time units before it was safe to start using the feature (otherwise\nold clients just get the blank line!).\n\nI also don't think blank lines by themselves are useful. You'd want them\nin addition to being able to handle multiple lines. So a nicer fix is\nmore along the lines of \"accept multiple ERR lines, including blank\nlines, followed by a terminating line (\"ERRDONE\" or something).\n\nThen servers can do:\n\n ERR unable to access foo.git: Printer on fire\n ERR\n ERR You may have misspelled the repository name. Did you mean:\n ERR\n ERR foobar.git\n ERRDONE\n\nOld clients would see the first line and die. Newer clients would print\nthe helpful hint. Servers would just need to make sure that the first\nline stands on its own to cover both cases.\n\n> A fix, if one turns out to be needed, is outside the scope of this\n> patch, though, I think.\n\nYeah, definitely a separate topic.\n\nIt is not something I think anybody has asked for, but I can imagine a\nsite like GitHub making use of it (we already show custom errors for\nhttp, but there's no room beyond the single ERR line). And teaching the\nclients now expands the options for servers later. So it might be worth\ndoing just as a potential feature.\n\n-Peff\n"}]}