Re: [PATCH v4 05/12] ref-filter: introduce parsing functions for each valid atom

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

 



On Sun, Feb 7, 2016 at 12:03 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Sat, Feb 6, 2016 at 10:15 AM, Karthik Nayak <karthik.188@xxxxxxxxx> wrote:
>> On Sun, Jan 31, 2016 at 11:12 PM, Karthik Nayak <karthik.188@xxxxxxxxx> wrote:
>>> @@ -138,10 +140,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;
>>>         }
>>
>> Also having a look at this, this breaks the previous error checking we
>> had at parse_ref_filter_atom().
>> e.g: git for-each-ref --format="%(refnameboo)" would not throw an error.
>>
>> I think the code needs to be changed to:
>>
>> -               if ((!arg || len == arg - sp) &&
>> +               if ((arg || len == ep - sp) &&
>> +                   (!arg || len == arg - sp) &&
>
> For completeness, for people reading the mailing list archive, a
> couple alternate fixes were presented elsewhere[1], with a personal
> bias toward:
>
>     arg = memchr(...);
>     if (!arg)
>         arg = ep;
>     if (len == arg - sp && !memcmp(...))
>         ...
>
> [1]: http://git.661346.n2.nabble.com/PATCH-ref-filter-c-don-t-stomp-on-memory-tp7647432p7647433.html

There is a slight issue with this solution though, as you see 'arg'
gets modified
here, hence 'arg' passed to parser functions will never will null.

We could do something like
if (arg ==ep)
    arg = NULL;
if (arg)
    arg = used_atom[at].name + (arg - atom) + 1;
if (valid_atom[i].parser)
    valid_atom[i].parser(&used_atom[at], arg);

Else we could avoid this assignment and re-assignment by letting 'arg'
hold the value it gets from memcmp(...) and use the solution provided
by me or Ramsay (preferably)

Ramsay's solution being

 ref-filter.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ref-filter.c b/ref-filter.c
index d48e2a3..c98065e 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -260,7 +260,8 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
                 * table.
                 */
                arg = memchr(sp, ':', ep - sp);
-               if ((!arg || len == arg - sp) &&
+               if ((( arg && len == arg - sp)  ||
+                    (!arg && len == ep - sp )) &&
                    !memcmp(valid_atom[i].name, sp, len))
                        break;
        }

-- 
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]