From 9e1a5ebe52d84c35eee18a896ad1b8bab53cadbf Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 18 Jun 2014 15:41:50 -0400 Subject: [PATCH 01/18] parse_diff_color_slot: drop ofs parameter This function originally took a whole config variable name ("var") and an offset ("ofs"). It checked "var+ofs" against each color slot, but reported errors using the whole "var". However, since 8b8e862 (ignore unknown color configuration, 2009-12-12), it returns -1 rather than printing its own error, and therefore only cares about var+ofs. We can drop the ofs parameter and teach its sole caller to derive the pointer itself. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- diff.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/diff.c b/diff.c index bba9a558a6..77c5eb4529 100644 --- a/diff.c +++ b/diff.c @@ -52,23 +52,23 @@ static char diff_colors[][COLOR_MAXLEN] = { GIT_COLOR_NORMAL, /* FUNCINFO */ }; -static int parse_diff_color_slot(const char *var, int ofs) +static int parse_diff_color_slot(const char *var) { - if (!strcasecmp(var+ofs, "plain")) + if (!strcasecmp(var, "plain")) return DIFF_PLAIN; - if (!strcasecmp(var+ofs, "meta")) + if (!strcasecmp(var, "meta")) return DIFF_METAINFO; - if (!strcasecmp(var+ofs, "frag")) + if (!strcasecmp(var, "frag")) return DIFF_FRAGINFO; - if (!strcasecmp(var+ofs, "old")) + if (!strcasecmp(var, "old")) return DIFF_FILE_OLD; - if (!strcasecmp(var+ofs, "new")) + if (!strcasecmp(var, "new")) return DIFF_FILE_NEW; - if (!strcasecmp(var+ofs, "commit")) + if (!strcasecmp(var, "commit")) return DIFF_COMMIT; - if (!strcasecmp(var+ofs, "whitespace")) + if (!strcasecmp(var, "whitespace")) return DIFF_WHITESPACE; - if (!strcasecmp(var+ofs, "func")) + if (!strcasecmp(var, "func")) return DIFF_FUNCINFO; return -1; } @@ -240,7 +240,7 @@ int git_diff_basic_config(const char *var, const char *value, void *cb) return -1; if (starts_with(var, "diff.color.") || starts_with(var, "color.diff.")) { - int slot = parse_diff_color_slot(var, 11); + int slot = parse_diff_color_slot(var + 11); if (slot < 0) return 0; if (!value) From 1055a890f04784d7ea7cdaca8d30479e685d8643 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 18 Jun 2014 15:41:58 -0400 Subject: [PATCH 02/18] daemon: mark some strings as const None of these strings is modified; marking them as const will help later refactoring. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- daemon.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/daemon.c b/daemon.c index f9c63e9613..18818c3fda 100644 --- a/daemon.c +++ b/daemon.c @@ -39,8 +39,8 @@ static int strict_paths; static int export_all_trees; /* Take all paths relative to this one if non-NULL */ -static char *base_path; -static char *interpolated_path; +static const char *base_path; +static const char *interpolated_path; static int base_path_relaxed; /* Flag indicating client sent extra args. */ @@ -106,12 +106,12 @@ static void NORETURN daemon_die(const char *err, va_list params) exit(1); } -static const char *path_ok(char *directory) +static const char *path_ok(const char *directory) { static char rpath[PATH_MAX]; static char interp_path[PATH_MAX]; const char *path; - char *dir; + const char *dir; dir = directory; @@ -131,7 +131,7 @@ static const char *path_ok(char *directory) * "~alice/%s/foo". */ int namlen, restlen = strlen(dir); - char *slash = strchr(dir, '/'); + const char *slash = strchr(dir, '/'); if (!slash) slash = dir + restlen; namlen = slash - dir; @@ -253,7 +253,7 @@ static int daemon_error(const char *dir, const char *msg) return -1; } -static char *access_hook; +static const char *access_hook; static int run_access_hook(struct daemon_service *service, const char *dir, const char *path) { @@ -318,7 +318,7 @@ error_return: return -1; } -static int run_service(char *dir, struct daemon_service *service) +static int run_service(const char *dir, struct daemon_service *service) { const char *path; int enabled = service->enabled; From c0264180d74510669bcec9bb5ff12a145c685b8e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 18 Jun 2014 15:42:14 -0400 Subject: [PATCH 03/18] avoid using skip_prefix as a boolean There's no point in using: if (skip_prefix(buf, "foo")) over if (starts_with(buf, "foo")) as the point of skip_prefix is to return a pointer to the data after the prefix. Using starts_with is more readable, and will make refactoring skip_prefix easier. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/fmt-merge-msg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c index 3906eda877..72378e6810 100644 --- a/builtin/fmt-merge-msg.c +++ b/builtin/fmt-merge-msg.c @@ -298,7 +298,7 @@ static void credit_people(struct strbuf *out, (them->nr == 1 && me && (me = skip_prefix(me, them->items->string)) != NULL && - skip_prefix(me, " <"))) + starts_with(me, " <"))) return; strbuf_addf(out, "\n%c %s ", comment_line_char, label); add_people_count(out, them); From cf4fff579e02d8585d59f6c8739534b7b0d617dd Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 18 Jun 2014 15:44:19 -0400 Subject: [PATCH 04/18] refactor skip_prefix to return a boolean The skip_prefix() function returns a pointer to the content past the prefix, or NULL if the prefix was not found. While this is nice and simple, in practice it makes it hard to use for two reasons: 1. When you want to conditionally skip or keep the string as-is, you have to introduce a temporary variable. For example: tmp = skip_prefix(buf, "foo"); if (tmp) buf = tmp; 2. It is verbose to check the outcome in a conditional, as you need extra parentheses to silence compiler warnings. For example: if ((cp = skip_prefix(buf, "foo")) /* do something with cp */ Both of these make it harder to use for long if-chains, and we tend to use starts_with() instead. However, the first line of "do something" is often to then skip forward in buf past the prefix, either using a magic constant or with an extra strlen(3) (which is generally computed at compile time, but means we are repeating ourselves). This patch refactors skip_prefix() to return a simple boolean, and to provide the pointer value as an out-parameter. If the prefix is not found, the out-parameter is untouched. This lets you write: if (skip_prefix(arg, "foo ", &arg)) do_foo(arg); else if (skip_prefix(arg, "bar ", &arg)) do_bar(arg); Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- advice.c | 5 ++++- branch.c | 4 ++-- builtin/branch.c | 6 +++--- builtin/clone.c | 11 +++++++---- builtin/commit.c | 5 ++--- builtin/fmt-merge-msg.c | 2 +- builtin/push.c | 7 +++---- builtin/remote.c | 4 +--- column.c | 5 +++-- commit.c | 6 ++---- config.c | 3 +-- credential-cache--daemon.c | 6 ++---- credential.c | 3 +-- fsck.c | 14 +++++--------- git-compat-util.h | 27 +++++++++++++++++++++++---- parse-options.c | 16 +++++++++------- transport.c | 4 +++- urlmatch.c | 3 +-- 18 files changed, 73 insertions(+), 58 deletions(-) diff --git a/advice.c b/advice.c index c50ebdf5fe..9b42033192 100644 --- a/advice.c +++ b/advice.c @@ -61,9 +61,12 @@ void advise(const char *advice, ...) int git_default_advice_config(const char *var, const char *value) { - const char *k = skip_prefix(var, "advice."); + const char *k; int i; + if (!skip_prefix(var, "advice.", &k)) + return 0; + for (i = 0; i < ARRAY_SIZE(advice_config); i++) { if (strcmp(k, advice_config[i].name)) continue; diff --git a/branch.c b/branch.c index 660097bc29..46e8aa86df 100644 --- a/branch.c +++ b/branch.c @@ -50,11 +50,11 @@ static int should_setup_rebase(const char *origin) void install_branch_config(int flag, const char *local, const char *origin, const char *remote) { - const char *shortname = skip_prefix(remote, "refs/heads/"); + const char *shortname = NULL; struct strbuf key = STRBUF_INIT; int rebasing = should_setup_rebase(origin); - if (shortname + if (skip_prefix(remote, "refs/heads/", &shortname) && !strcmp(local, shortname) && !origin) { warning(_("Not setting branch %s as its own upstream."), diff --git a/builtin/branch.c b/builtin/branch.c index 652b1d2d14..0591b22a48 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -294,13 +294,13 @@ static char *resolve_symref(const char *src, const char *prefix) { unsigned char sha1[20]; int flag; - const char *dst, *cp; + const char *dst; dst = resolve_ref_unsafe(src, sha1, 0, &flag); if (!(dst && (flag & REF_ISSYMREF))) return NULL; - if (prefix && (cp = skip_prefix(dst, prefix))) - dst = cp; + if (prefix) + skip_prefix(dst, prefix, &dst); return xstrdup(dst); } diff --git a/builtin/clone.c b/builtin/clone.c index b12989d1ca..a5b2d9db36 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -584,11 +584,11 @@ static void update_remote_refs(const struct ref *refs, static void update_head(const struct ref *our, const struct ref *remote, const char *msg) { - if (our && starts_with(our->name, "refs/heads/")) { + const char *head; + if (our && skip_prefix(our->name, "refs/heads/", &head)) { /* Local default branch link */ create_symref("HEAD", our->name, NULL); if (!option_bare) { - const char *head = skip_prefix(our->name, "refs/heads/"); update_ref(msg, "HEAD", our->old_sha1, NULL, 0, UPDATE_REFS_DIE_ON_ERR); install_branch_config(0, head, option_origin, our->name); @@ -703,9 +703,12 @@ static void write_refspec_config(const char* src_ref_prefix, strbuf_addf(&value, "+%s:%s%s", our_head_points_at->name, branch_top->buf, option_branch); } else if (remote_head_points_at) { + const char *head = remote_head_points_at->name; + if (!skip_prefix(head, "refs/heads/", &head)) + die("BUG: remote HEAD points at non-head?"); + strbuf_addf(&value, "+%s:%s%s", remote_head_points_at->name, - branch_top->buf, - skip_prefix(remote_head_points_at->name, "refs/heads/")); + branch_top->buf, head); } /* * otherwise, the next "git fetch" will diff --git a/builtin/commit.c b/builtin/commit.c index 5e2221c8e8..ec75341238 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1020,7 +1020,7 @@ static int message_is_empty(struct strbuf *sb) static int template_untouched(struct strbuf *sb) { struct strbuf tmpl = STRBUF_INIT; - char *start; + const char *start; if (cleanup_mode == CLEANUP_NONE && sb->len) return 0; @@ -1029,8 +1029,7 @@ static int template_untouched(struct strbuf *sb) return 0; stripspace(&tmpl, cleanup_mode == CLEANUP_ALL); - start = (char *)skip_prefix(sb->buf, tmpl.buf); - if (!start) + if (!skip_prefix(sb->buf, tmpl.buf, &start)) start = sb->buf; strbuf_release(&tmpl); return rest_is_empty(sb, start - sb->buf); diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c index 72378e6810..3c19241554 100644 --- a/builtin/fmt-merge-msg.c +++ b/builtin/fmt-merge-msg.c @@ -297,7 +297,7 @@ static void credit_people(struct strbuf *out, if (!them->nr || (them->nr == 1 && me && - (me = skip_prefix(me, them->items->string)) != NULL && + skip_prefix(me, them->items->string, &me) && starts_with(me, " <"))) return; strbuf_addf(out, "\n%c %s ", comment_line_char, label); diff --git a/builtin/push.c b/builtin/push.c index f8dfea41e1..f50e3d5e77 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -127,11 +127,10 @@ static NORETURN int die_push_simple(struct branch *branch, struct remote *remote * them the big ugly fully qualified ref. */ const char *advice_maybe = ""; - const char *short_upstream = - skip_prefix(branch->merge[0]->src, "refs/heads/"); + const char *short_upstream = branch->merge[0]->src; + + skip_prefix(short_upstream, "refs/heads/", &short_upstream); - if (!short_upstream) - short_upstream = branch->merge[0]->src; /* * Don't show advice for people who explicitly set * push.default. diff --git a/builtin/remote.c b/builtin/remote.c index c9102e8fe9..a8efe3da4d 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -250,9 +250,7 @@ static struct string_list branch_list; static const char *abbrev_ref(const char *name, const char *prefix) { - const char *abbrev = skip_prefix(name, prefix); - if (abbrev) - return abbrev; + skip_prefix(name, prefix, &name); return name; } #define abbrev_branch(name) abbrev_ref((name), "refs/heads/") diff --git a/column.c b/column.c index 1a468debb4..ca878bcea7 100644 --- a/column.c +++ b/column.c @@ -336,8 +336,9 @@ static int column_config(const char *var, const char *value, int git_column_config(const char *var, const char *value, const char *command, unsigned int *colopts) { - const char *it = skip_prefix(var, "column."); - if (!it) + const char *it; + + if (!skip_prefix(var, "column.", &it)) return 0; if (!strcmp(it, "ui")) diff --git a/commit.c b/commit.c index 881be3baa3..dfc0eb0241 100644 --- a/commit.c +++ b/commit.c @@ -556,8 +556,7 @@ static void record_author_date(struct author_date_slab *author_date, buf; buf = line_end + 1) { line_end = strchrnul(buf, '\n'); - ident_line = skip_prefix(buf, "author "); - if (!ident_line) { + if (!skip_prefix(buf, "author ", &ident_line)) { if (!line_end[0] || line_end[1] == '\n') return; /* end of header */ continue; @@ -1183,8 +1182,7 @@ static void parse_gpg_output(struct signature_check *sigc) for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) { const char *found, *next; - found = skip_prefix(buf, sigcheck_gpg_status[i].check + 1); - if (!found) { + if (!skip_prefix(buf, sigcheck_gpg_status[i].check + 1, &found)) { found = strstr(buf, sigcheck_gpg_status[i].check); if (!found) continue; diff --git a/config.c b/config.c index a1aef1cf3e..ba882a1f51 100644 --- a/config.c +++ b/config.c @@ -138,8 +138,7 @@ int git_config_include(const char *var, const char *value, void *data) if (ret < 0) return ret; - type = skip_prefix(var, "include."); - if (!type) + if (!skip_prefix(var, "include.", &type)) return ret; if (!strcmp(type, "path")) diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c index 390f194252..3b370ca5e5 100644 --- a/credential-cache--daemon.c +++ b/credential-cache--daemon.c @@ -109,14 +109,12 @@ static int read_request(FILE *fh, struct credential *c, const char *p; strbuf_getline(&item, fh, '\n'); - p = skip_prefix(item.buf, "action="); - if (!p) + if (!skip_prefix(item.buf, "action=", &p)) return error("client sent bogus action line: %s", item.buf); strbuf_addstr(action, p); strbuf_getline(&item, fh, '\n'); - p = skip_prefix(item.buf, "timeout="); - if (!p) + if (!skip_prefix(item.buf, "timeout=", &p)) return error("client sent bogus timeout line: %s", item.buf); *timeout = atoi(p); diff --git a/credential.c b/credential.c index e54753c75d..4d79d320f8 100644 --- a/credential.c +++ b/credential.c @@ -40,8 +40,7 @@ static int credential_config_callback(const char *var, const char *value, struct credential *c = data; const char *key, *dot; - key = skip_prefix(var, "credential."); - if (!key) + if (!skip_prefix(var, "credential.", &key)) return 0; if (!value) diff --git a/fsck.c b/fsck.c index abed62bac7..bdbea2b995 100644 --- a/fsck.c +++ b/fsck.c @@ -278,20 +278,18 @@ static int fsck_ident(const char **ident, struct object *obj, fsck_error error_f static int fsck_commit(struct commit *commit, fsck_error error_func) { - const char *buffer = commit->buffer, *tmp; + const char *buffer = commit->buffer; unsigned char tree_sha1[20], sha1[20]; struct commit_graft *graft; int parents = 0; int err; - buffer = skip_prefix(buffer, "tree "); - if (!buffer) + if (!skip_prefix(buffer, "tree ", &buffer)) return error_func(&commit->object, FSCK_ERROR, "invalid format - expected 'tree' line"); if (get_sha1_hex(buffer, tree_sha1) || buffer[40] != '\n') return error_func(&commit->object, FSCK_ERROR, "invalid 'tree' line format - bad sha1"); buffer += 41; - while ((tmp = skip_prefix(buffer, "parent "))) { - buffer = tmp; + while (skip_prefix(buffer, "parent ", &buffer)) { if (get_sha1_hex(buffer, sha1) || buffer[40] != '\n') return error_func(&commit->object, FSCK_ERROR, "invalid 'parent' line format - bad sha1"); buffer += 41; @@ -318,14 +316,12 @@ static int fsck_commit(struct commit *commit, fsck_error error_func) if (p || parents) return error_func(&commit->object, FSCK_ERROR, "parent objects missing"); } - buffer = skip_prefix(buffer, "author "); - if (!buffer) + if (!skip_prefix(buffer, "author ", &buffer)) return error_func(&commit->object, FSCK_ERROR, "invalid format - expected 'author' line"); err = fsck_ident(&buffer, &commit->object, error_func); if (err) return err; - buffer = skip_prefix(buffer, "committer "); - if (!buffer) + if (!skip_prefix(buffer, "committer ", &buffer)) return error_func(&commit->object, FSCK_ERROR, "invalid format - expected 'committer' line"); err = fsck_ident(&buffer, &commit->object, error_func); if (err) diff --git a/git-compat-util.h b/git-compat-util.h index b6f03b36dc..d29e1dff08 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -349,13 +349,32 @@ extern void set_die_is_recursing_routine(int (*routine)(void)); extern int starts_with(const char *str, const char *prefix); extern int ends_with(const char *str, const char *suffix); -static inline const char *skip_prefix(const char *str, const char *prefix) +/* + * If the string "str" begins with the string found in "prefix", return 1. + * The "out" parameter is set to "str + strlen(prefix)" (i.e., to the point in + * the string right after the prefix). + * + * Otherwise, return 0 and leave "out" untouched. + * + * Examples: + * + * [extract branch name, fail if not a branch] + * if (!skip_prefix(ref, "refs/heads/", &branch) + * return -1; + * + * [skip prefix if present, otherwise use whole string] + * skip_prefix(name, "refs/heads/", &name); + */ +static inline int skip_prefix(const char *str, const char *prefix, + const char **out) { do { - if (!*prefix) - return str; + if (!*prefix) { + *out = str; + return 1; + } } while (*str++ == *prefix++); - return NULL; + return 0; } #if defined(NO_MMAP) || defined(USE_WIN32_MMAP) diff --git a/parse-options.c b/parse-options.c index b536896f26..e7dafa80d5 100644 --- a/parse-options.c +++ b/parse-options.c @@ -231,7 +231,8 @@ static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg, continue; again: - rest = skip_prefix(arg, long_name); + if (!skip_prefix(arg, long_name, &rest)) + rest = NULL; if (options->type == OPTION_ARGUMENT) { if (!rest) continue; @@ -280,12 +281,13 @@ is_abbreviated: continue; } flags |= OPT_UNSET; - rest = skip_prefix(arg + 3, long_name); - /* abbreviated and negated? */ - if (!rest && starts_with(long_name, arg + 3)) - goto is_abbreviated; - if (!rest) - continue; + if (!skip_prefix(arg + 3, long_name, &rest)) { + /* abbreviated and negated? */ + if (starts_with(long_name, arg + 3)) + goto is_abbreviated; + else + continue; + } } if (*rest) { if (*rest != '=') diff --git a/transport.c b/transport.c index 325f03e1ee..59c9727d8d 100644 --- a/transport.c +++ b/transport.c @@ -192,7 +192,9 @@ static void set_upstreams(struct transport *transport, struct ref *refs, static const char *rsync_url(const char *url) { - return !starts_with(url, "rsync://") ? skip_prefix(url, "rsync:") : url; + if (!starts_with(url, "rsync://")) + skip_prefix(url, "rsync:", &url); + return url; } static struct ref *get_refs_via_rsync(struct transport *transport, int for_push) diff --git a/urlmatch.c b/urlmatch.c index ec87cba750..3d4c54b5cd 100644 --- a/urlmatch.c +++ b/urlmatch.c @@ -483,8 +483,7 @@ int urlmatch_config_entry(const char *var, const char *value, void *cb) int user_matched = 0; int retval; - key = skip_prefix(var, collect->section); - if (!key || *(key++) != '.') { + if (!skip_prefix(var, collect->section, &key) || *(key++) != '.') { if (collect->cascade_fn) return collect->cascade_fn(var, value, cb); return 0; /* not interested */ From ce2ecf29241ca90655da37918044cbbaff48a2bd Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 18 Jun 2014 15:45:34 -0400 Subject: [PATCH 05/18] apply: use skip_prefix instead of raw addition A submodule diff generally has content like: -Subproject commit [0-9a-f]{40} +Subproject commit [0-9a-f]{40} When we are using "git apply --index" with a submodule, we first apply the textual diff, and then parse that result to figure out the new sha1. If the diff has bogus input like: -Subproject commit 1234567890123456789012345678901234567890 +bogus we will parse the "bogus" portion. Our parser assumes that the buffer starts with "Subproject commit", and blindly skips past it using strlen(). This can cause us to read random memory after the buffer. This problem was unlikely to have come up in practice (since it requires a malformed diff), and even when it did, we likely noticed the problem anyway as the next operation was to call get_sha1_hex on the random memory. However, we can easily fix it by using skip_prefix to notice the parsing error. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/apply.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/builtin/apply.c b/builtin/apply.c index 9c5724eacc..bc924ab2d0 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -3847,9 +3847,10 @@ static void add_index_file(const char *path, unsigned mode, void *buf, unsigned ce->ce_flags = create_ce_flags(0); ce->ce_namelen = namelen; if (S_ISGITLINK(mode)) { - const char *s = buf; + const char *s; - if (get_sha1_hex(s + strlen("Subproject commit "), ce->sha1)) + if (!skip_prefix(buf, "Subproject commit ", &s) || + get_sha1_hex(s, ce->sha1)) die(_("corrupt patch for submodule %s"), path); } else { if (!cached) { From ff45c0d4a316d620d118ec628dd8e78597a23321 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 18 Jun 2014 15:46:15 -0400 Subject: [PATCH 06/18] fast-import: fix read of uninitialized argv memory Fast-import shares code between its command-line parser and the "option" command. To do so, it strips the "--" from any command-line options and passes them to the option parser. However, it does not confirm that the option even begins with "--" before blindly passing "arg + 2". It does confirm that the option starts with "-", so the only affected case was: git fast-import - which would read uninitialized memory after the argument. We can fix it by using skip_prefix and checking the result. As a bonus, this gets rid of some magic numbers. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- fast-import.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/fast-import.c b/fast-import.c index 6707a66471..b2030ccfb4 100644 --- a/fast-import.c +++ b/fast-import.c @@ -3342,18 +3342,21 @@ static void parse_argv(void) if (*a != '-' || !strcmp(a, "--")) break; - if (parse_one_option(a + 2)) + if (!skip_prefix(a, "--", &a)) + die("unknown option %s", a); + + if (parse_one_option(a)) continue; - if (parse_one_feature(a + 2, 0)) + if (parse_one_feature(a, 0)) continue; - if (starts_with(a + 2, "cat-blob-fd=")) { - option_cat_blob_fd(a + 2 + strlen("cat-blob-fd=")); + if (skip_prefix(a, "cat-blob-fd=", &a)) { + option_cat_blob_fd(a); continue; } - die("unknown option %s", a); + die("unknown option --%s", a); } if (i != global_argc) usage(fast_import_usage); From 21a2d4ada52132e6b0b67f8e28aa4bcda416f7f2 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 18 Jun 2014 15:47:17 -0400 Subject: [PATCH 07/18] transport-helper: avoid reading past end-of-string We detect the "import-marks" capability by looking for that string, but _without_ a trailing space. Then we skip past it using strlen("import-marks "), with a space. So if a remote helper gives us exactly "import-marks", we will read past the end-of-string by one character. This is unlikely to be a problem in practice, because such input is malformed in the first place, and because there is a good chance that the string has an extra NUL terminator one character after the original (because it formerly had a newline in it that we parsed off). We can fix it by using skip_prefix with "import-marks ", with the space. The other form appears to be a typo from a515ebe (transport-helper: implement marks location as capability, 2011-07-16); "import-marks" has never existed without an argument, and it should match the "export-marks" definition above. Speaking of which, we can also use skip_prefix in a few other places while we are in the function. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- transport-helper.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/transport-helper.c b/transport-helper.c index 84c616f180..3d8fe7d801 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -153,7 +153,7 @@ static struct child_process *get_helper(struct transport *transport) write_constant(helper->in, "capabilities\n"); while (1) { - const char *capname; + const char *capname, *arg; int mandatory = 0; if (recvline(data, &buf)) exit(128); @@ -183,19 +183,19 @@ static struct child_process *get_helper(struct transport *transport) data->export = 1; else if (!strcmp(capname, "check-connectivity")) data->check_connectivity = 1; - else if (!data->refspecs && starts_with(capname, "refspec ")) { + else if (!data->refspecs && skip_prefix(capname, "refspec ", &arg)) { ALLOC_GROW(refspecs, refspec_nr + 1, refspec_alloc); - refspecs[refspec_nr++] = xstrdup(capname + strlen("refspec ")); + refspecs[refspec_nr++] = xstrdup(arg); } else if (!strcmp(capname, "connect")) { data->connect = 1; } else if (!strcmp(capname, "signed-tags")) { data->signed_tags = 1; - } else if (starts_with(capname, "export-marks ")) { - data->export_marks = xstrdup(capname + strlen("export-marks ")); - } else if (starts_with(capname, "import-marks")) { - data->import_marks = xstrdup(capname + strlen("import-marks ")); + } else if (skip_prefix(capname, "export-marks ", &arg)) { + data->export_marks = xstrdup(arg); + } else if (skip_prefix(capname, "import-marks ", &arg)) { + data->import_marks = xstrdup(arg); } else if (starts_with(capname, "no-private-update")) { data->no_private_update = 1; } else if (mandatory) { From ae021d87911da4328157273df24779892cb51277 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 18 Jun 2014 15:47:50 -0400 Subject: [PATCH 08/18] use skip_prefix to avoid magic numbers It's a common idiom to match a prefix and then skip past it with a magic number, like: if (starts_with(foo, "bar")) foo += 3; This is easy to get wrong, since you have to count the prefix string yourself, and there's no compiler check if the string changes. We can use skip_prefix to avoid the magic numbers here. Note that some of these conversions could be much shorter. For example: if (starts_with(arg, "--foo=")) { bar = arg + 6; continue; } could become: if (skip_prefix(arg, "--foo=", &bar)) continue; However, I have left it as: if (skip_prefix(arg, "--foo=", &v)) { bar = v; continue; } to visually match nearby cases which need to actually process the string. Like: if (skip_prefix(arg, "--foo=", &v)) { bar = atoi(v); continue; } Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- alias.c | 3 ++- connect.c | 11 ++++---- convert.c | 4 +-- daemon.c | 73 ++++++++++++++++++++++++++------------------------ diff.c | 65 +++++++++++++++++++++++--------------------- fast-import.c | 69 +++++++++++++++++++++++++---------------------- fetch-pack.c | 9 ++++--- git.c | 18 ++++++------- help.c | 6 +++-- http-backend.c | 11 +++++--- http-push.c | 11 ++++---- 11 files changed, 149 insertions(+), 131 deletions(-) diff --git a/alias.c b/alias.c index 5efc3d6986..758c867149 100644 --- a/alias.c +++ b/alias.c @@ -5,7 +5,8 @@ static char *alias_val; static int alias_lookup_cb(const char *k, const char *v, void *cb) { - if (starts_with(k, "alias.") && !strcmp(k + 6, alias_key)) { + const char *name; + if (skip_prefix(k, "alias.", &name) && !strcmp(name, alias_key)) { if (!v) return config_error_nonbool(k); alias_val = xstrdup(v); diff --git a/connect.c b/connect.c index 94a6650246..37ff018f13 100644 --- a/connect.c +++ b/connect.c @@ -129,6 +129,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len, char *name; int len, name_len; char *buffer = packet_buffer; + const char *arg; len = packet_read(in, &src_buf, &src_len, packet_buffer, sizeof(packet_buffer), @@ -140,12 +141,12 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len, if (!len) break; - if (len > 4 && starts_with(buffer, "ERR ")) - die("remote error: %s", buffer + 4); + if (len > 4 && skip_prefix(buffer, "ERR ", &arg)) + die("remote error: %s", arg); - if (len == 48 && starts_with(buffer, "shallow ")) { - if (get_sha1_hex(buffer + 8, old_sha1)) - die("protocol error: expected shallow sha-1, got '%s'", buffer + 8); + if (len == 48 && skip_prefix(buffer, "shallow ", &arg)) { + if (get_sha1_hex(arg, old_sha1)) + die("protocol error: expected shallow sha-1, got '%s'", arg); if (!shallow_points) die("repository on the other end cannot be shallow"); sha1_array_append(shallow_points, old_sha1); diff --git a/convert.c b/convert.c index ab80b72357..cb5fbb45ea 100644 --- a/convert.c +++ b/convert.c @@ -1121,9 +1121,9 @@ static int is_foreign_ident(const char *str) { int i; - if (!starts_with(str, "$Id: ")) + if (!skip_prefix(str, "$Id: ", &str)) return 0; - for (i = 5; str[i]; i++) { + for (i = 0; str[i]; i++) { if (isspace(str[i]) && str[i+1] != '$') return 1; } diff --git a/daemon.c b/daemon.c index 18818c3fda..6d25828a18 100644 --- a/daemon.c +++ b/daemon.c @@ -235,8 +235,10 @@ static int service_enabled; static int git_daemon_config(const char *var, const char *value, void *cb) { - if (starts_with(var, "daemon.") && - !strcmp(var + 7, service_looking_at->config_name)) { + const char *service; + + if (skip_prefix(var, "daemon.", &service) && + !strcmp(service, service_looking_at->config_name)) { service_enabled = git_config_bool(var, value); return 0; } @@ -1133,16 +1135,17 @@ int main(int argc, char **argv) for (i = 1; i < argc; i++) { char *arg = argv[i]; + const char *v; - if (starts_with(arg, "--listen=")) { - string_list_append(&listen_addr, xstrdup_tolower(arg + 9)); + if (skip_prefix(arg, "--listen=", &v)) { + string_list_append(&listen_addr, xstrdup_tolower(v)); continue; } - if (starts_with(arg, "--port=")) { + if (skip_prefix(arg, "--port=", &v)) { char *end; unsigned long n; - n = strtoul(arg+7, &end, 0); - if (arg[7] && !*end) { + n = strtoul(v, &end, 0); + if (*v && !*end) { listen_port = n; continue; } @@ -1168,20 +1171,20 @@ int main(int argc, char **argv) export_all_trees = 1; continue; } - if (starts_with(arg, "--access-hook=")) { - access_hook = arg + 14; + if (skip_prefix(arg, "--access-hook=", &v)) { + access_hook = v; continue; } - if (starts_with(arg, "--timeout=")) { - timeout = atoi(arg+10); + if (skip_prefix(arg, "--timeout=", &v)) { + timeout = atoi(v); continue; } - if (starts_with(arg, "--init-timeout=")) { - init_timeout = atoi(arg+15); + if (skip_prefix(arg, "--init-timeout=", &v)) { + init_timeout = atoi(v); continue; } - if (starts_with(arg, "--max-connections=")) { - max_connections = atoi(arg+18); + if (skip_prefix(arg, "--max-connections=", &v)) { + max_connections = atoi(v); if (max_connections < 0) max_connections = 0; /* unlimited */ continue; @@ -1190,16 +1193,16 @@ int main(int argc, char **argv) strict_paths = 1; continue; } - if (starts_with(arg, "--base-path=")) { - base_path = arg+12; + if (skip_prefix(arg, "--base-path=", &v)) { + base_path = v; continue; } if (!strcmp(arg, "--base-path-relaxed")) { base_path_relaxed = 1; continue; } - if (starts_with(arg, "--interpolated-path=")) { - interpolated_path = arg+20; + if (skip_prefix(arg, "--interpolated-path=", &v)) { + interpolated_path = v; continue; } if (!strcmp(arg, "--reuseaddr")) { @@ -1210,12 +1213,12 @@ int main(int argc, char **argv) user_path = ""; continue; } - if (starts_with(arg, "--user-path=")) { - user_path = arg + 12; + if (skip_prefix(arg, "--user-path=", &v)) { + user_path = v; continue; } - if (starts_with(arg, "--pid-file=")) { - pid_file = arg + 11; + if (skip_prefix(arg, "--pid-file=", &v)) { + pid_file = v; continue; } if (!strcmp(arg, "--detach")) { @@ -1223,28 +1226,28 @@ int main(int argc, char **argv) log_syslog = 1; continue; } - if (starts_with(arg, "--user=")) { - user_name = arg + 7; + if (skip_prefix(arg, "--user=", &v)) { + user_name = v; continue; } - if (starts_with(arg, "--group=")) { - group_name = arg + 8; + if (skip_prefix(arg, "--group=", &v)) { + group_name = v; continue; } - if (starts_with(arg, "--enable=")) { - enable_service(arg + 9, 1); + if (skip_prefix(arg, "--enable=", &v)) { + enable_service(v, 1); continue; } - if (starts_with(arg, "--disable=")) { - enable_service(arg + 10, 0); + if (skip_prefix(arg, "--disable=", &v)) { + enable_service(v, 0); continue; } - if (starts_with(arg, "--allow-override=")) { - make_service_overridable(arg + 17, 1); + if (skip_prefix(arg, "--allow-override=", &v)) { + make_service_overridable(v, 1); continue; } - if (starts_with(arg, "--forbid-override=")) { - make_service_overridable(arg + 18, 0); + if (skip_prefix(arg, "--forbid-override=", &v)) { + make_service_overridable(v, 0); continue; } if (!strcmp(arg, "--informative-errors")) { diff --git a/diff.c b/diff.c index 77c5eb4529..97db9323ee 100644 --- a/diff.c +++ b/diff.c @@ -231,6 +231,8 @@ int git_diff_ui_config(const char *var, const char *value, void *cb) int git_diff_basic_config(const char *var, const char *value, void *cb) { + const char *name; + if (!strcmp(var, "diff.renamelimit")) { diff_rename_limit_default = git_config_int(var, value); return 0; @@ -239,8 +241,9 @@ int git_diff_basic_config(const char *var, const char *value, void *cb) if (userdiff_config(var, value) < 0) return -1; - if (starts_with(var, "diff.color.") || starts_with(var, "color.diff.")) { - int slot = parse_diff_color_slot(var + 11); + if (skip_prefix(var, "diff.color.", &name) || + skip_prefix(var, "color.diff.", &name)) { + int slot = parse_diff_color_slot(name); if (slot < 0) return 0; if (!value) @@ -2341,6 +2344,7 @@ static void builtin_diff(const char *name_a, } else { /* Crazy xdl interfaces.. */ const char *diffopts = getenv("GIT_DIFF_OPTS"); + const char *v; xpparam_t xpp; xdemitconf_t xecfg; struct emit_callback ecbdata; @@ -2379,10 +2383,10 @@ static void builtin_diff(const char *name_a, xdiff_set_find_func(&xecfg, pe->pattern, pe->cflags); if (!diffopts) ; - else if (starts_with(diffopts, "--unified=")) - xecfg.ctxlen = strtoul(diffopts + 10, NULL, 10); - else if (starts_with(diffopts, "-u")) - xecfg.ctxlen = strtoul(diffopts + 2, NULL, 10); + else if (skip_prefix(diffopts, "--unified=", &v)) + xecfg.ctxlen = strtoul(v, NULL, 10); + else if (skip_prefix(diffopts, "-u", &v)) + xecfg.ctxlen = strtoul(v, NULL, 10); if (o->word_diff) init_diff_words_data(&ecbdata, o, one, two); xdi_diff_outf(&mf1, &mf2, fn_out_consume, &ecbdata, @@ -3609,17 +3613,17 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac) options->output_format |= DIFF_FORMAT_SHORTSTAT; else if (!strcmp(arg, "-X") || !strcmp(arg, "--dirstat")) return parse_dirstat_opt(options, ""); - else if (starts_with(arg, "-X")) - return parse_dirstat_opt(options, arg + 2); - else if (starts_with(arg, "--dirstat=")) - return parse_dirstat_opt(options, arg + 10); + else if (skip_prefix(arg, "-X", &arg)) + return parse_dirstat_opt(options, arg); + else if (skip_prefix(arg, "--dirstat=", &arg)) + return parse_dirstat_opt(options, arg); else if (!strcmp(arg, "--cumulative")) return parse_dirstat_opt(options, "cumulative"); else if (!strcmp(arg, "--dirstat-by-file")) return parse_dirstat_opt(options, "files"); - else if (starts_with(arg, "--dirstat-by-file=")) { + else if (skip_prefix(arg, "--dirstat-by-file=", &arg)) { parse_dirstat_opt(options, "files"); - return parse_dirstat_opt(options, arg + 18); + return parse_dirstat_opt(options, arg); } else if (!strcmp(arg, "--check")) options->output_format |= DIFF_FORMAT_CHECKDIFF; @@ -3669,9 +3673,9 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac) DIFF_OPT_CLR(options, RENAME_EMPTY); else if (!strcmp(arg, "--relative")) DIFF_OPT_SET(options, RELATIVE_NAME); - else if (starts_with(arg, "--relative=")) { + else if (skip_prefix(arg, "--relative=", &arg)) { DIFF_OPT_SET(options, RELATIVE_NAME); - options->prefix = arg + 11; + options->prefix = arg; } /* xdiff options */ @@ -3722,8 +3726,8 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac) DIFF_OPT_CLR(options, FOLLOW_RENAMES); else if (!strcmp(arg, "--color")) options->use_color = 1; - else if (starts_with(arg, "--color=")) { - int value = git_config_colorbool(NULL, arg+8); + else if (skip_prefix(arg, "--color=", &arg)) { + int value = git_config_colorbool(NULL, arg); if (value < 0) return error("option `color' expects \"always\", \"auto\", or \"never\""); options->use_color = value; @@ -3734,29 +3738,28 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac) options->use_color = 1; options->word_diff = DIFF_WORDS_COLOR; } - else if (starts_with(arg, "--color-words=")) { + else if (skip_prefix(arg, "--color-words=", &arg)) { options->use_color = 1; options->word_diff = DIFF_WORDS_COLOR; - options->word_regex = arg + 14; + options->word_regex = arg; } else if (!strcmp(arg, "--word-diff")) { if (options->word_diff == DIFF_WORDS_NONE) options->word_diff = DIFF_WORDS_PLAIN; } - else if (starts_with(arg, "--word-diff=")) { - const char *type = arg + 12; - if (!strcmp(type, "plain")) + else if (skip_prefix(arg, "--word-diff=", &arg)) { + if (!strcmp(arg, "plain")) options->word_diff = DIFF_WORDS_PLAIN; - else if (!strcmp(type, "color")) { + else if (!strcmp(arg, "color")) { options->use_color = 1; options->word_diff = DIFF_WORDS_COLOR; } - else if (!strcmp(type, "porcelain")) + else if (!strcmp(arg, "porcelain")) options->word_diff = DIFF_WORDS_PORCELAIN; - else if (!strcmp(type, "none")) + else if (!strcmp(arg, "none")) options->word_diff = DIFF_WORDS_NONE; else - die("bad --word-diff argument: %s", type); + die("bad --word-diff argument: %s", arg); } else if ((argcount = parse_long_opt("word-diff-regex", av, &optarg))) { if (options->word_diff == DIFF_WORDS_NONE) @@ -3779,13 +3782,13 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac) else if (!strcmp(arg, "--ignore-submodules")) { DIFF_OPT_SET(options, OVERRIDE_SUBMODULE_CONFIG); handle_ignore_submodules_arg(options, "all"); - } else if (starts_with(arg, "--ignore-submodules=")) { + } else if (skip_prefix(arg, "--ignore-submodules=", &arg)) { DIFF_OPT_SET(options, OVERRIDE_SUBMODULE_CONFIG); - handle_ignore_submodules_arg(options, arg + 20); + handle_ignore_submodules_arg(options, arg); } else if (!strcmp(arg, "--submodule")) DIFF_OPT_SET(options, SUBMODULE_LOG); - else if (starts_with(arg, "--submodule=")) - return parse_submodule_opt(options, arg + 12); + else if (skip_prefix(arg, "--submodule=", &arg)) + return parse_submodule_opt(options, arg); /* misc options */ else if (!strcmp(arg, "-z")) @@ -3820,8 +3823,8 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac) } else if (!strcmp(arg, "--abbrev")) options->abbrev = DEFAULT_ABBREV; - else if (starts_with(arg, "--abbrev=")) { - options->abbrev = strtoul(arg + 9, NULL, 10); + else if (skip_prefix(arg, "--abbrev=", &arg)) { + options->abbrev = strtoul(arg, NULL, 10); if (options->abbrev < MINIMUM_ABBREV) options->abbrev = MINIMUM_ABBREV; else if (40 < options->abbrev) diff --git a/fast-import.c b/fast-import.c index b2030ccfb4..a3ffe3063c 100644 --- a/fast-import.c +++ b/fast-import.c @@ -1912,8 +1912,9 @@ static void skip_optional_lf(void) static void parse_mark(void) { - if (starts_with(command_buf.buf, "mark :")) { - next_mark = strtoumax(command_buf.buf + 6, NULL, 10); + const char *v; + if (skip_prefix(command_buf.buf, "mark :", &v)) { + next_mark = strtoumax(v, NULL, 10); read_next_command(); } else @@ -1922,14 +1923,15 @@ static void parse_mark(void) static int parse_data(struct strbuf *sb, uintmax_t limit, uintmax_t *len_res) { + const char *data; strbuf_reset(sb); - if (!starts_with(command_buf.buf, "data ")) + if (!skip_prefix(command_buf.buf, "data ", &data)) die("Expected 'data n' command, found: %s", command_buf.buf); - if (starts_with(command_buf.buf + 5, "<<")) { - char *term = xstrdup(command_buf.buf + 5 + 2); - size_t term_len = command_buf.len - 5 - 2; + if (skip_prefix(data, "<<", &data)) { + char *term = xstrdup(data); + size_t term_len = command_buf.len - (data - command_buf.buf); strbuf_detach(&command_buf, NULL); for (;;) { @@ -1944,7 +1946,7 @@ static int parse_data(struct strbuf *sb, uintmax_t limit, uintmax_t *len_res) free(term); } else { - uintmax_t len = strtoumax(command_buf.buf + 5, NULL, 10); + uintmax_t len = strtoumax(data, NULL, 10); size_t n = 0, length = (size_t)len; if (limit && limit < len) { @@ -2676,6 +2678,7 @@ static void parse_new_commit(void) struct hash_list *merge_list = NULL; unsigned int merge_count; unsigned char prev_fanout, new_fanout; + const char *v; /* Obtain the branch name from the rest of our command */ sp = strchr(command_buf.buf, ' ') + 1; @@ -2685,12 +2688,12 @@ static void parse_new_commit(void) read_next_command(); parse_mark(); - if (starts_with(command_buf.buf, "author ")) { - author = parse_ident(command_buf.buf + 7); + if (skip_prefix(command_buf.buf, "author ", &v)) { + author = parse_ident(v); read_next_command(); } - if (starts_with(command_buf.buf, "committer ")) { - committer = parse_ident(command_buf.buf + 10); + if (skip_prefix(command_buf.buf, "committer ", &v)) { + committer = parse_ident(v); read_next_command(); } if (!committer) @@ -2777,6 +2780,7 @@ static void parse_new_tag(void) uintmax_t from_mark = 0; unsigned char sha1[20]; enum object_type type; + const char *v; /* Obtain the new tag name from the rest of our command */ sp = strchr(command_buf.buf, ' ') + 1; @@ -2819,8 +2823,8 @@ static void parse_new_tag(void) read_next_command(); /* tagger ... */ - if (starts_with(command_buf.buf, "tagger ")) { - tagger = parse_ident(command_buf.buf + 7); + if (skip_prefix(command_buf.buf, "tagger ", &v)) { + tagger = parse_ident(v); read_next_command(); } else tagger = NULL; @@ -3207,9 +3211,9 @@ static void option_export_pack_edges(const char *edges) static int parse_one_option(const char *option) { - if (starts_with(option, "max-pack-size=")) { + if (skip_prefix(option, "max-pack-size=", &option)) { unsigned long v; - if (!git_parse_ulong(option + 14, &v)) + if (!git_parse_ulong(option, &v)) return 0; if (v < 8192) { warning("max-pack-size is now in bytes, assuming --max-pack-size=%lum", v); @@ -3219,17 +3223,17 @@ static int parse_one_option(const char *option) v = 1024 * 1024; } max_packsize = v; - } else if (starts_with(option, "big-file-threshold=")) { + } else if (skip_prefix(option, "big-file-threshold=", &option)) { unsigned long v; - if (!git_parse_ulong(option + 19, &v)) + if (!git_parse_ulong(option, &v)) return 0; big_file_threshold = v; - } else if (starts_with(option, "depth=")) { - option_depth(option + 6); - } else if (starts_with(option, "active-branches=")) { - option_active_branches(option + 16); - } else if (starts_with(option, "export-pack-edges=")) { - option_export_pack_edges(option + 18); + } else if (skip_prefix(option, "depth=", &option)) { + option_depth(option); + } else if (skip_prefix(option, "active-branches=", &option)) { + option_active_branches(option); + } else if (skip_prefix(option, "export-pack-edges=", &option)) { + option_export_pack_edges(option); } else if (starts_with(option, "quiet")) { show_stats = 0; } else if (starts_with(option, "stats")) { @@ -3243,15 +3247,16 @@ static int parse_one_option(const char *option) static int parse_one_feature(const char *feature, int from_stream) { - if (starts_with(feature, "date-format=")) { - option_date_format(feature + 12); - } else if (starts_with(feature, "import-marks=")) { - option_import_marks(feature + 13, from_stream, 0); - } else if (starts_with(feature, "import-marks-if-exists=")) { - option_import_marks(feature + strlen("import-marks-if-exists="), - from_stream, 1); - } else if (starts_with(feature, "export-marks=")) { - option_export_marks(feature + 13); + const char *arg; + + if (skip_prefix(feature, "date-format=", &arg)) { + option_date_format(arg); + } else if (skip_prefix(feature, "import-marks=", &arg)) { + option_import_marks(arg, from_stream, 0); + } else if (skip_prefix(feature, "import-marks-if-exists=", &arg)) { + option_import_marks(arg, from_stream, 1); + } else if (skip_prefix(feature, "export-marks=", &arg)) { + option_export_marks(arg); } else if (!strcmp(feature, "cat-blob")) { ; /* Don't die - this feature is supported */ } else if (!strcmp(feature, "relative-marks")) { diff --git a/fetch-pack.c b/fetch-pack.c index eeee2bb7e0..3de3bd508e 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -319,18 +319,19 @@ static int find_common(struct fetch_pack_args *args, if (args->depth > 0) { char *line; + const char *arg; unsigned char sha1[20]; send_request(args, fd[1], &req_buf); while ((line = packet_read_line(fd[0], NULL))) { - if (starts_with(line, "shallow ")) { - if (get_sha1_hex(line + 8, sha1)) + if (skip_prefix(line, "shallow ", &arg)) { + if (get_sha1_hex(arg, sha1)) die("invalid shallow line: %s", line); register_shallow(sha1); continue; } - if (starts_with(line, "unshallow ")) { - if (get_sha1_hex(line + 10, sha1)) + if (skip_prefix(line, "unshallow ", &arg)) { + if (get_sha1_hex(arg, sha1)) die("invalid unshallow line: %s", line); if (!lookup_object(sha1)) die("object not found: %s", line); diff --git a/git.c b/git.c index 7780572948..b2bb09e40a 100644 --- a/git.c +++ b/git.c @@ -54,8 +54,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged) /* * Check remaining flags. */ - if (starts_with(cmd, "--exec-path")) { - cmd += 11; + if (skip_prefix(cmd, "--exec-path", &cmd)) { if (*cmd == '=') git_set_argv_exec_path(cmd + 1); else { @@ -92,8 +91,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged) *envchanged = 1; (*argv)++; (*argc)--; - } else if (starts_with(cmd, "--git-dir=")) { - setenv(GIT_DIR_ENVIRONMENT, cmd + 10, 1); + } else if (skip_prefix(cmd, "--git-dir=", &cmd)) { + setenv(GIT_DIR_ENVIRONMENT, cmd, 1); if (envchanged) *envchanged = 1; } else if (!strcmp(cmd, "--namespace")) { @@ -106,8 +105,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged) *envchanged = 1; (*argv)++; (*argc)--; - } else if (starts_with(cmd, "--namespace=")) { - setenv(GIT_NAMESPACE_ENVIRONMENT, cmd + 12, 1); + } else if (skip_prefix(cmd, "--namespace=", &cmd)) { + setenv(GIT_NAMESPACE_ENVIRONMENT, cmd, 1); if (envchanged) *envchanged = 1; } else if (!strcmp(cmd, "--work-tree")) { @@ -120,8 +119,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged) *envchanged = 1; (*argv)++; (*argc)--; - } else if (starts_with(cmd, "--work-tree=")) { - setenv(GIT_WORK_TREE_ENVIRONMENT, cmd + 12, 1); + } else if (skip_prefix(cmd, "--work-tree=", &cmd)) { + setenv(GIT_WORK_TREE_ENVIRONMENT, cmd, 1); if (envchanged) *envchanged = 1; } else if (!strcmp(cmd, "--bare")) { @@ -578,8 +577,7 @@ int main(int argc, char **av) * So we just directly call the builtin handler, and die if * that one cannot handle it. */ - if (starts_with(cmd, "git-")) { - cmd += 4; + if (skip_prefix(cmd, "git-", &cmd)) { argv[0] = cmd; handle_builtin(argc, argv); die("cannot handle %s as a builtin", cmd); diff --git a/help.c b/help.c index b266b09320..b0f1a69721 100644 --- a/help.c +++ b/help.c @@ -251,11 +251,13 @@ static struct cmdnames aliases; static int git_unknown_cmd_config(const char *var, const char *value, void *cb) { + const char *p; + if (!strcmp(var, "help.autocorrect")) autocorrect = git_config_int(var,value); /* Also use aliases for command lookup */ - if (starts_with(var, "alias.")) - add_cmdname(&aliases, var + 6, strlen(var + 6)); + if (skip_prefix(var, "alias.", &p)) + add_cmdname(&aliases, p, strlen(p)); return git_default_config(var, value, cb); } diff --git a/http-backend.c b/http-backend.c index d2c0a625ce..57290d9bda 100644 --- a/http-backend.c +++ b/http-backend.c @@ -221,17 +221,19 @@ static void get_idx_file(char *name) static int http_config(const char *var, const char *value, void *cb) { + const char *p; + if (!strcmp(var, "http.getanyfile")) { getanyfile = git_config_bool(var, value); return 0; } - if (starts_with(var, "http.")) { + if (skip_prefix(var, "http.", &p)) { int i; for (i = 0; i < ARRAY_SIZE(rpc_service); i++) { struct rpc_service *svc = &rpc_service[i]; - if (!strcmp(var + 5, svc->config_name)) { + if (!strcmp(p, svc->config_name)) { svc->enabled = git_config_bool(var, value); return 0; } @@ -244,15 +246,16 @@ static int http_config(const char *var, const char *value, void *cb) static struct rpc_service *select_service(const char *name) { + const char *svc_name; struct rpc_service *svc = NULL; int i; - if (!starts_with(name, "git-")) + if (!skip_prefix(name, "git-", &svc_name)) forbidden("Unsupported service: '%s'", name); for (i = 0; i < ARRAY_SIZE(rpc_service); i++) { struct rpc_service *s = &rpc_service[i]; - if (!strcmp(s->name, name + 4)) { + if (!strcmp(s->name, svc_name)) { svc = s; break; } diff --git a/http-push.c b/http-push.c index de00d1693a..26dfa6724e 100644 --- a/http-push.c +++ b/http-push.c @@ -770,9 +770,9 @@ static void handle_new_lock_ctx(struct xml_ctx *ctx, int tag_closed) lock->owner = xmalloc(strlen(ctx->cdata) + 1); strcpy(lock->owner, ctx->cdata); } else if (!strcmp(ctx->name, DAV_ACTIVELOCK_TIMEOUT)) { - if (starts_with(ctx->cdata, "Second-")) - lock->timeout = - strtol(ctx->cdata + 7, NULL, 10); + const char *arg; + if (skip_prefix(ctx->cdata, "Second-", &arg)) + lock->timeout = strtol(arg, NULL, 10); } else if (!strcmp(ctx->name, DAV_ACTIVELOCK_TOKEN)) { lock->token = xmalloc(strlen(ctx->cdata) + 1); strcpy(lock->token, ctx->cdata); @@ -1561,6 +1561,7 @@ static void fetch_symref(const char *path, char **symref, unsigned char *sha1) { char *url; struct strbuf buffer = STRBUF_INIT; + const char *name; url = xmalloc(strlen(repo->url) + strlen(path) + 1); sprintf(url, "%s%s", repo->url, path); @@ -1578,8 +1579,8 @@ static void fetch_symref(const char *path, char **symref, unsigned char *sha1) return; /* If it's a symref, set the refname; otherwise try for a sha1 */ - if (starts_with((char *)buffer.buf, "ref: ")) { - *symref = xmemdupz((char *)buffer.buf + 5, buffer.len - 6); + if (skip_prefix(buffer.buf, "ref: ", &name)) { + *symref = xmemdupz(name, buffer.len - (name - buffer.buf)); } else { get_sha1_hex(buffer.buf, sha1); } From 95b567c7c3cf6b85d74b79424cdfbd40a7dee7c9 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 18 Jun 2014 15:48:29 -0400 Subject: [PATCH 09/18] use skip_prefix to avoid repeating strings It's a common idiom to match a prefix and then skip past it with strlen, like: if (starts_with(foo, "bar")) foo += strlen("bar"); This avoids magic numbers, but means we have to repeat the string (and there is no compiler check that we didn't make a typo in one of the strings). We can use skip_prefix to handle this case without repeating ourselves. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/checkout.c | 4 ++-- builtin/fmt-merge-msg.c | 6 +++--- builtin/log.c | 12 ++++++------ diff.c | 27 +++++++++------------------ help.c | 7 ++++--- merge-recursive.c | 15 ++++++++------- pretty.c | 3 +-- remote-curl.c | 15 ++++++++------- remote.c | 5 ++--- sha1_name.c | 4 +--- 10 files changed, 44 insertions(+), 54 deletions(-) diff --git a/builtin/checkout.c b/builtin/checkout.c index f1dc56e55f..463cfeea50 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -776,8 +776,8 @@ static int switch_branches(const struct checkout_opts *opts, if (!(flag & REF_ISSYMREF)) old.path = NULL; - if (old.path && starts_with(old.path, "refs/heads/")) - old.name = old.path + strlen("refs/heads/"); + if (old.path) + skip_prefix(old.path, "refs/heads/", &old.name); if (!new->name) { new->name = "HEAD"; diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c index 3c19241554..ad3bc58c74 100644 --- a/builtin/fmt-merge-msg.c +++ b/builtin/fmt-merge-msg.c @@ -100,7 +100,8 @@ static int handle_line(char *line, struct merge_parents *merge_parents) { int i, len = strlen(line); struct origin_data *origin_data; - char *src, *origin; + char *src; + const char *origin; struct src_data *src_data; struct string_list_item *item; int pulling_head = 0; @@ -164,8 +165,7 @@ static int handle_line(char *line, struct merge_parents *merge_parents) origin = line; string_list_append(&src_data->tag, origin + 4); src_data->head_status |= 2; - } else if (starts_with(line, "remote-tracking branch ")) { - origin = line + strlen("remote-tracking branch "); + } else if (skip_prefix(line, "remote-tracking branch ", &origin)) { string_list_append(&src_data->r_branch, origin); src_data->head_status |= 2; } else { diff --git a/builtin/log.c b/builtin/log.c index a7ba211731..0f59c25d36 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -872,7 +872,7 @@ static char *find_branch_name(struct rev_info *rev) int i, positive = -1; unsigned char branch_sha1[20]; const unsigned char *tip_sha1; - const char *ref; + const char *ref, *v; char *full_ref, *branch = NULL; for (i = 0; i < rev->cmdline.nr; i++) { @@ -888,9 +888,9 @@ static char *find_branch_name(struct rev_info *rev) ref = rev->cmdline.rev[positive].name; tip_sha1 = rev->cmdline.rev[positive].item->sha1; if (dwim_ref(ref, strlen(ref), branch_sha1, &full_ref) && - starts_with(full_ref, "refs/heads/") && + skip_prefix(full_ref, "refs/heads/", &v) && !hashcmp(tip_sha1, branch_sha1)) - branch = xstrdup(full_ref + strlen("refs/heads/")); + branch = xstrdup(v); free(full_ref); return branch; } @@ -1394,10 +1394,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) if (check_head) { unsigned char sha1[20]; - const char *ref; + const char *ref, *v; ref = resolve_ref_unsafe("HEAD", sha1, 1, NULL); - if (ref && starts_with(ref, "refs/heads/")) - branch_name = xstrdup(ref + strlen("refs/heads/")); + if (ref && skip_prefix(ref, "refs/heads/", &v)) + branch_name = xstrdup(v); else branch_name = xstrdup(""); /* no branch */ } diff --git a/diff.c b/diff.c index 97db9323ee..2378ae49c6 100644 --- a/diff.c +++ b/diff.c @@ -3395,12 +3395,10 @@ int parse_long_opt(const char *opt, const char **argv, const char **optarg) { const char *arg = argv[0]; - if (arg[0] != '-' || arg[1] != '-') + if (!skip_prefix(arg, "--", &arg)) return 0; - arg += strlen("--"); - if (!starts_with(arg, opt)) + if (!skip_prefix(arg, opt, &arg)) return 0; - arg += strlen(opt); if (*arg == '=') { /* stuck form: --option=value */ *optarg = arg + 1; return 1; @@ -3429,8 +3427,7 @@ static int stat_opt(struct diff_options *options, const char **av) switch (*arg) { case '-': - if (starts_with(arg, "-width")) { - arg += strlen("-width"); + if (skip_prefix(arg, "-width", &arg)) { if (*arg == '=') width = strtoul(arg + 1, &end, 10); else if (!*arg && !av[1]) @@ -3439,8 +3436,7 @@ static int stat_opt(struct diff_options *options, const char **av) width = strtoul(av[1], &end, 10); argcount = 2; } - } else if (starts_with(arg, "-name-width")) { - arg += strlen("-name-width"); + } else if (skip_prefix(arg, "-name-width", &arg)) { if (*arg == '=') name_width = strtoul(arg + 1, &end, 10); else if (!*arg && !av[1]) @@ -3449,8 +3445,7 @@ static int stat_opt(struct diff_options *options, const char **av) name_width = strtoul(av[1], &end, 10); argcount = 2; } - } else if (starts_with(arg, "-graph-width")) { - arg += strlen("-graph-width"); + } else if (skip_prefix(arg, "-graph-width", &arg)) { if (*arg == '=') graph_width = strtoul(arg + 1, &end, 10); else if (!*arg && !av[1]) @@ -3459,8 +3454,7 @@ static int stat_opt(struct diff_options *options, const char **av) graph_width = strtoul(av[1], &end, 10); argcount = 2; } - } else if (starts_with(arg, "-count")) { - arg += strlen("-count"); + } else if (skip_prefix(arg, "-count", &arg)) { if (*arg == '=') count = strtoul(arg + 1, &end, 10); else if (!*arg && !av[1]) @@ -3905,16 +3899,13 @@ static int diff_scoreopt_parse(const char *opt) cmd = *opt++; if (cmd == '-') { /* convert the long-form arguments into short-form versions */ - if (starts_with(opt, "break-rewrites")) { - opt += strlen("break-rewrites"); + if (skip_prefix(opt, "break-rewrites", &opt)) { if (*opt == 0 || *opt++ == '=') cmd = 'B'; - } else if (starts_with(opt, "find-copies")) { - opt += strlen("find-copies"); + } else if (skip_prefix(opt, "find-copies", &opt)) { if (*opt == 0 || *opt++ == '=') cmd = 'C'; - } else if (starts_with(opt, "find-renames")) { - opt += strlen("find-renames"); + } else if (skip_prefix(opt, "find-renames", &opt)) { if (*opt == 0 || *opt++ == '=') cmd = 'M'; } diff --git a/help.c b/help.c index b0f1a69721..79e8007147 100644 --- a/help.c +++ b/help.c @@ -414,11 +414,12 @@ static int append_similar_ref(const char *refname, const unsigned char *sha1, { struct similar_ref_cb *cb = (struct similar_ref_cb *)(cb_data); char *branch = strrchr(refname, '/') + 1; + const char *remote; + /* A remote branch of the same name is deemed similar */ - if (starts_with(refname, "refs/remotes/") && + if (skip_prefix(refname, "refs/remotes/", &remote) && !strcmp(branch, cb->base_ref)) - string_list_append(cb->similar_refs, - refname + strlen("refs/remotes/")); + string_list_append(cb->similar_refs, remote); return 0; } diff --git a/merge-recursive.c b/merge-recursive.c index cab16fafb5..f848001817 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -2063,6 +2063,8 @@ void init_merge_options(struct merge_options *o) int parse_merge_opt(struct merge_options *o, const char *s) { + const char *arg; + if (!s || !*s) return -1; if (!strcmp(s, "ours")) @@ -2071,14 +2073,14 @@ int parse_merge_opt(struct merge_options *o, const char *s) o->recursive_variant = MERGE_RECURSIVE_THEIRS; else if (!strcmp(s, "subtree")) o->subtree_shift = ""; - else if (starts_with(s, "subtree=")) - o->subtree_shift = s + strlen("subtree="); + else if (skip_prefix(s, "subtree=", &arg)) + o->subtree_shift = arg; else if (!strcmp(s, "patience")) o->xdl_opts = DIFF_WITH_ALG(o, PATIENCE_DIFF); else if (!strcmp(s, "histogram")) o->xdl_opts = DIFF_WITH_ALG(o, HISTOGRAM_DIFF); - else if (starts_with(s, "diff-algorithm=")) { - long value = parse_algorithm_value(s + strlen("diff-algorithm=")); + else if (skip_prefix(s, "diff-algorithm=", &arg)) { + long value = parse_algorithm_value(arg); if (value < 0) return -1; /* clear out previous settings */ @@ -2096,9 +2098,8 @@ int parse_merge_opt(struct merge_options *o, const char *s) o->renormalize = 1; else if (!strcmp(s, "no-renormalize")) o->renormalize = 0; - else if (starts_with(s, "rename-threshold=")) { - const char *score = s + strlen("rename-threshold="); - if ((o->rename_score = parse_rename_score(&score)) == -1 || *score != 0) + else if (skip_prefix(s, "rename-threshold=", &arg)) { + if ((o->rename_score = parse_rename_score(&arg)) == -1 || *arg != 0) return -1; } else diff --git a/pretty.c b/pretty.c index 4f512876ce..f24752a85b 100644 --- a/pretty.c +++ b/pretty.c @@ -40,10 +40,9 @@ static int git_pretty_formats_config(const char *var, const char *value, void *c const char *fmt; int i; - if (!starts_with(var, "pretty.")) + if (!skip_prefix(var, "pretty.", &name)) return 0; - name = var + strlen("pretty."); for (i = 0; i < builtin_formats_len; i++) { if (!strcmp(commit_formats[i].name, name)) return 0; diff --git a/remote-curl.c b/remote-curl.c index 4493b389de..cdcca2903b 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -791,9 +791,9 @@ static void parse_fetch(struct strbuf *buf) int alloc_heads = 0, nr_heads = 0; do { - if (starts_with(buf->buf, "fetch ")) { - char *p = buf->buf + strlen("fetch "); - char *name; + const char *p; + if (skip_prefix(buf->buf, "fetch ", &p)) { + const char *name; struct ref *ref; unsigned char old_sha1[20]; @@ -968,6 +968,8 @@ int main(int argc, const char **argv) http_init(remote, url.buf, 0); do { + const char *arg; + if (strbuf_getline(&buf, stdin, '\n') == EOF) { if (ferror(stdin)) fprintf(stderr, "Error reading command stream\n"); @@ -989,9 +991,8 @@ int main(int argc, const char **argv) } else if (starts_with(buf.buf, "push ")) { parse_push(&buf); - } else if (starts_with(buf.buf, "option ")) { - char *name = buf.buf + strlen("option "); - char *value = strchr(name, ' '); + } else if (skip_prefix(buf.buf, "option ", &arg)) { + char *value = strchr(arg, ' '); int result; if (value) @@ -999,7 +1000,7 @@ int main(int argc, const char **argv) else value = "true"; - result = set_option(name, value); + result = set_option(arg, value); if (!result) printf("ok\n"); else if (result < 0) diff --git a/remote.c b/remote.c index 0e9459cc06..30d2829c2b 100644 --- a/remote.c +++ b/remote.c @@ -488,9 +488,8 @@ static void read_config(void) current_branch = NULL; head_ref = resolve_ref_unsafe("HEAD", sha1, 0, &flag); if (head_ref && (flag & REF_ISSYMREF) && - starts_with(head_ref, "refs/heads/")) { - current_branch = - make_branch(head_ref + strlen("refs/heads/"), 0); + skip_prefix(head_ref, "refs/heads/", &head_ref)) { + current_branch = make_branch(head_ref, 0); } git_config(handle_config, NULL); if (branch_pushremote_name) { diff --git a/sha1_name.c b/sha1_name.c index 2b6322fad0..72e6ac6a6e 100644 --- a/sha1_name.c +++ b/sha1_name.c @@ -911,10 +911,8 @@ static int grab_nth_branch_switch(unsigned char *osha1, unsigned char *nsha1, const char *match = NULL, *target = NULL; size_t len; - if (starts_with(message, "checkout: moving from ")) { - match = message + strlen("checkout: moving from "); + if (skip_prefix(message, "checkout: moving from ", &match)) target = strstr(match, " to "); - } if (!match || !target) return 0; From 97313bef2a16eb8b3d40830204c20c34ba9d6554 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 18 Jun 2014 15:49:12 -0400 Subject: [PATCH 10/18] fast-import: use skip_prefix for parsing input Fast-import does a lot of parsing of commands and dispatching to sub-functions. For example, given "option foo", we might recognize "option " using starts_with, and then hand it off to parse_option() to do the rest. However, we do not let parse_option know that we have parsed the first part already. It gets the full buffer, and has to skip past the uninteresting bits. Some functions simply add a magic constant: char *option = command_buf.buf + 7; Others use strlen: char *option = command_buf.buf + strlen("option "); And others use strchr: char *option = strchr(command_buf.buf, ' ') + 1; All of these are brittle and easy to get wrong (especially given that the starts_with call and the code that assumes the presence of the prefix are far apart). Instead, we can use skip_prefix, and just pass each handler a pointer to its arguments. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- fast-import.c | 123 +++++++++++++++++++++----------------------------- 1 file changed, 51 insertions(+), 72 deletions(-) diff --git a/fast-import.c b/fast-import.c index a3ffe3063c..c608fc68a0 100644 --- a/fast-import.c +++ b/fast-import.c @@ -371,8 +371,8 @@ static volatile sig_atomic_t checkpoint_requested; static int cat_blob_fd = STDOUT_FILENO; static void parse_argv(void); -static void parse_cat_blob(void); -static void parse_ls(struct branch *b); +static void parse_cat_blob(const char *p); +static void parse_ls(const char *p, struct branch *b); static void write_branch_report(FILE *rpt, struct branch *b) { @@ -1861,6 +1861,8 @@ static int read_next_command(void) } for (;;) { + const char *p; + if (unread_command_buf) { unread_command_buf = 0; } else { @@ -1893,8 +1895,8 @@ static int read_next_command(void) rc->prev->next = rc; cmd_tail = rc; } - if (starts_with(command_buf.buf, "cat-blob ")) { - parse_cat_blob(); + if (skip_prefix(command_buf.buf, "cat-blob ", &p)) { + parse_cat_blob(p); continue; } if (command_buf.buf[0] == '#') @@ -2273,9 +2275,8 @@ static uintmax_t parse_mark_ref_space(const char **p) return mark; } -static void file_change_m(struct branch *b) +static void file_change_m(const char *p, struct branch *b) { - const char *p = command_buf.buf + 2; static struct strbuf uq = STRBUF_INIT; const char *endp; struct object_entry *oe; @@ -2376,9 +2377,8 @@ static void file_change_m(struct branch *b) tree_content_set(&b->branch_tree, p, sha1, mode, NULL); } -static void file_change_d(struct branch *b) +static void file_change_d(const char *p, struct branch *b) { - const char *p = command_buf.buf + 2; static struct strbuf uq = STRBUF_INIT; const char *endp; @@ -2391,15 +2391,14 @@ static void file_change_d(struct branch *b) tree_content_remove(&b->branch_tree, p, NULL, 1); } -static void file_change_cr(struct branch *b, int rename) +static void file_change_cr(const char *s, struct branch *b, int rename) { - const char *s, *d; + const char *d; static struct strbuf s_uq = STRBUF_INIT; static struct strbuf d_uq = STRBUF_INIT; const char *endp; struct tree_entry leaf; - s = command_buf.buf + 2; strbuf_reset(&s_uq); if (!unquote_c_style(&s_uq, s, &endp)) { if (*endp != ' ') @@ -2444,9 +2443,8 @@ static void file_change_cr(struct branch *b, int rename) leaf.tree); } -static void note_change_n(struct branch *b, unsigned char *old_fanout) +static void note_change_n(const char *p, struct branch *b, unsigned char *old_fanout) { - const char *p = command_buf.buf + 2; static struct strbuf uq = STRBUF_INIT; struct object_entry *oe; struct branch *s; @@ -2587,7 +2585,7 @@ static int parse_from(struct branch *b) const char *from; struct branch *s; - if (!starts_with(command_buf.buf, "from ")) + if (!skip_prefix(command_buf.buf, "from ", &from)) return 0; if (b->branch_tree.tree) { @@ -2595,7 +2593,6 @@ static int parse_from(struct branch *b) b->branch_tree.tree = NULL; } - from = strchr(command_buf.buf, ' ') + 1; s = lookup_branch(from); if (b == s) die("Can't create a branch from itself: %s", b->name); @@ -2636,8 +2633,7 @@ static struct hash_list *parse_merge(unsigned int *count) struct branch *s; *count = 0; - while (starts_with(command_buf.buf, "merge ")) { - from = strchr(command_buf.buf, ' ') + 1; + while (skip_prefix(command_buf.buf, "merge ", &from)) { n = xmalloc(sizeof(*n)); s = lookup_branch(from); if (s) @@ -2668,11 +2664,10 @@ static struct hash_list *parse_merge(unsigned int *count) return list; } -static void parse_new_commit(void) +static void parse_new_commit(const char *arg) { static struct strbuf msg = STRBUF_INIT; struct branch *b; - char *sp; char *author = NULL; char *committer = NULL; struct hash_list *merge_list = NULL; @@ -2680,11 +2675,9 @@ static void parse_new_commit(void) unsigned char prev_fanout, new_fanout; const char *v; - /* Obtain the branch name from the rest of our command */ - sp = strchr(command_buf.buf, ' ') + 1; - b = lookup_branch(sp); + b = lookup_branch(arg); if (!b) - b = new_branch(sp); + b = new_branch(arg); read_next_command(); parse_mark(); @@ -2713,20 +2706,20 @@ static void parse_new_commit(void) /* file_change* */ while (command_buf.len > 0) { - if (starts_with(command_buf.buf, "M ")) - file_change_m(b); - else if (starts_with(command_buf.buf, "D ")) - file_change_d(b); - else if (starts_with(command_buf.buf, "R ")) - file_change_cr(b, 1); - else if (starts_with(command_buf.buf, "C ")) - file_change_cr(b, 0); - else if (starts_with(command_buf.buf, "N ")) - note_change_n(b, &prev_fanout); + if (skip_prefix(command_buf.buf, "M ", &v)) + file_change_m(v, b); + else if (skip_prefix(command_buf.buf, "D ", &v)) + file_change_d(v, b); + else if (skip_prefix(command_buf.buf, "R ", &v)) + file_change_cr(v, b, 1); + else if (skip_prefix(command_buf.buf, "C ", &v)) + file_change_cr(v, b, 0); + else if (skip_prefix(command_buf.buf, "N ", &v)) + note_change_n(v, b, &prev_fanout); else if (!strcmp("deleteall", command_buf.buf)) file_change_deleteall(b); - else if (starts_with(command_buf.buf, "ls ")) - parse_ls(b); + else if (skip_prefix(command_buf.buf, "ls ", &v)) + parse_ls(v, b); else { unread_command_buf = 1; break; @@ -2769,10 +2762,9 @@ static void parse_new_commit(void) b->last_commit = object_count_by_type[OBJ_COMMIT]; } -static void parse_new_tag(void) +static void parse_new_tag(const char *arg) { static struct strbuf msg = STRBUF_INIT; - char *sp; const char *from; char *tagger; struct branch *s; @@ -2782,11 +2774,9 @@ static void parse_new_tag(void) enum object_type type; const char *v; - /* Obtain the new tag name from the rest of our command */ - sp = strchr(command_buf.buf, ' ') + 1; t = pool_alloc(sizeof(struct tag)); memset(t, 0, sizeof(struct tag)); - t->name = pool_strdup(sp); + t->name = pool_strdup(arg); if (last_tag) last_tag->next_tag = t; else @@ -2795,9 +2785,8 @@ static void parse_new_tag(void) read_next_command(); /* from ... */ - if (!starts_with(command_buf.buf, "from ")) + if (!skip_prefix(command_buf.buf, "from ", &from)) die("Expected from command, got %s", command_buf.buf); - from = strchr(command_buf.buf, ' ') + 1; s = lookup_branch(from); if (s) { if (is_null_sha1(s->sha1)) @@ -2853,14 +2842,11 @@ static void parse_new_tag(void) t->pack_id = pack_id; } -static void parse_reset_branch(void) +static void parse_reset_branch(const char *arg) { struct branch *b; - char *sp; - /* Obtain the branch name from the rest of our command */ - sp = strchr(command_buf.buf, ' ') + 1; - b = lookup_branch(sp); + b = lookup_branch(arg); if (b) { hashclr(b->sha1); hashclr(b->branch_tree.versions[0].sha1); @@ -2871,7 +2857,7 @@ static void parse_reset_branch(void) } } else - b = new_branch(sp); + b = new_branch(arg); read_next_command(); parse_from(b); if (command_buf.len > 0) @@ -2929,14 +2915,12 @@ static void cat_blob(struct object_entry *oe, unsigned char sha1[20]) free(buf); } -static void parse_cat_blob(void) +static void parse_cat_blob(const char *p) { - const char *p; struct object_entry *oe = oe; unsigned char sha1[20]; /* cat-blob SP LF */ - p = command_buf.buf + strlen("cat-blob "); if (*p == ':') { oe = find_mark(parse_mark_ref_eol(p)); if (!oe) @@ -3053,14 +3037,12 @@ static void print_ls(int mode, const unsigned char *sha1, const char *path) cat_blob_write(line.buf, line.len); } -static void parse_ls(struct branch *b) +static void parse_ls(const char *p, struct branch *b) { - const char *p; struct tree_entry *root = NULL; struct tree_entry leaf = {NULL}; /* ls SP ( SP)? */ - p = command_buf.buf + strlen("ls "); if (*p == '"') { if (!b) die("Not in a commit: %s", command_buf.buf); @@ -3276,10 +3258,8 @@ static int parse_one_feature(const char *feature, int from_stream) return 1; } -static void parse_feature(void) +static void parse_feature(const char *feature) { - char *feature = command_buf.buf + 8; - if (seen_data_command) die("Got feature command '%s' after data command", feature); @@ -3289,10 +3269,8 @@ static void parse_feature(void) die("This version of fast-import does not support feature %s.", feature); } -static void parse_option(void) +static void parse_option(const char *option) { - char *option = command_buf.buf + 11; - if (seen_data_command) die("Got option command '%s' after data command", option); @@ -3408,26 +3386,27 @@ int main(int argc, char **argv) set_die_routine(die_nicely); set_checkpoint_signal(); while (read_next_command() != EOF) { + const char *v; if (!strcmp("blob", command_buf.buf)) parse_new_blob(); - else if (starts_with(command_buf.buf, "ls ")) - parse_ls(NULL); - else if (starts_with(command_buf.buf, "commit ")) - parse_new_commit(); - else if (starts_with(command_buf.buf, "tag ")) - parse_new_tag(); - else if (starts_with(command_buf.buf, "reset ")) - parse_reset_branch(); + else if (skip_prefix(command_buf.buf, "ls ", &v)) + parse_ls(v, NULL); + else if (skip_prefix(command_buf.buf, "commit ", &v)) + parse_new_commit(v); + else if (skip_prefix(command_buf.buf, "tag ", &v)) + parse_new_tag(v); + else if (skip_prefix(command_buf.buf, "reset ", &v)) + parse_reset_branch(v); else if (!strcmp("checkpoint", command_buf.buf)) parse_checkpoint(); else if (!strcmp("done", command_buf.buf)) break; else if (starts_with(command_buf.buf, "progress ")) parse_progress(); - else if (starts_with(command_buf.buf, "feature ")) - parse_feature(); - else if (starts_with(command_buf.buf, "option git ")) - parse_option(); + else if (skip_prefix(command_buf.buf, "feature ", &v)) + parse_feature(v); + else if (skip_prefix(command_buf.buf, "option git ", &v)) + parse_option(v); else if (starts_with(command_buf.buf, "option ")) /* ignore non-git options*/; else From d12c24d2a96f8f9aeb100b800d8fb217f28a6a2a Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 18 Jun 2014 15:49:44 -0400 Subject: [PATCH 11/18] daemon: use skip_prefix to avoid magic numbers Like earlier cases, we can use skip_prefix to avoid magic numbers that must match the length of starts_with prefixes. However, the numbers are a little more complicated here, as we keep parsing past the prefix. We can solve it by keeping a running pointer as we parse; its final value is the location we want. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- daemon.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/daemon.c b/daemon.c index 6d25828a18..1eb6631723 100644 --- a/daemon.c +++ b/daemon.c @@ -626,15 +626,16 @@ static int execute(void) for (i = 0; i < ARRAY_SIZE(daemon_service); i++) { struct daemon_service *s = &(daemon_service[i]); - int namelen = strlen(s->name); - if (starts_with(line, "git-") && - !strncmp(s->name, line + 4, namelen) && - line[namelen + 4] == ' ') { + const char *arg; + + if (skip_prefix(line, "git-", &arg) && + skip_prefix(arg, s->name, &arg) && + *arg++ == ' ') { /* * Note: The directory here is probably context sensitive, * and might depend on the actual service being performed. */ - return run_service(line + namelen + 5, s); + return run_service(arg, s); } } From 0539cc0038ff3411da1fea342b7d6615643bff5b Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 18 Jun 2014 15:51:17 -0400 Subject: [PATCH 12/18] stat_opt: check extra strlen call As in earlier commits, the diff option parser uses starts_with to find that an argument starts with "--stat-", and then adds strlen("stat-") to find the rest of the option. However, in this case the starts_with and the strlen are separated across functions, making it easy to call the latter without the former. Let's use skip_prefix instead of raw pointer arithmetic to catch such a case. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- diff.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/diff.c b/diff.c index 2378ae49c6..06bdfb8ae5 100644 --- a/diff.c +++ b/diff.c @@ -3422,7 +3422,8 @@ static int stat_opt(struct diff_options *options, const char **av) int count = options->stat_count; int argcount = 1; - arg += strlen("--stat"); + if (!skip_prefix(arg, "--stat", &arg)) + die("BUG: stat option does not begin with --stat: %s", arg); end = (char *)arg; switch (*arg) { From e814c39c2fe7cc915ba70c0aa6f03156a28920fc Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 18 Jun 2014 15:51:57 -0400 Subject: [PATCH 13/18] fast-import: refactor parsing of spaces When we see a file change in a commit, we expect one of: 1. A mark. 2. An "inline" keyword. 3. An object sha1. The handling of spaces is inconsistent between the three options. Option 1 calls a sub-function which checks for the space, but doesn't parse past it. Option 2 parses the space, then deliberately avoids moving the pointer past it. Option 3 detects the space locally but doesn't move past it. This is confusing, because it looks like option 1 forgets to check for the space (it's just buried). And option 2 checks for "inline ", but only moves strlen("inline") characters forward, which looks like a bug but isn't. We can make this more clear by just having each branch move past the space as it is checked (and we can replace the doubled use of "inline" with a call to skip_prefix). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- fast-import.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/fast-import.c b/fast-import.c index c608fc68a0..fa635f75c3 100644 --- a/fast-import.c +++ b/fast-import.c @@ -2269,7 +2269,7 @@ static uintmax_t parse_mark_ref_space(const char **p) char *end; mark = parse_mark_ref(*p, &end); - if (*end != ' ') + if (*end++ != ' ') die("Missing space after mark: %s", command_buf.buf); *p = end; return mark; @@ -2304,20 +2304,17 @@ static void file_change_m(const char *p, struct branch *b) if (*p == ':') { oe = find_mark(parse_mark_ref_space(&p)); hashcpy(sha1, oe->idx.sha1); - } else if (starts_with(p, "inline ")) { + } else if (skip_prefix(p, "inline ", &p)) { inline_data = 1; oe = NULL; /* not used with inline_data, but makes gcc happy */ - p += strlen("inline"); /* advance to space */ } else { if (get_sha1_hex(p, sha1)) die("Invalid dataref: %s", command_buf.buf); oe = find_object(sha1); p += 40; - if (*p != ' ') + if (*p++ != ' ') die("Missing space after SHA1: %s", command_buf.buf); } - assert(*p == ' '); - p++; /* skip space */ strbuf_reset(&uq); if (!unquote_c_style(&uq, p, &endp)) { @@ -2474,20 +2471,17 @@ static void note_change_n(const char *p, struct branch *b, unsigned char *old_fa if (*p == ':') { oe = find_mark(parse_mark_ref_space(&p)); hashcpy(sha1, oe->idx.sha1); - } else if (starts_with(p, "inline ")) { + } else if (skip_prefix(p, "inline ", &p)) { inline_data = 1; oe = NULL; /* not used with inline_data, but makes gcc happy */ - p += strlen("inline"); /* advance to space */ } else { if (get_sha1_hex(p, sha1)) die("Invalid dataref: %s", command_buf.buf); oe = find_object(sha1); p += 40; - if (*p != ' ') + if (*p++ != ' ') die("Missing space after SHA1: %s", command_buf.buf); } - assert(*p == ' '); - p++; /* skip space */ /* */ s = lookup_branch(p); @@ -3003,6 +2997,8 @@ static struct object_entry *parse_treeish_dataref(const char **p) die("Invalid dataref: %s", command_buf.buf); e = find_object(sha1); *p += 40; + if (*(*p)++ != ' ') + die("Missing space after tree-ish: %s", command_buf.buf); } while (!e || e->type != OBJ_TREE) @@ -3054,8 +3050,6 @@ static void parse_ls(const char *p, struct branch *b) if (!is_null_sha1(root->versions[1].sha1)) root->versions[1].mode = S_IFDIR; load_tree(root); - if (*p++ != ' ') - die("Missing space after tree-ish: %s", command_buf.buf); } if (*p == '"') { static struct strbuf uq = STRBUF_INIT; From 82e56767aa9334bb0c4cfe81964a576778b93d6e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 18 Jun 2014 15:56:03 -0400 Subject: [PATCH 14/18] fetch-pack: refactor parsing in get_ack There are several uses of the magic number "line+45" when parsing ACK lines from the server, and it's rather unclear why 45 is the correct number. We can make this more clear by keeping a running pointer as we parse, using skip_prefix to jump past the first "ACK ", then adding 40 to jump past get_sha1_hex (which is still magical, but hopefully 40 is less magical to readers of git code). Note that this actually puts us at line+44. The original required some character between the sha1 and further ACK flags (it is supposed to be a space, but we never enforced that). We start our search for flags at line+44, which meanas we are slightly more liberal than the old code. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- fetch-pack.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/fetch-pack.c b/fetch-pack.c index 3de3bd508e..72ec520fda 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -189,20 +189,23 @@ static enum ack_type get_ack(int fd, unsigned char *result_sha1) { int len; char *line = packet_read_line(fd, &len); + const char *arg; if (!len) die("git fetch-pack: expected ACK/NAK, got EOF"); if (!strcmp(line, "NAK")) return NAK; - if (starts_with(line, "ACK ")) { - if (!get_sha1_hex(line+4, result_sha1)) { - if (len < 45) + if (skip_prefix(line, "ACK ", &arg)) { + if (!get_sha1_hex(arg, result_sha1)) { + arg += 40; + len -= arg - line; + if (len < 1) return ACK; - if (strstr(line+45, "continue")) + if (strstr(arg, "continue")) return ACK_continue; - if (strstr(line+45, "common")) + if (strstr(arg, "common")) return ACK_common; - if (strstr(line+45, "ready")) + if (strstr(arg, "ready")) return ACK_ready; return ACK; } From 6d87780399e1196932cd5ea72f84aa90224e7402 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 18 Jun 2014 15:56:48 -0400 Subject: [PATCH 15/18] git: avoid magic number with skip_prefix After handling options, any leftover arguments should be commands. However, we pass through "--help" and "--version", so that we convert them into "git help" and "git version" respectively. This is a straightforward use of skip_prefix to avoid a magic number, but while we are there, it is worth adding a comment to explain this otherwise confusing behavior. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- git.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/git.c b/git.c index b2bb09e40a..1537f00bc7 100644 --- a/git.c +++ b/git.c @@ -588,8 +588,8 @@ int main(int argc, char **av) argc--; handle_options(&argv, &argc, NULL); if (argc > 0) { - if (starts_with(argv[0], "--")) - argv[0] += 2; + /* translate --help and --version into commands */ + skip_prefix(argv[0], "--", &argv[0]); } else { /* The user didn't specify a command; give them help */ commit_pager_choice(); From de8118e153c5e527263086605e437ccca5d4f1ef Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 18 Jun 2014 15:57:17 -0400 Subject: [PATCH 16/18] use skip_prefix to avoid repeated calculations In some cases, we use starts_with to check for a prefix, and then use an already-calculated prefix length to advance a pointer past the prefix. There are no magic numbers or duplicated strings here, but we can still make the code simpler and more obvious by using skip_prefix. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- help.c | 11 +++++------ http.c | 3 +-- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/help.c b/help.c index 79e8007147..f31f29ac42 100644 --- a/help.c +++ b/help.c @@ -129,7 +129,6 @@ static void list_commands_in_dir(struct cmdnames *cmds, const char *path, const char *prefix) { - int prefix_len; DIR *dir = opendir(path); struct dirent *de; struct strbuf buf = STRBUF_INIT; @@ -139,15 +138,15 @@ static void list_commands_in_dir(struct cmdnames *cmds, return; if (!prefix) prefix = "git-"; - prefix_len = strlen(prefix); strbuf_addf(&buf, "%s/", path); len = buf.len; while ((de = readdir(dir)) != NULL) { + const char *ent; int entlen; - if (!starts_with(de->d_name, prefix)) + if (!skip_prefix(de->d_name, prefix, &ent)) continue; strbuf_setlen(&buf, len); @@ -155,11 +154,11 @@ static void list_commands_in_dir(struct cmdnames *cmds, if (!is_executable(buf.buf)) continue; - entlen = strlen(de->d_name) - prefix_len; - if (has_extension(de->d_name, ".exe")) + entlen = strlen(ent); + if (has_extension(ent, ".exe")) entlen -= 4; - add_cmdname(cmds, de->d_name + prefix_len, entlen); + add_cmdname(cmds, ent, entlen); } closedir(dir); strbuf_release(&buf); diff --git a/http.c b/http.c index 2b4f6a357c..f04621e61d 100644 --- a/http.c +++ b/http.c @@ -1087,11 +1087,10 @@ static int update_url_from_redirect(struct strbuf *base, if (!strcmp(asked, got->buf)) return 0; - if (!starts_with(asked, base->buf)) + if (!skip_prefix(asked, base->buf, &tail)) die("BUG: update_url_from_redirect: %s is not a superset of %s", asked, base->buf); - tail = asked + base->len; tail_len = strlen(tail); if (got->len < tail_len || From 59a642f8ac42c6cd11f63175689aa21b27ce1a6a Mon Sep 17 00:00:00 2001 From: Tanay Abhra Date: Thu, 19 Jun 2014 05:22:46 +0000 Subject: [PATCH 17/18] imap-send: use skip_prefix instead of using magic numbers Signed-off-by: Tanay Abhra Signed-off-by: Junio C Hamano --- imap-send.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/imap-send.c b/imap-send.c index 83a6ed2ac3..524fbabc96 100644 --- a/imap-send.c +++ b/imap-send.c @@ -1328,13 +1328,9 @@ static char *imap_folder; static int git_imap_config(const char *key, const char *val, void *cb) { - char imap_key[] = "imap."; - - if (strncmp(key, imap_key, sizeof imap_key - 1)) + if (!skip_prefix(key, "imap.", &key)) return 0; - key += sizeof imap_key - 1; - /* check booleans first, and barf on others */ if (!strcmp("sslverify", key)) server.ssl_verify = git_config_bool(key, val); From 67a31f612830e79fe768ac886ed9ef7eadd8fb10 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 19 Jun 2014 17:58:10 -0400 Subject: [PATCH 18/18] http-push: refactor parsing of remote object names We get loose object names like "objects/??/..." from the remote side, and need to convert them to their hex representation. The code to do so is rather hard to follow, as it uses some calculated lengths whose origins are hard to understand and verify (e.g., the path must be exactly 49 characters long. why? Why doesn't the strcpy overflow obj_hex, which is the same length as path?). We can simplify this a bit by using skip_prefix, using standard 40- and 20-character buffers for hex and binary sha1s, and adding some comments. We also drop a totally bogus comment that claims strlcpy cannot be used because "path" is not NUL-terminated. Right between a call to strlen(path) and strcpy(path). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- http-push.c | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/http-push.c b/http-push.c index 26dfa6724e..c5c95e85ea 100644 --- a/http-push.c +++ b/http-push.c @@ -719,14 +719,10 @@ static int fetch_indices(void) return ret; } -static void one_remote_object(const char *hex) +static void one_remote_object(const unsigned char *sha1) { - unsigned char sha1[20]; struct object *obj; - if (get_sha1_hex(hex, sha1) != 0) - return; - obj = lookup_object(sha1); if (!obj) obj = parse_object(sha1); @@ -1020,26 +1016,38 @@ static void remote_ls(const char *path, int flags, void (*userFunc)(struct remote_ls_ctx *ls), void *userData); +/* extract hex from sharded "xx/x{40}" filename */ +static int get_sha1_hex_from_objpath(const char *path, unsigned char *sha1) +{ + char hex[40]; + + if (strlen(path) != 41) + return -1; + + memcpy(hex, path, 2); + path += 2; + path++; /* skip '/' */ + memcpy(hex, path, 38); + + return get_sha1_hex(hex, sha1); +} + static void process_ls_object(struct remote_ls_ctx *ls) { unsigned int *parent = (unsigned int *)ls->userData; - char *path = ls->dentry_name; - char *obj_hex; + const char *path = ls->dentry_name; + unsigned char sha1[20]; if (!strcmp(ls->path, ls->dentry_name) && (ls->flags & IS_DIR)) { remote_dir_exists[*parent] = 1; return; } - if (strlen(path) != 49) + if (!skip_prefix(path, "objects/", &path) || + get_sha1_hex_from_objpath(path, sha1)) return; - path += 8; - obj_hex = xmalloc(strlen(path)); - /* NB: path is not null-terminated, can not use strlcpy here */ - memcpy(obj_hex, path, 2); - strcpy(obj_hex + 2, path + 3); - one_remote_object(obj_hex); - free(obj_hex); + + one_remote_object(sha1); } static void process_ls_ref(struct remote_ls_ctx *ls)