Re: [PATCH v2 11/11] ref-filter: introduce objectname_atom_parser()
From: Eric Sunshine <hidden>
Date: 2016-06-15 23:07:30
On Wed, Dec 16, 2015 at 10:30 AM, Karthik Nayak [off-list ref] wrote:
quoted hunk ↗ jump to hunk
Introduce objectname_atom_parser() which will parse the '%(objectname)' atom and store information into the 'used_atom' structure based on the modifiers used along with the atom. Helped-by: Ramsay Jones [off-list ref] Signed-off-by: Karthik Nayak <redacted> ---diff --git a/ref-filter.c b/ref-filter.c@@ -43,6 +43,7 @@ static struct used_atom { enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option; unsigned int no_lines; } contents; + enum { O_FULL, O_SHORT } objectname; } u; } *used_atom;@@ -124,6 +125,21 @@ static void contents_atom_parser(struct used_atom *atom) +static void objectname_atom_parser(struct used_atom *atom) +{ + const char * buf; + + if (match_atom_name(atom->str, "objectname", &buf)) + atom->u.objectname = O_FULL; + if (!buf) + return;
Let me make sure that I understand this correctly.
make_atom_name("objectname") will return true only for "objectname" or
"objectname:", and will return false for anything else, such as
"objectnamely" or "schmorf". Furthermore, the only way
objectname_atom_parser() can be called is when %(objectname) or
%(objectname:...) is seen, thus match_atom_name() *must* return true
here, which means the above conditional is misleading, suggesting that
it could somehow return false.
And, if match_atom_name() did return false here, then that indicates a
programming error: objectname_atom_parser() somehow got called for
something other than %(objectname) or %(objectname:...). This implies
that the code should instead be structured like this:
if (!match_atom_name(..., "objectname", &buf)
die("BUG: parsing non-'objectname'")
if (!buf)
atom->u.objectname = O_FULL;
else if (!strcmp(buf, "short"))
atom->u.objectname = O_SHORT;
else
die(_("unrecognized %%(objectname) argument: %s"), buf);
However, this can be simplified further by recognizing that, following
this patch series, match_atom_name() is *only* called by these new
parse functions[1], which means that, as a convenience,
match_atom_name() itself could become a void rather than boolean
function and die() if the expected atom name is not found. Thus, the
code would become:
match_atom_name(...);
if (!buf)
...
else if (!strcmp(...))
...
...
By the way, the above commentary isn't specific to this patch and
%(objectname), but is in fact also relevant for all of the preceding
patches which introduce parse functions calling match_atom_name().
More below...
[1]: ...assuming you replace the unnecessary match_atom_name() in
populate_value() with starts_with() as suggested in my patch 7/11
review addendum[2].
[2]: http://article.gmane.org/gmane.comp.version-control.git/282700
quoted hunk ↗ jump to hunk
+ + if (!strcmp(buf, "short")) + atom->u.objectname = O_SHORT; + else + die(_("unrecognized %%(objectname) argument: %s"), buf); +} +@@ -461,15 +477,16 @@ static void *get_obj(const unsigned char *sha1, struct object **obj, unsigned lo static int grab_objectname(const char *name, const unsigned char *sha1, - struct atom_value *v) + struct atom_value *v, struct used_atom *atom) { - if (!strcmp(name, "objectname")) { - v->s = xstrdup(sha1_to_hex(sha1)); - return 1; - } - if (!strcmp(name, "objectname:short")) { - v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV)); - return 1; + if (starts_with(name, "objectname")) { + if (atom->u.objectname == O_SHORT) { + v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV)); + return 1; + } else if (atom->u.objectname == O_FULL) { + v->s = xstrdup(sha1_to_hex(sha1)); + return 1; + }
Since 'objectname' can only ever be O_SHORT or O_FULL wouldn't it be a
programming error if it ever falls through to this point after the
closing brace? Perhaps a die("BUG:...") would be appropriate?
quoted hunk ↗ jump to hunk
} return 0; }@@ -493,7 +510,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); + grab_objectname(name, obj->sha1, v, &used_atom[i]);
This patch hunk is somehow corrupt and doesn't apply. Was there some hand-editing involved, or were some earlier patches regenerated after this patch was made or something?
quoted hunk ↗ jump to hunk
} }@@ -999,7 +1016,7 @@ static void populate_value(struct ref_array_item *ref) v->s = xstrdup(buf + 1); } continue; - } else if (!deref && grab_objectname(name, ref->objectname, v)) { + } else if (!deref && grab_objectname(name, ref->objectname, v, atom)) { continue; } else if (!strcmp(name, "HEAD")) { const char *head; --2.6.4