Re: test -e patch

[adrian15]

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


> 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