Re: [PATCΗ] Quoted values in squid.conf

On 05/27/2013 07:26 AM, Amos Jeffries wrote:
> On 27/05/2013 2:07 a.m., Tsantilas Christos wrote:
>> On 05/26/2013 06:30 AM, Amos Jeffries wrote:
>>> On 24/05/2013 7:38 a.m., Tsantilas Christos wrote:
>
> <snip strings>
>
>>>
>>> Secondly, multiple features in one patch:
>>>
>>> The MacroUser system you are adding here is combining some, but not
>>> enough, of the next step in the libformat project. Thank you for doing
>>> that, but I do not think it appropriate to merge it into this whitespace
>>> and quote handling patch. It was planned to be done as a followup once
>>> the parser was updated in a way such as what this patch is doing.
>> The MacroUser just only check if %macros supported or not, and which
>> macros supported. Also the user of this class may select to not do any
>> check at all, just use it to enable macros in the next parsed token.
>> I do not think that it is related to libformat ...
>
> Whether macros are supported or not is irrelevant to the string being
> de-coded into a single token or split on whitespace. Which is what the
> rest of this patch is doing.
> The one potential crossover is that %"<h is a valid macro/code/token
> which contains a quote. But your update needs to ensure that quoting
> always is at the beginnning or end of a word when it toggles anyway.

ok.
Looks that we should recognize a " as quote after a whitespace (start)
or before a whitespace(end), or find an other way to represent %" fmt
codes. Now this is does not work well...

>
> The only code you have pulling in macros is code which is or will be
> shortly using libformat to parse those macros. So I'm seeing all this
> macro code (as currently implemented) as part of the libformat project,
> possibly the very next step. But still a different scope to the string
> quoting patch. That is all.

What are you suggesting for MacroUser class? Just remove it and don't do
any check for '%' fmt codes? Replace it with a simple on/off which
enables/disable '%' chars in tokens? eg:

 ConfigParser::EnableMacro();
 token = ConfigParser::NextToken();
 ConfigParser::DisableMacro();

The MacroUser class used in two cases, in external_acl.cc and in
format/Format.cc files :
 - The first case (external acl) it just implements a
MacroUser::supportedMacro method which return always true.
 - The second case implements a MacroUser::supportedMacro method which
check for valid token.

In the second case the check is not actually needed because in later
step we check for valid %fmt codes. If this is your problem we can just
remove it, and return always true.

The current MacroUser class is very simple. For now it can be
considered, just as a mark for the code someone should touch to
implement checks for valid %fmt codes.
The MacroUser class can be replaced with other mechanism later if you want.
The question for me, is if we need or not such checks in the parsing
code (and maybe what kind of checks).

>
>>
>>> Also by naming the class MacroUser you are adding a fourth term
>>> (macro)
>>> to describe these %things, we already have %-tokens, %-tags, %-format,
>>> and %-codes in use today. I think we should start to redux those terms
>>> usage rather than expanding with another variation on the theme.
>> The naming is not something important, we can always rename this class,
>> before commit the patch or later when we find a better name ...
>>
>>> ==> Please separate the MacroUser functionality out into a followup
>>> patch.
>>>
>>>
>>> Amos
>>>
>>>
>
>
Received on Mon May 27 2013 - 10:41:03 MDT