Re: [PATCH] note acl

On 23/05/2013 5:26 a.m., Tsantilas Christos wrote:
> On 05/21/2013 09:14 AM, Amos Jeffries wrote:
>> On 17/05/2013 4:41 a.m., Tsantilas Christos wrote:
>>> 2) note acl ( trunk-note-ACL-v5.patch )
>>> Syntax:
>>> acl aclname note name [value ...]
>>>
>>> Without values, matches any annotation with a given name. With value(s),
>>> matches any annotation with a given name that also has one of the given
>>> values. Annotation sources include note and adaptation_meta directives
>>> as well as helper and eCAP responses.
>> audit results for the second patch:
>>
>> * please do not cut-n-paste the (partially wrong) copyright from other
>> files into new ones.
>> NOTE: a default Squid copyright blurb is planned to be automatically
>> pre-pended somehow. So for now only add one if you want one different
>> from the main Squid COPYRIGHT file. To mark the copyright as yours for
>> this file under the default, add your name in an AUTHOR: line instead
>> for now.
> I removed any copyright comment from these files.

You would be leaving three apparently useless lines for any particular
reason?

+/*
+ */
+

>
>> * please also add a "note_regex" ACL which takes regex patterns as the
>> value (and/or key) details.
>> NP: that will allow removing "Names and values are compared using a
>> string equality test." statement in cf.data.pre
> I prefer to avoid it for now. We spent a lot of time for this project,
> so even the few hours required to add this feature looks long time.
> It is easy for someone to add it if needed.

Fine. But I'm expecting we will not be long before somebody requests it.

One last nit:
   The if-condition in ACLNoteData::match() is wrapping strangely onto
two lines where it would best be on one, particularly in light of the
long adaptation if-condition later.

Those can be fixed when applying to trunk. +1 from me, and thank you
for doing this.

Amos
Received on Fri May 24 2013 - 14:42:16 MDT