Re: test -e patch

[adrian15 <adrian15@raulete.net>]

> adrian15 <adrian15@raulete.net> 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 ?


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

Or have you coded it yourself too?

>> > How the hell should I treat grub errors. Maybe the test_file_exists has
>> > to return a static grub_err_t and then from grub_cmd_test I should call
>> > it like this: return (test_file_exists (args[0])) so that the error
>> > propagates ?
> 
> Right.  Otherwise the error will be lost.

Ok. I will try that every function propagates errors.

> 
>> > What are the error polices ?
> 
> Returning the err_t that grub_error returns when possible.
Ok.
>> > One question. Should we put the test-e function into a separate file or
>> > not ?
> 
> 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.

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

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

test_filename_exists
or
filename

?

>> >  {
>> > +
> 
> You accidently introduced a whiteline.

No whitelines after an initial {. I write down it.

>> > +  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.


adrian15