Re: [PATCH 17/28] attr: expose validity check for attribute names

From: Stefan Beller <sbeller@google.com>
Date: Oct 11, 2016, 18:28 UTC
Message-ID: <CAGZ79kbqikTtUGxXFB3W9k1nL6ST2iwRRi5zEuL9BOpHxZWc8A@mail.gmail.com>
In-Reply-To: <20161011172805.GB9085@google.com>

On Tue, Oct 11, 2016 at 10:28 AM, Brandon Williams <bmwill@google.com> wrote:

Show 27 quoted lines

> On 10/10, Stefan Beller wrote:
>> From: Junio C Hamano <gitster@pobox.com>
>> -static int invalid_attr_name(const char *name, int namelen)
>> +int attr_name_valid(const char *name, size_t namelen)
>>  {
>>       /*
>>        * Attribute name cannot begin with '-' and must consist of
>>        * characters from [-A-Za-z0-9_.].
>>        */
>>       if (namelen <= 0 || *name == '-')
>> -             return -1;
>> +             return 0;
>>       while (namelen--) {
>>               char ch = *name++;
>>               if (! (ch == '-' || ch == '.' || ch == '_' ||
>>                      ('0' <= ch && ch <= '9') ||
>>                      ('a' <= ch && ch <= 'z') ||
>>                      ('A' <= ch && ch <= 'Z')) )
>> -                     return -1;
>> +                     return 0;
>>       }
>> -     return 0;
>> +     return -1;
>> +}
>
> Whats the reason behind returning -1 for a valid attr name vs 1?
>

Usually we prefer negative values for errors, whereas 0,1,2... is used for actual return values.

If you're not really interested in the value, but rather "does it work at all?" you can use it via

    if (function() < 0)
        die_errno("function has error");

Otherwise you'd do a

    int value = function();
    if (value < 0)
        die(...);

    switch(value) {
    default:
    case 0:
        doZeroThing(); break;
    case 1:
        doOtherThing();
    }

At first I thought the two different return path for -1 are different error classes (one being just a minor error compared to the other), but they are not, so having the same code there makes sense.

So I think we could change it to +1 in this instance, as a non zero value would just indicate the attr name is not valid, but not necessarily an error.