On Mon, Sep 7, 2015 at 1:22 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Sat, Sep 5, 2015 at 2:52 PM, Karthik Nayak <karthik.188@xxxxxxxxx> wrote:
>> 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.
>>
>> Signed-off-by: Karthik Nayak <karthik.188@xxxxxxxxx>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> index a993216..e99c342 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> +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 */
>> + return 1;
>
> If this is invoked as match_atom_name("colors", "color", ...), then it
> will return true (matched, but no value), which is not correct at all;
> "colors" is not a match for atom %(color). Maybe you meant?
>
> if (!body[0] || (body[0] == ':' && !body[1])) {
>
> But, that's getting ugly and complicated, and would be bettered
> handled by reordering the logic of this function for dealing with the
> various valid and invalid cases. However...
>
Well as you infered the logic is to check so that there is something
existing after the ':'. About why I didn't do something like this
"(body[0] == ':' && !body[1])" is because ref-filter already
checks for invalid atom names.
So before even match_atom_name() is called, the check is done in
parse_ref_filter_atom().
>> + }
>> + if (body[0] != ':')
>> + return 0; /* "atom_namefoo" is not "atom_name" or "atom_name:..." */
>> + *val = body + 1; /* "atomname:val" */
>> + return 1;
>> +}
>
> It's not clear why this function exists in the first place. It's only
> purpose seems to be to specially recognize instances of atoms which
> should have a ":" but lack it, so that the caller can then report an
> error.
>
> But why is this better than the more generic solution of merely
> reporting an error for *any* unrecognized atom? How does it help to
> single out these special cases?
>
> There is a further inconsistency in that you are only specially
> recognizing %(color) and %(align) lacking ":", but not
> %(content:lines) lacking "=" in patch 8/14. Why do %(color:...) and
> %(align:...) get special treatment but %(content:lines=...) does not?
> Such inconsistency again argues in favor of a generic "unrecognized
> atom" detection and reporting over special case handling.
>
This is from http://article.gmane.org/gmane.comp.version-control.git/277099
Junio suggested to have this to check for ":" rather than rely on
skip_prefix() and check manually after that.
Agreed, a more generic solution would be better, and If I remember I said,
I'll work on that after this entire series.
About contents:lines, we declare contents:lines itself as an atom, this was to
keep it similar to how the other contents atoms are declared, so the testing
for this again is already done by parse_ref_filter_atom().
--
Regards,
Karthik Nayak
--
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