On Wed, Sep 4, 2019 at 2:47 PM Johan Corveleyn <jcor...@gmail.com> wrote: > > On Wed, Sep 4, 2019 at 2:01 PM Branko Čibej <br...@apache.org> wrote: > > > > 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. > > Perfect. > > > > 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. > > Ack, will fix. > > > > + 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. > > OK, I'll add those tests too, should be easy enough. > > (BTW, I copied the test from the cache_control_header() test in the > same file :-)) > > Thanks for the review! > -- > Johan
Committed in r1866425. -- Johan
