You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ruediger Pluem <rp...@apache.org> on 2015/08/21 16:25:48 UTC
Re: svn commit: r1697002 -
/httpd/httpd/trunk/modules/filters/mod_substitute.c
Wasn't it intended for trunk to change the default behavior?
Regards
Rüdiger
On 08/21/2015 04:19 PM, ylavic@apache.org wrote:
> Author: ylavic
> Date: Fri Aug 21 14:19:17 2015
> New Revision: 1697002
>
> URL: http://svn.apache.org/r1697002
> Log:
> mod_substitute: follow up r1684900.
> Really inherit before when asked to (inverse if/else contents).
>
> Modified:
> httpd/httpd/trunk/modules/filters/mod_substitute.c
>
> Modified: httpd/httpd/trunk/modules/filters/mod_substitute.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/filters/mod_substitute.c?rev=1697002&r1=1697001&r2=1697002&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/filters/mod_substitute.c (original)
> +++ httpd/httpd/trunk/modules/filters/mod_substitute.c Fri Aug 21 14:19:17 2015
> @@ -97,13 +97,13 @@ static void *merge_substitute_dcfg(apr_p
> * 'off' to follow the corrected/expected behavior, without violating POLS.
> */
> if (a->inherit_before == 1) {
> - a->patterns = apr_array_append(p, base->patterns,
> - over->patterns);
> - }
> - else {
> a->patterns = apr_array_append(p, over->patterns,
> base->patterns);
> }
> + else {
> + a->patterns = apr_array_append(p, base->patterns,
> + over->patterns);
> + }
> a->max_line_length = over->max_line_length_set ?
> over->max_line_length : base->max_line_length;
> a->max_line_length_set = over->max_line_length_set
>
>
>
Re: svn commit: r1697002 - /httpd/httpd/trunk/modules/filters/mod_substitute.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Aug 21, 2015 at 5:46 PM, William A Rowe Jr <wr...@rowe-clan.net> wrote:
> On Fri, Aug 21, 2015 at 10:32 AM, William A Rowe Jr <wr...@rowe-clan.net>
> wrote:
>>
>>
>> It's a toggle, please leave it alone. The default state will be one case
>> in
>> 2.4.x, and a different case in 2.5.x and later, but a user who puts that
>> directive into their config will observe the same behavior when they run
>> their config under 2.4 or later under 2.6.
>
>
> (But correcting the comments and the docs in the respective code branches as
> applicable would be terrific, so users and the rest of us aren't confused
> again when this is reviewed again in the future.)
Done in r1697013 and r1697015.
Backports proposals to 2.4.x and 2.2.x updated to v5 too.
Thanks Ruediger and Bill for the review.
Re: svn commit: r1697002 - /httpd/httpd/trunk/modules/filters/mod_substitute.c
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Fri, Aug 21, 2015 at 10:32 AM, William A Rowe Jr <wr...@rowe-clan.net>
wrote:
>
> It's a toggle, please leave it alone. The default state will be one case
> in
> 2.4.x, and a different case in 2.5.x and later, but a user who puts that
> directive into their config will observe the same behavior when they run
> their config under 2.4 or later under 2.6.
>
(But correcting the comments and the docs in the respective code branches
as applicable would be terrific, so users and the rest of us aren't
confused again when this is reviewed again in the future.)
Re: svn commit: r1697002 - /httpd/httpd/trunk/modules/filters/mod_substitute.c
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Fri, Aug 21, 2015 at 10:05 AM, Yann Ylavic <yl...@gmail.com> wrote:
> On Fri, Aug 21, 2015 at 4:36 PM, Ruediger Pluem <rp...@apache.org> wrote:
> >
> >
> > On 08/21/2015 04:31 PM, Yann Ylavic wrote:
> >> On Fri, Aug 21, 2015 at 4:29 PM, Yann Ylavic <yl...@gmail.com>
> wrote:
> >>> On Fri, Aug 21, 2015 at 4:25 PM, Ruediger Pluem <rp...@apache.org>
> wrote:
> >>>> Wasn't it intended for trunk to change the default behavior?
> >>>
> >>> Yes it was, still when inherit_before is set we need to do that.
> >>
> >> I meant, we need to do "apr_array_append(p, over->patterns,
> >> base->patterns)" and not the inverse.
> >>
> >
> > But apr_array_append(p, over->patterns, base->patterns) means that we do
> NOT inherit the stuff from the base
> > configuration before the configuration of our section. So the code
> before your recent patch was correct.
>
> Hm, you are right, I was misleaded by the comment in trunk:
> /* SubstituteInheritBefore was the default behavior until 2.5.x, ... */
> since SubstituteInheritBefore was *not* the default behavior until 2.5.x.
>
> To recap, apr_array_append(p, over->patterns, base->patterns) is the
> legacy but it is obviously not the inherited base which is merged
> before...
>
> So to keep the existing/prior-to-r1697002 logic in the code (which was
> good), I propose to revert r1697002 and rename SubstituteInheritBefore
> to SubstituteInheritAfter, WDYT?
>
It's a toggle, please leave it alone. The default state will be one case in
2.4.x, and a different case in 2.5.x and later, but a user who puts that
directive into their config will observe the same behavior when they run
their config under 2.4 or later under 2.6.
Re: svn commit: r1697002 - /httpd/httpd/trunk/modules/filters/mod_substitute.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Aug 21, 2015 at 4:36 PM, Ruediger Pluem <rp...@apache.org> wrote:
>
>
> On 08/21/2015 04:31 PM, Yann Ylavic wrote:
>> On Fri, Aug 21, 2015 at 4:29 PM, Yann Ylavic <yl...@gmail.com> wrote:
>>> On Fri, Aug 21, 2015 at 4:25 PM, Ruediger Pluem <rp...@apache.org> wrote:
>>>> Wasn't it intended for trunk to change the default behavior?
>>>
>>> Yes it was, still when inherit_before is set we need to do that.
>>
>> I meant, we need to do "apr_array_append(p, over->patterns,
>> base->patterns)" and not the inverse.
>>
>
> But apr_array_append(p, over->patterns, base->patterns) means that we do NOT inherit the stuff from the base
> configuration before the configuration of our section. So the code before your recent patch was correct.
Hm, you are right, I was misleaded by the comment in trunk:
/* SubstituteInheritBefore was the default behavior until 2.5.x, ... */
since SubstituteInheritBefore was *not* the default behavior until 2.5.x.
To recap, apr_array_append(p, over->patterns, base->patterns) is the
legacy but it is obviously not the inherited base which is merged
before...
So to keep the existing/prior-to-r1697002 logic in the code (which was
good), I propose to revert r1697002 and rename SubstituteInheritBefore
to SubstituteInheritAfter, WDYT?
Re: svn commit: r1697002 -
/httpd/httpd/trunk/modules/filters/mod_substitute.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 08/21/2015 04:31 PM, Yann Ylavic wrote:
> On Fri, Aug 21, 2015 at 4:29 PM, Yann Ylavic <yl...@gmail.com> wrote:
>> On Fri, Aug 21, 2015 at 4:25 PM, Ruediger Pluem <rp...@apache.org> wrote:
>>> Wasn't it intended for trunk to change the default behavior?
>>
>> Yes it was, still when inherit_before is set we need to do that.
>
> I meant, we need to do "apr_array_append(p, over->patterns,
> base->patterns)" and not the inverse.
>
But apr_array_append(p, over->patterns, base->patterns) means that we do NOT inherit the stuff from the base
configuration before the configuration of our section. So the code before your recent patch was correct.
Regards
Rüdiger
Re: svn commit: r1697002 - /httpd/httpd/trunk/modules/filters/mod_substitute.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Aug 21, 2015 at 4:29 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Fri, Aug 21, 2015 at 4:25 PM, Ruediger Pluem <rp...@apache.org> wrote:
>> Wasn't it intended for trunk to change the default behavior?
>
> Yes it was, still when inherit_before is set we need to do that.
I meant, we need to do "apr_array_append(p, over->patterns,
base->patterns)" and not the inverse.
Re: svn commit: r1697002 - /httpd/httpd/trunk/modules/filters/mod_substitute.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Aug 21, 2015 at 4:25 PM, Ruediger Pluem <rp...@apache.org> wrote:
> Wasn't it intended for trunk to change the default behavior?
Yes it was, still when inherit_before is set we need to do that.
The code in trunk is:
if (a->inherit_before == 1) {
whereas 2.4 and 2.2 (would) have:
if (a->inherit_before) { <= can be 1 but also -1
Regards,
Yann.