This series cleans up populate_value() in ref-filter, by moving out the parsing part of atoms to separate parsing functions. This ensures that parsing is only done once and also improves the modularity of the code. v1: http://thread.gmane.org/gmane.comp.version-control.git/281180 v2: http://thread.gmane.org/gmane.comp.version-control.git/282563 v3: http://thread.gmane.org/gmane.comp.version-control.git/283350 Changes: * The parsing functions now take the arguments of the atom as function parameteres, instead of parsing it inside the fucntion. * Rebased on top of pu:jk/list-tag-2.7-regression * In strbuf use a copylen variable rather than using multiplication to perform a logical operation. * Code movement for easier review and general improvement. * Use COLOR_MAXLEN as the maximum size for the color variable. * Small code changes. * Documentation changes. * Fixed incorrect style of test (t6302). Karthik Nayak (12): strbuf: introduce strbuf_split_str_omit_term() ref-filter: use strbuf_split_str_omit_term() ref-filter: bump 'used_atom' and related code to the top ref-filter: introduce struct used_atom ref-filter: introduce parsing functions for each valid atom ref-filter: introduce color_atom_parser() ref-filter: introduce parse_align_position() ref-filter: introduce align_atom_parser() ref-filter: align: introduce long-form syntax ref-filter: introduce remote_ref_atom_parser() ref-filter: introduce contents_atom_parser() ref-filter: introduce objectname_atom_parser() Interdiff: diff --git a/ref-filter.c b/ref-filter.c index 7a634e6..d48e2a3 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -35,7 +35,7 @@ static struct used_atom { const char *name; cmp_type type; union { - char *color; + char color[COLOR_MAXLEN]; struct align align; enum { RR_NORMAL, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT } remote_ref; @@ -49,99 +49,68 @@ static struct used_atom { static int used_atom_cnt, need_tagged, need_symref; static int need_color_reset_at_eol; -static int match_atom_name(const char *name, const char *atom_name, const char **val) +static void color_atom_parser(struct used_atom *atom, const char *color_value) { - const char *body; - - /* skip the deref specifier */ - if (name[0] == '*') - name++; - - if (!skip_prefix(name, atom_name, &body)) - return 0; /* doesn't even begin with "atom_name" */ - if (!body[0]) { - *val = NULL; /* %(atom_name) and no customization */ - return 1; - } - if (body[0] != ':') - return 0; /* "atom_namefoo" is not "atom_name" or "atom_name:..." */ - *val = body + 1; /* "atom_name:val" */ - return 1; -} - -static void color_atom_parser(struct used_atom *atom) -{ - if (!match_atom_name(atom->name, "color", (const char **)&atom->u.color)) - die("BUG: parsing non-'color'"); - if (!atom->u.color) + if (!color_value) die(_("expected format: %%(color:<color>)")); - /* atom->u.color points to part of atom->name */ - atom->u.color = xstrdup(atom->u.color); - if (color_parse(atom->u.color, atom->u.color) < 0) + if (color_parse(color_value, atom->u.color) < 0) die(_("invalid color value: %s"), atom->u.color); } -static void remote_ref_atom_parser(struct used_atom *atom) +static void remote_ref_atom_parser(struct used_atom *atom, const char *arg) { - const char *buf; - - buf = strchr(atom->name, ':'); - if (!buf) { + if (!arg) { atom->u.remote_ref = RR_NORMAL; - return; - } - buf++; - if (!strcmp(buf, "short")) + } else if (!strcmp(arg, "short")) atom->u.remote_ref = RR_SHORTEN; - else if (!strcmp(buf, "track")) + else if (!strcmp(arg, "track")) atom->u.remote_ref = RR_TRACK; - else if (!strcmp(buf, "trackshort")) + else if (!strcmp(arg, "trackshort")) atom->u.remote_ref = RR_TRACKSHORT; else die(_("unrecognized format: %%(%s)"), atom->name); } -static void contents_atom_parser(struct used_atom *atom) +static void body_atom_parser(struct used_atom *atom, const char *arg) { - const char * buf; + if (arg) + die("%%(body) atom does not take arguments"); + atom->u.contents.option = C_BODY_DEP; +} - if (match_atom_name(atom->name, "subject", &buf)) { - atom->u.contents.option = C_SUB; - return; - } else if (match_atom_name(atom->name, "body", &buf)) { - atom->u.contents.option = C_BODY_DEP; - return; - } if (!match_atom_name(atom->name, "contents", &buf)) - die("BUG: parsing non-'contents'"); +static void subject_atom_parser(struct used_atom *atom, const char *arg) +{ + if (arg) + die("%%(subject) atom does not take arguments"); + atom->u.contents.option = C_SUB; +} - if (!buf) +static void contents_atom_parser(struct used_atom *atom, const char *arg) +{ + if (!arg) atom->u.contents.option = C_BARE; - else if (!strcmp(buf, "body")) + else if (!strcmp(arg, "body")) atom->u.contents.option = C_BODY; - else if (!strcmp(buf, "signature")) + else if (!strcmp(arg, "signature")) atom->u.contents.option = C_SIG; - else if (!strcmp(buf, "subject")) + else if (!strcmp(arg, "subject")) atom->u.contents.option = C_SUB; - else if (skip_prefix(buf, "lines=", &buf)) { + else if (skip_prefix(arg, "lines=", &arg)) { atom->u.contents.option = C_LINES; - if (strtoul_ui(buf, 10, &atom->u.contents.nlines)) - die(_("positive value expected contents:lines=%s"), buf); + if (strtoul_ui(arg, 10, &atom->u.contents.nlines)) + die(_("positive value expected contents:lines=%s"), arg); } else - die(_("unrecognized %%(contents) argument: %s"), buf); + die(_("unrecognized %%(contents) argument: %s"), arg); } -static void objectname_atom_parser(struct used_atom *atom) +static void objectname_atom_parser(struct used_atom *atom, const char *arg) { - const char * buf; - - if (!match_atom_name(atom->name, "objectname", &buf)) - die("BUG: parsing non-'objectname'"); - if (!buf) + if (!arg) atom->u.objectname = O_FULL; - else if (!strcmp(buf, "short")) + else if (!strcmp(arg, "short")) atom->u.objectname = O_SHORT; else - die(_("unrecognized %%(objectname) argument: %s"), buf); + die(_("unrecognized %%(objectname) argument: %s"), arg); } static align_type parse_align_position(const char *s) @@ -155,39 +124,36 @@ static align_type parse_align_position(const char *s) return -1; } -static void align_atom_parser(struct used_atom *atom) +static void align_atom_parser(struct used_atom *atom, const char *arg) { struct align *align = &atom->u.align; - const char *buf = NULL; struct strbuf **s, **to_free; unsigned int width = ~0U; - if (!match_atom_name(atom->name, "align", &buf)) - die("BUG: parsing non-'align'"); - if (!buf) + if (!arg) die(_("expected format: %%(align:<width>,<position>)")); - s = to_free = strbuf_split_str_omit_term(buf, ',', 0); + s = to_free = strbuf_split_str_omit_term(arg, ',', 0); align->position = ALIGN_LEFT; while (*s) { int position; - buf = s[0]->buf; + arg = s[0]->buf; - if (skip_prefix(buf, "position=", &buf)) { - position = parse_align_position(buf); + if (skip_prefix(arg, "position=", &arg)) { + position = parse_align_position(arg); if (position < 0) - die(_("unrecognized position:%s"), buf); + die(_("unrecognized position:%s"), arg); align->position = position; - } else if (skip_prefix(buf, "width=", &buf)) { - if (strtoul_ui(buf, 10, &width)) - die(_("unrecognized width:%s"), buf); - } else if (!strtoul_ui(buf, 10, &width)) + } else if (skip_prefix(arg, "width=", &arg)) { + if (strtoul_ui(arg, 10, &width)) + die(_("unrecognized width:%s"), arg); + } else if (!strtoul_ui(arg, 10, &width)) ; - else if ((position = parse_align_position(buf)) >= 0) + else if ((position = parse_align_position(arg)) >= 0) align->position = position; else - die(_("unrecognized %%(align) argument: %s"), buf); + die(_("unrecognized %%(align) argument: %s"), arg); s++; } @@ -200,7 +166,7 @@ static void align_atom_parser(struct used_atom *atom) static struct { const char *name; cmp_type cmp_type; - void (*parser)(struct used_atom *atom); + void (*parser)(struct used_atom *atom, const char *arg); } valid_atom[] = { { "refname" }, { "objecttype" }, @@ -226,8 +192,8 @@ static struct { { "taggerdate", FIELD_TIME }, { "creator" }, { "creatordate", FIELD_TIME }, - { "subject", FIELD_STR, contents_atom_parser }, - { "body", FIELD_STR, contents_atom_parser }, + { "subject", FIELD_STR, subject_atom_parser }, + { "body", FIELD_STR, body_atom_parser }, { "contents", FIELD_STR, contents_atom_parser }, { "upstream", FIELD_STR, remote_ref_atom_parser }, { "push", FIELD_STR, remote_ref_atom_parser }, @@ -268,6 +234,7 @@ struct atom_value { int parse_ref_filter_atom(const char *atom, const char *ep) { const char *sp; + const char *arg; int i, at; sp = atom; @@ -292,10 +259,9 @@ int parse_ref_filter_atom(const char *atom, const char *ep) * shouldn't be used for checking against the valid_atom * table. */ - const char *formatp = strchr(sp, ':'); - if (!formatp || ep < formatp) - formatp = ep; - if (len == formatp - sp && !memcmp(valid_atom[i].name, sp, len)) + arg = memchr(sp, ':', ep - sp); + if ((!arg || len == arg - sp) && + !memcmp(valid_atom[i].name, sp, len)) break; } @@ -308,9 +274,11 @@ int parse_ref_filter_atom(const char *atom, const char *ep) REALLOC_ARRAY(used_atom, used_atom_cnt); used_atom[at].name = xmemdupz(atom, ep - atom); used_atom[at].type = valid_atom[i].cmp_type; + if (arg) + arg = used_atom[at].name + (arg - atom) + 1; memset(&used_atom[at].u, 0, sizeof(used_atom[at].u)); if (valid_atom[i].parser) - valid_atom[i].parser(&used_atom[at]); + valid_atom[i].parser(&used_atom[at], arg); if (*atom == '*') need_tagged = 1; if (!strcmp(used_atom[at].name, "symref")) @@ -516,7 +484,7 @@ static void grab_common_values(struct atom_value *val, int deref, struct object v->s = xstrfmt("%lu", sz); } else if (deref) - grab_objectname(name, obj->sha1, v, &used_atom[i]); + grab_objectname(name, obj->oid.hash, v, &used_atom[i]); } } @@ -538,7 +506,7 @@ static void grab_tag_values(struct atom_value *val, int deref, struct object *ob else if (!strcmp(name, "type") && tag->tagged) v->s = typename(tag->tagged->type); else if (!strcmp(name, "object") && tag->tagged) - v->s = xstrdup(sha1_to_hex(tag->tagged->sha1)); + v->s = xstrdup(oid_to_hex(&tag->tagged->oid)); } } @@ -556,7 +524,7 @@ static void grab_commit_values(struct atom_value *val, int deref, struct object if (deref) name++; if (!strcmp(name, "tree")) { - v->s = xstrdup(sha1_to_hex(commit->tree->object.sha1)); + v->s = xstrdup(oid_to_hex(&commit->tree->object.oid)); } else if (!strcmp(name, "numparent")) { v->ul = commit_list_count(commit->parents); @@ -569,7 +537,7 @@ static void grab_commit_values(struct atom_value *val, int deref, struct object struct commit *parent = parents->item; if (parents != commit->parents) strbuf_addch(&s, ' '); - strbuf_addstr(&s, sha1_to_hex(parent->object.sha1)); + strbuf_addstr(&s, oid_to_hex(&parent->object.oid)); } v->s = strbuf_detach(&s, NULL); } @@ -931,7 +922,7 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname, *s = ">"; else *s = "<>"; - } else if (atom->u.remote_ref == RR_NORMAL) + } else /* RR_NORMAL */ *s = refname; } @@ -986,9 +977,8 @@ static void populate_value(struct ref_array_item *ref) branch = branch_get(branch_name); refname = branch_get_upstream(branch, NULL); - if (!refname) - continue; - fill_remote_ref_details(atom, refname, branch, &v->s); + if (refname) + fill_remote_ref_details(atom, refname, branch, &v->s); continue; } else if (starts_with(name, "push")) { const char *branch_name; --- a/strbuf.c +++ b/strbuf.c @@ -123,15 +123,18 @@ struct strbuf **strbuf_split_buf(const char *str, size_t slen, while (slen) { int len = slen; + int copylen = len; const char *end = NULL; if (max <= 0 || nr + 1 < max) { end = memchr(str, terminator, slen); - if (end) + if (end) { len = end - str + 1; + copylen = len - !!omit_term; + } } t = xmalloc(sizeof(struct strbuf)); - strbuf_init(t, len); - strbuf_add(t, str, len - !!end * !!omit_term); + strbuf_init(t, copylen); + strbuf_add(t, str, copylen); ALLOC_GROW(ret, nr + 2, alloc); ret[nr++] = t; str += len; -- 2.7.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html