Re[2]: [PATCH] ext.c, regex_command ()

[Pavel Tsekov <ptsekov gmx net>]

Hello, Pavel! :)

Wednesday, August 21, 2002, 6:55:51 PM, you wrote:

PR> Hello, Pavel!

>> This patch makes the handling of the 'type/' directive much better. It
>> also improve the quality of the code.

PR> I appreciate your goals, but your patch is too hard for me to understand.

The original code was too hard to read :) Ok I'll answer you questions
below and I will try to explain what's wrong.

PR> I don't want to apply your patch without understanding it, especially when
PR> it adds a goto inside a huge function.  Maybe you could split it into
PR> smaller, more obvious parts?

Let's talk about it first. Then I may split though I don't see how :(

PR> By the way, if I hacked this function, I would first try to split
PR> processing of type/ into a separate function.  regex_command() is too
PR> large.  It even has two different variables "p".

Yes I though about this also but thought it wold be too intrusive. So
I modified the code so its much more readable and contains less nasty
constructs.

PR> Ok, I've just separated processing of 'type/' into regex_check_type().  
PR> Sorry that you would have to adjust your patch, but at least it will have
PR> some chances to be applied now.

You did it! :)

>>  o It checks if the data in 'content_string' is valid and doesn't
>>    preform any further checks, whether it matches a regex.

PR> I undestrand that content_string is empty if it could not be read.  It is
PR> properly terminated by the current code.  Please limit your changes for
PR> now to those fixing bugs and _significantly_ simplifying code.

Correct! But it is not necessary to process 'content_string' if there is
no data. In the current code nobody cares if it contains data or
not. Look the code after the 'match_file_output' label. Read
below for more info..

>>  o There was such check in the original, code but it wasn't handled
>>    properly, so that it wouldn't be used at all.

PR> Sorry, I cannot find that place, so please send a separate patch.

Right after the label there is a check 'hasread'. Now that variable is
is a real PITA.. It is used throughout the 'type/' processing code for
various reasons making it hard to understand whats going on - I've removed
that one btw.

Now the real  problem was the following code:

           if (islocal || file_supports_stdin) {
               char *pp;
>>>>>          int hasread = use_file_to_check_type; // This is always 1

               if (asked_file || !use_file_to_check_type) // Jump to  'file' output parsing code
                        goto match_file_output;

So lets say you're missing 'file' command. No problem - the original
code would try to check 'content_string' each time a 'type/' is found
in mc.ext though it is filled with garbage.

>>  o Almost all of the code is guarded by the 'asked_file' variable.

PR> I'd rather see the code restructured into smaller functions, than 
PR> "guarded".  Too many levels of braces are hard to read.

"Guarded" in therms of reading the output of file and parsing it only
once. Yes this definitely could be a new function.

I know the code is too long but look above - on different projects
'too intrusive' is spelled different :)

>> 1. Why this piece of code doesn't use the subshell ?

PR> Why should it?  Especially when we are feeding data from a remote server 
PR> into "file", bad things can happen if "file" dies.

I don't know what kind of things you refer but I agree with you though
:) And I'm sure you have a reason. Still it could be good to have a
single interface for executing programs. Just a thought - no need to
have a long discussion on that - I don't know all the MC code :)

>> 2. If it's going to live like that for the time being is it acceptable
>> at least to redirect the error output to the standard output so if
>> 'file' is missing the user doesn't get its prompt messed up.

PR> I don't think it should be redirected to stdout - it should be ignored of 
PR> put to a separate file and then displayed.  ext.c is a single caller of 
PR> mc_doublepopen() now, so it should be easy.

Ok I chose the wrong redirection  :)  I was thinking /dev/null but I
dont know if all platforms have /dev/null. Well we better ignore it
because this way we have to additional code to check that file and
parse the output - I think it is not worth. But it's not good to have
it in MC prompt.

Thanks! :)

Pavel Tsekov

P.S. have you applied my patch - it will be easier to understand it
that way. The layout of the code is almost the same - I tried
to keep it as close to original while removing the unnecessary parts
and fixing the suspicious ones.