Mailing List Archive

Re: svn commit: r1905170 - /httpd/httpd/trunk/modules/dav/main/mod_dav.c
On 11/9/22 2:12 AM, manu@apache.org wrote:
> Author: manu
> Date: Wed Nov 9 01:12:26 2022
> New Revision: 1905170
>
> URL: http://svn.apache.org/viewvc?rev=1905170&view=rev
> Log:
> Turn DavLockDiscovery into a flag
>
> As requested on dev@httpd.apache.org, turn DavLockDiscovery into a Flag.
> Expressions can still be used by enclosing the directive by
> <If "expr">...</If>
>
>
> Modified:
> httpd/httpd/trunk/modules/dav/main/mod_dav.c
>
> Modified: httpd/httpd/trunk/modules/dav/main/mod_dav.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/dav/main/mod_dav.c?rev=1905170&r1=1905169&r2=1905170&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/dav/main/mod_dav.c (original)
> +++ httpd/httpd/trunk/modules/dav/main/mod_dav.c Wed Nov 9 01:12:26 2022
> @@ -83,7 +83,7 @@ typedef struct {
> const char *dir;
> int locktimeout;
> int allow_depthinfinity;
> - ap_expr_info_t *allow_lockdiscovery;
> + int allow_lockdiscovery;
>
> } dav_dir_conf;
>
> @@ -160,6 +160,8 @@ static void *dav_create_dir_config(apr_p
>
> conf = (dav_dir_conf *)apr_pcalloc(p, sizeof(*conf));
>
> + conf->allow_lockdiscovery = DAV_ENABLED_ON;
> +

Better do not set it here, but leave it to 0 aka DAV_ENABLED_UNSET.
This makes it possible to use DAV_INHERIT_VALUE in dav_merge_dir_config
The corresponding code for dav_merge_dir_config is missing in this this patch.

> /* clean up the directory to remove any trailing slash */
> if (dir != NULL) {
> char *d;

Otherwise good first commit. Thanks for taking care.
Would you provide some documentation for the new directive in docs/manual/mod/mod_dav.xml to make it perfect?
Have a look here how to update the html after you adjusted the xml file:

https://httpd.apache.org/docs-project/docsformat.html

Typically the change to the xml file and the updated html files are committed separately e.g. look at

http://svn.apache.org/viewvc?rev=1904805&view=rev
http://svn.apache.org/viewvc?rev=1904806&view=rev


Regards

Rüdiger
Re: svn commit: r1905170 - /httpd/httpd/trunk/modules/dav/main/mod_dav.c [ In reply to ]
On Wed, Nov 09, 2022 at 08:19:47AM +0100, Ruediger Pluem wrote:
> Would you provide some documentation for the new directive

Yes, this is pending.

--
Emmanuel Dreyfus
manu@netbsd.org
Re: svn commit: r1905170 - /httpd/httpd/trunk/modules/dav/main/mod_dav.c [ In reply to ]
On Wed, Nov 09, 2022 at 08:19:47AM +0100, Ruediger Pluem wrote:
> Better do not set it here, but leave it to 0 aka DAV_ENABLED_UNSET.
> This makes it possible to use DAV_INHERIT_VALUE in dav_merge_dir_config
> The corresponding code for dav_merge_dir_config is missing in this this patch.

It was committed before:
newconf->allow_lockdiscovery = DAV_INHERIT_VALUE(parent, child,
allow_lockdiscovery);

The chnage below this seems to be enough to do the job.

allow_lockdiscovery is only checked against DAV_ENABLED_OFF, hence
DAV_ENABLED_UNSET and DAV_ENABLED_ON have the same effect, which is
what is desired for backward compatibility sake.

Index: modules/dav/main/mod_dav.c
===================================================================
--- modules/dav/main/mod_dav.c (revision 1905191)
+++ modules/dav/main/mod_dav.c (working copy)
@@ -160,7 +160,7 @@

conf = (dav_dir_conf *)apr_pcalloc(p, sizeof(*conf));

- conf->allow_lockdiscovery = DAV_ENABLED_ON;
+ conf->allow_lockdiscovery = DAV_ENABLED_UNSET;

/* clean up the directory to remove any trailing slash */
if (dir != NULL) {



--
Emmanuel Dreyfus
manu@netbsd.org
Re: svn commit: r1905170 - /httpd/httpd/trunk/modules/dav/main/mod_dav.c [ In reply to ]
On 11/9/22 10:59 AM, Emmanuel Dreyfus wrote:
> On Wed, Nov 09, 2022 at 08:19:47AM +0100, Ruediger Pluem wrote:
>> Better do not set it here, but leave it to 0 aka DAV_ENABLED_UNSET.
>> This makes it possible to use DAV_INHERIT_VALUE in dav_merge_dir_config
>> The corresponding code for dav_merge_dir_config is missing in this this patch.
>
> It was committed before:
> newconf->allow_lockdiscovery = DAV_INHERIT_VALUE(parent, child,
> allow_lockdiscovery);

Sorry I missed this. Good catch.

>
> The chnage below this seems to be enough to do the job.
>
> allow_lockdiscovery is only checked against DAV_ENABLED_OFF, hence
> DAV_ENABLED_UNSET and DAV_ENABLED_ON have the same effect, which is
> what is desired for backward compatibility sake.
>
> Index: modules/dav/main/mod_dav.c
> ===================================================================
> --- modules/dav/main/mod_dav.c (revision 1905191)
> +++ modules/dav/main/mod_dav.c (working copy)
> @@ -160,7 +160,7 @@
>
> conf = (dav_dir_conf *)apr_pcalloc(p, sizeof(*conf));
>
> - conf->allow_lockdiscovery = DAV_ENABLED_ON;
> + conf->allow_lockdiscovery = DAV_ENABLED_UNSET;
>
> /* clean up the directory to remove any trailing slash */
> if (dir != NULL) {
>
>
>

The above fixes this. It is just a change in style because currently we init conf to all zero's via apr_pcalloc which means all
fields are in an 'UNSET' state automatically. Hence just removing

conf->allow_lockdiscovery = DAV_ENABLED_ON;

would be more in line with the existing code.

Regards

Rüdiger
Re: svn commit: r1905170 - /httpd/httpd/trunk/modules/dav/main/mod_dav.c [ In reply to ]
On Wed, Nov 09, 2022 at 08:19:47AM +0100, Ruediger Pluem wrote:
> Typically the change to the xml file and the updated html files are committed separately e.g. look at

I will let someone review xml changes in r1905230 before committing
the html files.

--
Emmanuel Dreyfus
manu@netbsd.org
Re: svn commit: r1905170 - /httpd/httpd/trunk/modules/dav/main/mod_dav.c [ In reply to ]
On Fri, Nov 11, 2022 at 02:12:09AM +0000, Emmanuel Dreyfus wrote:
> I will let someone review xml changes in r1905230 before committing
> the html files.

I committed the html files.

What is the procedure for pushing changes to the 2.4 branch?

I have the following changes for DAVLockDiscovery:
r1905327
r1905230
r1905229
r1905206
r1905170
r1904662
r1904638

--
Emmanuel Dreyfus
manu@netbsd.org
Re: svn commit: r1905170 - /httpd/httpd/trunk/modules/dav/main/mod_dav.c [ In reply to ]
On 11/16/22 2:46 AM, Emmanuel Dreyfus wrote:
> On Fri, Nov 11, 2022 at 02:12:09AM +0000, Emmanuel Dreyfus wrote:
>> I will let someone review xml changes in r1905230 before committing
>> the html files.
>
> I committed the html files.
>
> What is the procedure for pushing changes to the 2.4 branch?

Trunk where you committed is under CTR (Commit then Review). Hence you can just commit
and people will review the commit and give feedback / comments if they see a need.
In the worst case they will veto the change with a -1, but this only happens rarely.
A -1 always requires a technical justification. Otherwise it is not valid.
Typically the -1 is just used as a starting point for an intense technical discussion
about the patch. This discussion either leads to a revert of the patch or a modification
of the patch that addresses the technical concerns that lead to the -1.

Even though trunk is CTR it is wise to discuss larger changes before committing them.
You can either post them to the dev list or even better, open a PR on Github and sent a
mail about it to the dev list.
The advantage of the PR is that it will do the CI on Github and thus eases the review
as your peers already know then that it compiles and passes the existing tests.
Keep in mind that our Github repo is a read only mirror of our SVN repository and hence
PR's cannot be merged directly. But we have some handy tools at
https://svn.apache.org/repos/asf/httpd/dev-tools/github to cope with this.

The 2.4.x branch is under RTC (Review then commit) with very few exceptions (see the section
"Current exceptions for RTC for this branch:" in the STATUS file at https://svn.apache.org/repos/asf/httpd/httpd/branches/2.4.x)
If you want to backport a patch to the 2.4.x branch just add your proposal to the STATUS file
as last proposal in the "PATCHES PROPOSED TO BACKPORT FROM TRUNK:" section.
Three committers (that can include you) need to give a +1 vote to the patch and no -1 must be voted.
Then the patch can be backported.
Keep in mind that backports need to follow the API and ABI rules we have for stable branches.
In this case this means that new stuff can be added to the API but you cannot remove or change
existing API's. For ABI compliance it is important to add new members of structs always to the end of the struct.
If a patch cannot be merged unmodified from trunk please create a patch that can be merged and either point to
it or create a PR on Github against the 2.4.x branch. Again the scripts in https://svn.apache.org/repos/asf/httpd/dev-tools/github
are handy for this task.

Regards

Rüdiger


>
> I have the following changes for DAVLockDiscovery:
> r1905327
> r1905230
> r1905229
> r1905206
> r1905170
> r1904662
> r1904638
>
Re: svn commit: r1905170 - /httpd/httpd/trunk/modules/dav/main/mod_dav.c [ In reply to ]
On Wed, Nov 16, 2022 at 08:05:43AM +0100, Ruediger Pluem wrote:
> If you want to backport a patch to the 2.4.x branch just add your proposal to the STATUS file

This way?
Index: STATUS
===================================================================
--- STATUS (revision 1905352)
+++ STATUS (working copy)
@@ -282,7 +282,25 @@
make it nonblocking (by default)?
jim: Non-blocking seems the best way to handle...

+ *) mod_dav: Open the lock database read-only when possible

+ trunk patch: http://svn.apache.org/r1905229
+ 2.4.x patch: trunk works
+ +1: manu
+
+ *) mod_dav: DAVlockDiscovery option to disable WebDAV lock discovery
+
+ trunk patch: http://svn.apache.org/r1904638
+ trunk patch: http://svn.apache.org/r1904662
+ trunk patch: http://svn.apache.org/r1905170
+ trunk patch: http://svn.apache.org/r1905206
+ trunk patch: http://svn.apache.org/r1905230
+ trunk patch: http://svn.apache.org/r1905327
+ 2.4.x patch: trunk works, except for
+ docs/manual/mod/mod_dav_fs.html.en.utf8 on trunk that is
+ docs/manual/mod/mod_dav_fs.html.en on 2.4.x branch
+ +1: manu
+
PATCHES/ISSUES THAT ARE STALLED

*) core: avoid duplicate headers when using ap_send_error_response.



--
Emmanuel Dreyfus
manu@netbsd.org
Re: svn commit: r1905170 - /httpd/httpd/trunk/modules/dav/main/mod_dav.c [ In reply to ]
On 11/17/22 6:41 PM, Emmanuel Dreyfus wrote:
> On Wed, Nov 16, 2022 at 08:05:43AM +0100, Ruediger Pluem wrote:
>> If you want to backport a patch to the 2.4.x branch just add your proposal to the STATUS file
>
> This way?

Almost :-) Only a small nitpick. We don't backport the HTML changes to the documentation only the XML changes.
After the backport has happened you can either build the documentation again on the 2.4.x branch and commit there
(this is CTR) or you just do nothing as rebuilding the docs is part of the release procedure for 2.4.x.

Some people also put a svn command in the 2.4.x patch line like:

svn merge -c 1904638,1904662,1905170,1905206,1905230,1905327 ^/httpd/httpd/trunk .

Or they use a little bit different formatting. But these are all matters of taste and each of us has
a little different style there. You will notice this over time and find your own one.


> Index: STATUS
> ===================================================================
> --- STATUS (revision 1905352)
> +++ STATUS (working copy)
> @@ -282,7 +282,25 @@
> make it nonblocking (by default)?
> jim: Non-blocking seems the best way to handle...
>
> + *) mod_dav: Open the lock database read-only when possible
>
> + trunk patch: http://svn.apache.org/r1905229
> + 2.4.x patch: trunk works
> + +1: manu
> +
> + *) mod_dav: DAVlockDiscovery option to disable WebDAV lock discovery
> +
> + trunk patch: http://svn.apache.org/r1904638
> + trunk patch: http://svn.apache.org/r1904662
> + trunk patch: http://svn.apache.org/r1905170
> + trunk patch: http://svn.apache.org/r1905206
> + trunk patch: http://svn.apache.org/r1905230
> + trunk patch: http://svn.apache.org/r1905327
> + 2.4.x patch: trunk works, except for
> + docs/manual/mod/mod_dav_fs.html.en.utf8 on trunk that is
> + docs/manual/mod/mod_dav_fs.html.en on 2.4.x branch
> + +1: manu
> +
> PATCHES/ISSUES THAT ARE STALLED
>
> *) core: avoid duplicate headers when using ap_send_error_response.
>

Regards

Rüdiger