On 15.11.2018 17:27, Branko Čibej wrote:
> [Moved from users@]
>
> On 06.11.2018 19:16, Branko Čibej wrote:
>> On 04.11.2018 20:11, Branko Čibej wrote:
>>> On 04.11.2018 18:57, Thorsten Schöning wrote:
>>>> Guten Tag Branko Čibej,
>>>> am Sonntag, 4. November 2018 um 17:47 schrieben Sie:
>>>>
>>>>> I'm not sure what you mean by "handles more than only DAV successfully"
>>>> I thought it might be possible that GitHub answers differently but
>>>> properly, because the other check mentioned something about HTTP v2.
>>>> Because of TLS, I was unable to look at the requests and responses
>>>> then, but it's like you said, they don't provide DAV-headers in their
>>>> response to OPTION.
>>>>
>>>>> And yes, the HTTP/DAV specification requires that header to be present
>>>>> in the response.
>>>> Which you didn't care about before and things worked for some years
>>>> for some users.
>>> We made this change because users complained about unhelpful error
>>> messages when they tried to connect to a server that did not even
>>> implement HTTP/DAV. The error message was "Malformed XML in response"
>>> which wasn't exactly helpful for diagnosing the problem.
>>>
>>> I admit I didn't have GitHub in mind when I added this check. ...
>> I added a test case to our suite that tries the following command:
>>
>> svn info https://github.com/apache/subversion/trunk
>>
>>
>> It runs on one of our build slaves, so we'll know fairly soon when (if)
>> GitHub deploys a fix. And, of course, we'll also know if this feature
>> breaks again in future.
>>
>> http://svn.apache.org/r1845942
>
> I created SVN-4789 to track this and added a patch with a possible fix.
> I did not commit it because it contains what is, in my opinion, a really
> horrible hack; but it does let our client work against
> https://github.com again. I would very much prefer that GitHub adds that
> trivial one-liner fix to their bridge implementation. However, if this
> issue persists and prevents users from migrating to 1.11.x and later, we
> may have to use this hack under protest. Note that it is in some ways
> similar to the now-defunct ASP.NET hack.
>
> [[[
> Hack ra_serf to work with GitHub's bridge again.
>
> * subversion/libsvn_ra_serf/options.c
> (options_context_t): New member 'received_github_request_id_header'.
> (capabilities_headers_iterator_callback): Check if we received that header.
> (options_response_handler): Allow DAV responses without DAV headers, if
> the 'X-GitHub-Request-Id' header is present.
>
> * subversion/tests/cmdline/dav_tests.py
> (connect_to_github_server):
> Remove the @XFail annotation. Add an @Issue annotation.
> Change the test URL to point at HTTPd's GitHub mirror.
>
> Issue: SVN-4789
> ]]]
Actually the attached updated patch is a bit more to my taste.
-- Brane
Index: subversion/libsvn_ra_serf/options.c
===================================================================
--- subversion/libsvn_ra_serf/options.c (revision 1846667)
+++ subversion/libsvn_ra_serf/options.c (working copy)
@@ -74,6 +74,10 @@
/* Have we received any DAV headers at all? */
svn_boolean_t received_dav_header;
+ /* ### FIXME: SVN-4789
+ Have we received the bespoke GitHub request ID header? */
+ svn_boolean_t received_github_request_id_header;
+
const char *activity_collection;
svn_revnum_t youngest_rev;
@@ -355,6 +359,22 @@
}
}
+ /* ### FIXME: SVN-4789
+ GitHub's Subversion bridge does not set any DAV: headers in its
+ response, even though it supports bits of our HTTP protocol and
+ returns a valid response body to an OPTIONS request. We try to work
+ around that monumental brokenness by checking for this
+ GitHub-specific response header.
+
+ Note that the case-sensitive string comparison here is intentional;
+ we do *not* want to support any number of strange hacks, so if
+ GitHub changes the case of their header name, this becomes their
+ problem entirely. */
+ else if (strcmp(key, "X-GitHub-Request-Id") == 0)
+ {
+ opt_ctx->received_github_request_id_header = TRUE;
+ }
+
return 0;
}
@@ -401,8 +421,19 @@
serf_bucket_headers_do(hdrs, capabilities_headers_iterator_callback,
opt_ctx);
- /* Bail out early if we're not talking to a DAV server.
- Note that this check is only valid if we've received a success
+
+ /* ### FIXME: SVN-4789
+ Treat GitHub as a DAV server, even though it's broken. */
+ if (opt_ctx->received_github_request_id_header
+ && !opt_ctx->received_dav_header)
+ {
+ /* FIXME: We really should emit a warning here, but apparently
+ we don't have a notification API for that. */
+ opt_ctx->received_dav_header = TRUE;
+ }
+
+ /* Bail out early if we're not talking to a DAV server. Note
+ that this check is only valid if we've received a success
response; redirects and errors don't count. */
if (opt_ctx->handler->sline.code >= 200
&& opt_ctx->handler->sline.code < 300
@@ -410,7 +441,7 @@
{
return svn_error_createf
(SVN_ERR_RA_DAV_OPTIONS_REQ_FAILED, NULL,
- _("The server at '%s' does not support the HTTP/DAV protocol"),
+ _("The server at '%s' does not support the WebDAV protocol"),
session->session_url_str);
}
Index: subversion/tests/cmdline/dav_tests.py
===================================================================
--- subversion/tests/cmdline/dav_tests.py (revision 1846667)
+++ subversion/tests/cmdline/dav_tests.py (working copy)
@@ -69,12 +69,15 @@
#----------------------------------------------------------------------
-@XFail()
+@Issue(4789)
@SkipUnless(svntest.main.is_remote_http_connection_allowed)
def connect_to_github_server(sbox):
"connect to GitHub's SVN bridge"
- github_mirror_url = 'https://github.com/apache/subversion/trunk'
+ # FIXME: For some reason, trying this against Subversion's own mirror
+ # always results in: 504 Gateway Timeout.
+ #github_mirror_url = 'https://github.com/apache/subversion/trunk/'
+ github_mirror_url = 'https://github.com/apache/httpd/trunk/'
# Skip this test if we can't connect to the GitHub server.
# We check this here instead of in a SkipUnless() predicate decorator,
