On 04.09.2019 11:44, Johan Corveleyn wrote: > On Tue, Sep 3, 2019 at 8:01 AM Branko Čibej <br...@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
