[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: test -e patch

From: Marco Gerards
Subject: Re: test -e patch
Date: Tue, 05 Jun 2007 20:13:07 +0200
User-agent: Gnus/5.110006 (No Gnus v0.6) Emacs/21.4 (gnu/linux)

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"?
> or
> FILE exists ?

FILE exists

> Or have you coded it yourself too?



>> 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 ;-)


reply via email to

[Prev in Thread] Current Thread [Next in Thread]