[pacman-dev] [PATCH 07/11] makepkg: less code repetition when sanitizing variable contents

[Allan McRae]

On 17/06/10 23:53, Andres P wrote:
> On Thu, Jun 17, 2010 at 8:52 AM, Allan McRae<allan at archlinux.org>  wrote:
>> On 17/06/10 22:44, Andres P wrote:
>>>
>>> During check_sanity, use regex and abstract the series of variable checks
>>> into
>>> a list.
>>>
>>> Signed-off-by: Andres P<aepd87 at gmail.com>
>>> ---
>>>   scripts/makepkg.sh.in |   70
>>> +++++++++++++++++++-----------------------------
>>>   1 files changed, 28 insertions(+), 42 deletions(-)
>>>
>>> diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
>>> index 23e3b36..991ad0f 100644
>>> --- a/scripts/makepkg.sh.in
>>> +++ b/scripts/makepkg.sh.in
>>> @@ -1161,6 +1161,19 @@ install_package() {
>>>         fi
>>>   }
>>>
>>> +var_lint() {
>>> +       local pattern="$1"
>>> +       local directive="$2"
>>> +       shift 2
>>> +
>>> +       local i
>>> +       for i; do
>>> +               [[ $i =~ $pattern ]] || continue
>>> +               error "$(gettext "'%s' is an invalid value for %s")" "$i"
>>> "$directive"
>>> +               return 1
>>> +       done
>>> +}
>>
>> I am against this as the error messages are no longer informative.
>>
>> Allan
>>
>
> Well, the error message would be the least of worries now that it's in
> one place instead of>= 7.
>
> What type of error message would be informative?
>
> "variable %s may not match regex %s"
>
> And if makepkg has code repetition because of documentation, then the
> man page out to be fixed? Not that the error message is less
> descriptive as it is anyhow.

The new error message is much less descriptive;  "pkgname is not allowed 
to start with a hyphen" is very clear, while "'-foo' is an invalid value 
for pkgname" does not tell me what is wrong.

Using the regex in the error message is just going to confuse the hell 
out of people too.  Reading an error message should not require you know 
regexes.

And your quest to reduce code duplication can go to the point of being 
nonsensical.  e.g. in this patch:

 > +	
 > +	local i
 > +	for i in 'pkgver' 'pkgrel'; do
 > +		var_lint '-' $i "${!i}" || return
 > +	done
 >

That would be two lines without the loop...