Re: [PATCH v16 05/14] ref-filter: introduce match_atom_name()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]