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

[Pavel Roskin <proski gnu org>]

Hello, Pavel!

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

I appreciate your goals, but your patch is too hard for me to understand.  
I don't want to apply your patch without understanding it, especially when
it adds a goto inside a huge function.  Maybe you could split it into
smaller, more obvious parts?

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

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

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

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

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

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

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

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

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

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

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

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

P.S. I just realized that spliting regex_check_type() breaks caching of
the "file" output, and I don't have time to fix it today, so your help
will be appreciated.

-- 
Regards,
Pavel Roskin