From c0264180d74510669bcec9bb5ff12a145c685b8e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 18 Jun 2014 15:42:14 -0400 Subject: 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(-) (limited to 'builtin') 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); -- cgit v1.2.1 From cf4fff579e02d8585d59f6c8739534b7b0d617dd Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 18 Jun 2014 15:44:19 -0400 Subject: 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 --- 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 +--- 6 files changed, 17 insertions(+), 18 deletions(-) (limited to 'builtin') 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/") -- cgit v1.2.1 From ce2ecf29241ca90655da37918044cbbaff48a2bd Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 18 Jun 2014 15:45:34 -0400 Subject: 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(-) (limited to 'builtin') 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) { -- cgit v1.2.1 From 95b567c7c3cf6b85d74b79424cdfbd40a7dee7c9 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 18 Jun 2014 15:48:29 -0400 Subject: 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 ++++++------ 3 files changed, 11 insertions(+), 11 deletions(-) (limited to 'builtin') 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 */ } -- cgit v1.2.1