Re: test -e patch

[Marco Gerards]

adrian15 <address@hidden> writes:

>> adrian15 <address@hidden> writes:
>>
>>> > Attached you will find the patch adding test -e support for grub2.
>>> >
>>> > This is my first patch. I have compiled it without no errors.
>>
>> Urgh... I thought/hoped I told you I had a test.c rewrite sitting on
>> my harddisk?  Or did I tell Robert to poke me until next weekend so I
>> will work on it?  It includes everything you expect from test.c,
>> expect the cleanup and testing.  ;-)
>
> Do you mean you also have the '-e' option ?

I mean I have everything that is possible.  That includes `-e', but
also all other features.

Something noone looked at is "expr".

>> Please have a look at the wiki.  It has quite some information about
>> GRUB 2.
> Whenever possible I'll download some info from the wiki.
>
>>> > Should I write "Test if a file exists" instead of "test if a file
>>> > exists" or "FILE exists"?
>>
>> FILE
>
> FILE
> or
> FILE exists ?

FILE exists

> Or have you coded it yourself too?

Hm?

[...]

>> No, the problem is that the design of test.c (which is just a
>> placeholder) is wrong.  It needs a proper parser for the arguments and
>> a way to deal with this...
>
> Ok. We will wait for your code.

:-)

Sorry for the confusion :-/

>>> > The question is if the user will see the -e, -f or other options when
>>> > querying the test command help or not ?
>>
>> They should.  But I am not sure if the final version will support
>> this.  Especially because of the nested syntax of the test arguments.
>
> Do you mean the -e options support
> or
> do you mean the -e options showing at help test support ?

Well, the version for GNU/Linux doesn't show help text.  Perhaps it
cannot be implemented using the argument parser in a clean way.  I do
not remember.

>>> > +static void
>>> > +test_file_exists (const char *key)
>>
>> Why not filename?
>
> test_filename_exists
> or
> filename

I mean instead of key.

> ?
>
>>> >  {
>>> > +
>>
>> You accidently introduced a whiteline.
>
> No whitelines after an initial {. I write down it.

Well, usually adding whitespaces around code you didn't change is
wrong or dirty.

>>> > +  if (state[0].set)
>>> > +    test_file_exists (args[0]);
>>> > +  else
>>> > +  {
>>
>> This means that this check is run for any other expression.  This is
>> quite error sensitive.
>
> In my code the only implemented option is '-e'. When there will be more
> I could add more nested if with the other options, or maybe better we
> will enjoy your improved code.

Sure.  Just poke me a lot during the weekend on IRC ;-)

--
Marco