You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by William A Rowe Jr <wr...@rowe-clan.net> on 2015/06/30 02:03:27 UTC
Re: svn commit: r1688331 - /httpd/httpd/trunk/modules/filters/mod_substitute.c
I can't approve this semantic mess.
EITHER it is inherit_before on trunk-2.4-2.2 with a change of default
behavior, or it is inherit_after, again across all branches with a change
of default behavior. The delta should consist of a one line difference,
evaluating inheritance behavior within the merge.
Please express your preference and I will offer several style fixes on
trunk that make this easier to follow, but we are not adding one directive
to trunk and a different one to 2.4 & 2.2 :-/
On Jun 29, 2015 6:44 PM, <yl...@apache.org> wrote:
> Author: ylavic
> Date: Mon Jun 29 23:44:28 2015
> New Revision: 1688331
>
> URL: http://svn.apache.org/r1688331
> Log:
> mod_substitute: follow up to r1687680.
> Fix dir config merger 'over'-write, thanks Bill (again).
>
> 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=1688331&r1=1688330&r2=1688331&view=diff
>
> ==============================================================================
> --- httpd/httpd/trunk/modules/filters/mod_substitute.c (original)
> +++ httpd/httpd/trunk/modules/filters/mod_substitute.c Mon Jun 29 23:44:28
> 2015
> @@ -86,10 +86,9 @@ static void *merge_substitute_dcfg(apr_p
> subst_dir_conf *base = (subst_dir_conf *) basev;
> subst_dir_conf *over = (subst_dir_conf *) overv;
>
> - if (over->inherit_before < 0) {
> - over->inherit_before = (base->inherit_before > 0);
> - }
> - if (over->inherit_before) {
> + a->inherit_before = (over->inherit_before > 0 ||
> (over->inherit_before < 0 &&
> +
> base->inherit_before > 0));
> + if (a->inherit_before) {
> a->patterns = apr_array_append(p, base->patterns,
> over->patterns);
> }
>
>
>
Re: svn commit: r1688331 - /httpd/httpd/trunk/modules/filters/mod_substitute.c
Posted by Yann Ylavic <yl...@gmail.com>.
This is the case, a single SubstituteInheritBefore directive,
defaulting to Off in trunk and On in 2.4 / 2.2 (proposed patches).
On Tue, Jun 30, 2015 at 12:39 PM, Plüm, Rüdiger, Vodafone Group
<ru...@vodafone.com> wrote:
> +1. They can even make their configuration "future proof" today by setting the 2.4 default behaviour explicitly.
>
> Regards
>
> Rüdiger
>
>> -----Original Message-----
>> From: Jim Jagielski [mailto:jim@jaguNET.com]
>> Sent: Dienstag, 30. Juni 2015 12:25
>> To: dev@httpd.apache.org
>> Cc: cvs@httpd.apache.org
>> Subject: Re: svn commit: r1688331 -
>> /httpd/httpd/trunk/modules/filters/mod_substitute.c
>>
>> My pref is that we create one single directive which controls
>> this. With 2.4 the default is the "old" (incorrect) merge and
>> with trunk it is the new (correct) merge. That way those
>> upgrading from 2.4 -> 2.6/3.0 will only need to worry about
>> a directive default change.
RE: svn commit: r1688331 -
/httpd/httpd/trunk/modules/filters/mod_substitute.c
Posted by Plüm,
Rüdiger,
Vodafone Group <ru...@vodafone.com>.
+1. They can even make their configuration "future proof" today by setting the 2.4 default behaviour explicitly.
Regards
Rüdiger
> -----Original Message-----
> From: Jim Jagielski [mailto:jim@jaguNET.com]
> Sent: Dienstag, 30. Juni 2015 12:25
> To: dev@httpd.apache.org
> Cc: cvs@httpd.apache.org
> Subject: Re: svn commit: r1688331 -
> /httpd/httpd/trunk/modules/filters/mod_substitute.c
>
> My pref is that we create one single directive which controls
> this. With 2.4 the default is the "old" (incorrect) merge and
> with trunk it is the new (correct) merge. That way those
> upgrading from 2.4 -> 2.6/3.0 will only need to worry about
> a directive default change.
Re: svn commit: r1688331 - /httpd/httpd/trunk/modules/filters/mod_substitute.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
My pref is that we create one single directive which controls
this. With 2.4 the default is the "old" (incorrect) merge and
with trunk it is the new (correct) merge. That way those
upgrading from 2.4 -> 2.6/3.0 will only need to worry about
a directive default change.
Re: svn commit: r1688331 - /httpd/httpd/trunk/modules/filters/mod_substitute.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Jun 30, 2015 at 3:35 AM, William A Rowe Jr <wr...@rowe-clan.net> wrote:
> On Mon, Jun 29, 2015 at 8:06 PM, Yann Ylavic <yl...@gmail.com> wrote:
>>
>> Maybe defining (naming) inherit_before tristate values would help:
>
>
> Not really...
>
>> + a->inherit_before = (over->inherit_before == INHERIT_ON
>> + || (over->inherit_before == INHERIT_UNSET
>> + && base->inherit_before == INHERIT_ON));
>> if (a->inherit_before) {
>
>
> This logic was convoluted
Agreed.
> and therefore resulted in in the old default
> behavior if the option wasn't explicitly set.
I can't see this though, over->inherit_before == over->inherit_before
== INHERIT_UNSET resulted in a->inherit_before = FALSE in trunk and
TRUE in 2.[24].x.
> See the most recent trunk
> commits for a more legible solution that follows the design pattern used
> throughout other core modules.
That's simpler indeed.
I previously overlooked that the merge were already against the merged
parents, so base->inherit_before will be unset iff it is unused so
far, hence I was wrong about the third level issue...
( I had to play with gdb to figure out though :)
>
> Your logic above fails to preserve the unset state, and fails to when
> consider base->inherit_before is explicitly off.
I can't see this either...
I still think my logic was working too, just like the change you did
regarding max_line_length_set in r1688341 (you don't preserve the
unset state there either).
> This is why it is
> preferred not to invent new wheels when good wheels exist, particularly if
> there will be a square side on the new wheel.
That I can agree with ;)
Btw, I added your proposed changes to the 2.4.x/2.2.x proposals.
Regards,
Yann.
Re: svn commit: r1688331 - /httpd/httpd/trunk/modules/filters/mod_substitute.c
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Mon, Jun 29, 2015 at 8:06 PM, Yann Ylavic <yl...@gmail.com> wrote:
> Maybe defining (naming) inherit_before tristate values would help:
>
Not really...
+ a->inherit_before = (over->inherit_before == INHERIT_ON
> + || (over->inherit_before == INHERIT_UNSET
> + && base->inherit_before == INHERIT_ON));
> if (a->inherit_before) {
>
This logic was convoluted and therefore resulted in in the old default
behavior if the option wasn't explicitly set. See the most recent trunk
commits for a more legible solution that follows the design pattern used
throughout other core modules.
Your logic above fails to preserve the unset state, and fails to when
consider base->inherit_before is explicitly off. This is why it is
preferred not to invent new wheels when good wheels exist, particularly if
there will be a square side on the new wheel.
Bill
Re: svn commit: r1688331 - /httpd/httpd/trunk/modules/filters/mod_substitute.c
Posted by Yann Ylavic <yl...@gmail.com>.
Maybe defining (naming) inherit_before tristate values would help:
Index: modules/filters/mod_substitute.c
===================================================================
--- modules/filters/mod_substitute.c (revision 1688331)
+++ modules/filters/mod_substitute.c (working copy)
@@ -68,6 +68,10 @@ typedef struct {
apr_pool_t *tpool;
} substitute_module_ctx;
+#define INHERIT_UNSET -1
+#define INHERIT_OFF 0
+#define INHERIT_ON 1
+
static void *create_substitute_dcfg(apr_pool_t *p, char *d)
{
subst_dir_conf *dcfg =
@@ -75,7 +79,7 @@ static void *create_substitute_dcfg(apr_pool_t *p,
dcfg->patterns = apr_array_make(p, 10, sizeof(subst_pattern_t));
dcfg->max_line_length = AP_SUBST_MAX_LINE_LENGTH;
- dcfg->inherit_before = -1;
+ dcfg->inherit_before = INHERIT_UNSET;
return dcfg;
}
@@ -86,8 +90,9 @@ static void *merge_substitute_dcfg(apr_pool_t *p,
subst_dir_conf *base = (subst_dir_conf *) basev;
subst_dir_conf *over = (subst_dir_conf *) overv;
- a->inherit_before = (over->inherit_before > 0 ||
(over->inherit_before < 0 &&
-
base->inherit_before > 0));
+ a->inherit_before = (over->inherit_before == INHERIT_ON
+ || (over->inherit_before == INHERIT_UNSET
+ && base->inherit_before == INHERIT_ON));
if (a->inherit_before) {
a->patterns = apr_array_append(p, base->patterns,
over->patterns);
?
Which would be:
+ a->inherit_before = (over->inherit_before == INHERIT_ON
+ || (over->inherit_before == INHERIT_UNSET
+ && base->inherit_before != INHERIT_OFF));
in 2.2 and 2.4.
On Tue, Jun 30, 2015 at 2:50 AM, William A Rowe Jr <wr...@rowe-clan.net> wrote:
> I was literally switching between a dead and live box repairing a corrupted
> boot volume, so you may be right or I might have studied a stale patch.
>
> Will refresh trunk in a few minutes here with suggested changes.
>
> On Jun 29, 2015 7:42 PM, "Yann Ylavic" <yl...@gmail.com> wrote:
>>
>> On Tue, Jun 30, 2015 at 2:03 AM, William A Rowe Jr <wr...@rowe-clan.net>
>> wrote:
>> > I can't approve this semantic mess.
>> >
>> > EITHER it is inherit_before on trunk-2.4-2.2 with a change of default
>> > behavior, or it is inherit_after, again across all branches with a
>> > change of
>> > default behavior. The delta should consist of a one line difference,
>> > evaluating inheritance behavior within the merge.
>>
>> Well, that's the case already, no?
>> With 2.4.x patch applied:
>>
>> --- 2.4.x/modules/filters/mod_substitute.c 2015-06-30
>> 01:52:18.595947091 +0200
>> +++ trunk/modules/filters/mod_substitute.c 2015-06-30
>> 01:41:18.027679427 +0200
>> @@ -87,7 +87,7 @@
>> subst_dir_conf *over = (subst_dir_conf *) overv;
>>
>> a->inherit_before = (over->inherit_before > 0 ||
>> (over->inherit_before < 0 &&
>> -
>> base->inherit_before != 0));
>> +
>> base->inherit_before > 0));
>> if (a->inherit_before) {
>> a->patterns = apr_array_append(p, base->patterns,
>> over->patterns);
>>
>> >
>> > Please express your preference and I will offer several style fixes on
>> > trunk
>> > that make this easier to follow, but we are not adding one directive to
>> > trunk and a different one to 2.4 & 2.2 :-/
>>
>> Same directive in trunk and 2.[24] branches, default only changes, I
>> don't see what you mean.
>> This proposal allows to merge the inherit_before flag itself, that may
>> be confusing / not suitable / overkill (dunno), so feel free to
>> implement simpler/better code (the default merge-base-before-over
>> semantic must be preserved for the branches, though).
Re: svn commit: r1688331 - /httpd/httpd/trunk/modules/filters/mod_substitute.c
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
I was literally switching between a dead and live box repairing a corrupted
boot volume, so you may be right or I might have studied a stale patch.
Will refresh trunk in a few minutes here with suggested changes.
On Jun 29, 2015 7:42 PM, "Yann Ylavic" <yl...@gmail.com> wrote:
> On Tue, Jun 30, 2015 at 2:03 AM, William A Rowe Jr <wr...@rowe-clan.net>
> wrote:
> > I can't approve this semantic mess.
> >
> > EITHER it is inherit_before on trunk-2.4-2.2 with a change of default
> > behavior, or it is inherit_after, again across all branches with a
> change of
> > default behavior. The delta should consist of a one line difference,
> > evaluating inheritance behavior within the merge.
>
> Well, that's the case already, no?
> With 2.4.x patch applied:
>
> --- 2.4.x/modules/filters/mod_substitute.c 2015-06-30
> 01:52:18.595947091 +0200
> +++ trunk/modules/filters/mod_substitute.c 2015-06-30
> 01:41:18.027679427 +0200
> @@ -87,7 +87,7 @@
> subst_dir_conf *over = (subst_dir_conf *) overv;
>
> a->inherit_before = (over->inherit_before > 0 ||
> (over->inherit_before < 0 &&
> -
> base->inherit_before != 0));
> +
> base->inherit_before > 0));
> if (a->inherit_before) {
> a->patterns = apr_array_append(p, base->patterns,
> over->patterns);
>
> >
> > Please express your preference and I will offer several style fixes on
> trunk
> > that make this easier to follow, but we are not adding one directive to
> > trunk and a different one to 2.4 & 2.2 :-/
>
> Same directive in trunk and 2.[24] branches, default only changes, I
> don't see what you mean.
> This proposal allows to merge the inherit_before flag itself, that may
> be confusing / not suitable / overkill (dunno), so feel free to
> implement simpler/better code (the default merge-base-before-over
> semantic must be preserved for the branches, though).
>
Re: svn commit: r1688331 - /httpd/httpd/trunk/modules/filters/mod_substitute.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Jun 30, 2015 at 2:03 AM, William A Rowe Jr <wr...@rowe-clan.net> wrote:
> I can't approve this semantic mess.
>
> EITHER it is inherit_before on trunk-2.4-2.2 with a change of default
> behavior, or it is inherit_after, again across all branches with a change of
> default behavior. The delta should consist of a one line difference,
> evaluating inheritance behavior within the merge.
Well, that's the case already, no?
With 2.4.x patch applied:
--- 2.4.x/modules/filters/mod_substitute.c 2015-06-30
01:52:18.595947091 +0200
+++ trunk/modules/filters/mod_substitute.c 2015-06-30
01:41:18.027679427 +0200
@@ -87,7 +87,7 @@
subst_dir_conf *over = (subst_dir_conf *) overv;
a->inherit_before = (over->inherit_before > 0 ||
(over->inherit_before < 0 &&
-
base->inherit_before != 0));
+
base->inherit_before > 0));
if (a->inherit_before) {
a->patterns = apr_array_append(p, base->patterns,
over->patterns);
>
> Please express your preference and I will offer several style fixes on trunk
> that make this easier to follow, but we are not adding one directive to
> trunk and a different one to 2.4 & 2.2 :-/
Same directive in trunk and 2.[24] branches, default only changes, I
don't see what you mean.
This proposal allows to merge the inherit_before flag itself, that may
be confusing / not suitable / overkill (dunno), so feel free to
implement simpler/better code (the default merge-base-before-over
semantic must be preserved for the branches, though).