On 04.09.2019 11:44, Johan Corveleyn wrote:
> On Tue, Sep 3, 2019 at 8:01 AM Branko Čibej <brane_at_apache.org> wrote:
[...]
>>> Anyway, how about bringing this feature back in some form?
>>> - Revert r1724790?
>> This is clearly the simplest solution, but I have no idea what the
>> performance impact would be. From looking at the diff, my best guess is
>> that svn_fs_node_created_rev() and svn_fs_revision_prop() dominate.
>>
>>> - or only for "external GET urls"?
>>> - or only if some Apache directive is set?
>>>
>>> Thoughts?
>> I would prefer not to add yet another configuration knob to the server.
>> I agree that versioned-resource URLs are only interesting for DAV-aware
>> clients, and those clients already know how to check for modifications
>> without looking at Last-Modified. That would imply that adding the
>> header for external URLs is the right solution.
>>
>> -- Brane
> Thanks for your response, Brane.
>
> I think the below patch would do it (set the Last-Modified header only
> for "external URLs").
> It's basically a revert of r1724790 (and adding a test), plus wrapping
> the actual setting of Last-Modified inside the following condition:
>
> if ((resource->type == DAV_RESOURCE_TYPE_REGULAR)
> && (resource->info->repos_path == resource->info->uri_path->data))
Yes, all our resources are "regular", I think. The second part of the
condition just says that if something is called "/foo/bar" in the
repository, it's being accessed as
"http://example.org/repos-root/foo/bar" and not some alias. Which is
exactly what you want.
> const char *
> dav_svn__getetag(const dav_resource *resource, apr_pool_t *pool)
> {
> @@ -3219,6 +3263,25 @@ set_headers(request_rec *r, const dav_resource *re
> if (!resource->exists)
> return NULL;
>
> + if ((resource->type == DAV_RESOURCE_TYPE_REGULAR)
> + && (resource->info->repos_path == resource->info->uri_path->data))
> + {
> + /* Include Last-Modified header for 'external' GET requests
> + (i.e. requests to URI's not under /!svn), to support usage of an
> + SVN server as a file server, where the client needs timestamps
> + for instance to use as "last modification time" of files on disk. */
> + apr_time_t last_modified;
> +
> + last_modified = get_last_modified(resource);
"Declaration is initialisation" please, wherever possible. So:
const apr_time_t last_modified = get_last_modified(resource);
This also prevents you from accidentally modifying last_modified later on.
> + if (last_modified != -1)
> + {
> + /* Note the modification time for the requested resource, and
> + include the Last-Modified header in the response. */
> + ap_update_mtime(r, last_modified);
> + ap_set_last_modified(r);
> + }
> + }
> +
> /* generate our etag and place it into the output */
> apr_table_setn(r->headers_out, "ETag",
> dav_svn__getetag(resource, resource->pool));
> Index: subversion/tests/cmdline/mod_dav_svn_tests.py
> ===================================================================
> --- subversion/tests/cmdline/mod_dav_svn_tests.py (revision 1866345)
> +++ subversion/tests/cmdline/mod_dav_svn_tests.py (working copy)
> @@ -640,6 +640,29 @@ def propfind_propname(sbox):
> actual_response = r.read()
> verify_xml_response(expected_response, actual_response)
>
> +@SkipUnless(svntest.main.is_ra_type_dav)
> +def last_modified_header(sbox):
> + "verify 'Last-Modified' header on 'external' GETs"
> +
> + sbox.build(create_wc=False, read_only=True)
> +
> + headers = {
> + 'Authorization': 'Basic ' +
> base64.b64encode(b'jconstant:rayjandom').decode(),
> + }
> +
> + h = svntest.main.create_http_connection(sbox.repo_url)
> +
> + # GET /repos/iota
> + # Expect to see a Last-Modified header.
> + h.request('GET', sbox.repo_url + '/iota', None, headers)
> + r = h.getresponse()
> + if r.status != httplib.OK:
> + raise svntest.Failure('Request failed: %d %s' % (r.status, r.reason))
> + svntest.verify.compare_and_display_lines(None, 'Last-Modified',
> + svntest.verify.RegexOutput('.+'),
> + r.getheader('Last-Modified'))
> + r.read()
Interesting approach ... and sure, it should work.
But I'd also add a test to check that Last-Modified is *not* set on
responses to versioned-resource URLs. And maybe also check that the HEAD
method returns this header as well -- it should, and checking for that
won't hurt.
-- Brane
Received on 2019-09-04 14:01:01 CEST