Re: [PATCH v17 05/14] ref-filter: introduce match_atom_name()
From: Matthieu Moy <hidden>
Date: 2016-06-15 23:06:30
Karthik Nayak [off-list ref] writes:
quoted hunk
Introduce match_atom_name() which helps in checking if a particular atom is the atom we're looking for and if it has a value attached to it or not. Use it instead of starts_with() for checking the value of %(color:...) atom. Write a test for the same. Mentored-by: Christian Couder [off-list ref] Mentored-by: Matthieu Moy [off-list ref] Thanks-to: Junio C Hamano [off-list ref] Signed-off-by: Karthik Nayak <redacted> --- ref-filter.c | 23 +++++++++++++++++++++-- t/t6302-for-each-ref-filter.sh | 4 ++++ 2 files changed, 25 insertions(+), 2 deletions(-)diff --git a/ref-filter.c b/ref-filter.c index a993216..70d36fe 100644 --- a/ref-filter.c +++ b/ref-filter.c@@ -189,6 +189,22 @@ static void pop_stack_element(struct ref_formatting_stack **stack) *stack = prev; } +static int match_atom_name(const char *name, const char *atom_name, const char **val) +{ + const char *body; + + if (!skip_prefix(name, atom_name, &body)) + return 0; /* doesn't even begin with "atom_name" */ + if (!body[0] || !body[1]) { + *val = NULL; /* %(atom_name) and no customization */
The logic is still hard to follow. If I read correctly, this function accepts "%(colorX)" the same ways as "%(color)" here. I first thought it was a bug, but it doesn't seem to be since %(colorX) would have been rejected earlier. It would be a bug if colorX was actually a valid atom name though: you would be returning 1 for match_atom_name(name, "color") when name=="colorX". So, I would say this "we can accept one extra character because some earlier code rejected it before" is too hard to follow for reviwers and too fragile. OTOH, you are now accepting %(atom:) as a synonym to %(atom), and it's not clear whether this is a deliberate decition.
+ return 1; + } + if (body[0] != ':') + return 0; /* "atom_namefoo" is not "atom_name" or "atom_name:..." */ + *val = body + 1; /* "atom_name:val" */ + return 1; +}
Reversing the logic like this would be better IMHO:
static int match_atom_name(const char *name, const char *atom_name, const char **val)
{
const char *body;
if (!skip_prefix(name, atom_name, &body)) {
/* doesn't even begin with "atom_name" */
return 0;
}
if (!body[0]) {
/* "atom_name" and no customization */
*val = NULL;
return 1;
}
if (body[0] != ':')
/* "atom_namefoo" is not "atom_name" or "atom_name:..." */
return 0;
if (!body[1]) {
/* "atom_name:" */
*val = NULL;
return 1;
}
/* "atom_name:... */
*val = body + 1;
return 1;
}
=> each case appears very clearly, and we check body[0] != ':' before
testing !body[1], so %(colorX) is rejected before noticing the '\0'
after the 'X'.
"atom_name:" appears explicitly. If we want to reject it, we know which
code to change.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/