[PATCH] Incorrect `svn log` behavior from mod_dav_svn from

Karl Fogel <kfogel@newton.ch.collab.net> writes:

> Daniel Rall <dlr@finemaltcoding.com> writes:
> > The output that I'm getting from `svn log` from the latest code seems
> > a bit odd:
>
> Whups.
>
> That should be easy to fix (note that -1 is the real value of
> SVN_INVALID_REVNUM). Can you file a quick issue on it? I'll bet it
> gets squashed in no time; well, *almost* no time -- I'm going to bed
> now :-).
>
> You may find it faster to patch than to make an issue, even.

It turned it that no, it was not faster to patch. But I took a stab
at it anyhow, with some debugging assistence from Ben and a solution
suggestion from Greg.

The log message problem is not reproducable over ra_local (you need at
least 1 revnum to have a shot at it), but is reproducable over ra_dav.

The problem is not in the client library, but on the server-side.
Specifically, the response returned by mod_dav_svn wraps the
SVN_ERR_FS_NOT_FOUND produced when determining the response in a
<S:log-report> element, rather than in the <D:error> element which the
client-side recognizes as an error.

REPORT /repos/svn/trunk HTTP/1.1
Host: 216.127.237.133:81
User-Agent: neon/0.23.5 SVN/0.15.0 (dev build)
Connection: TE
TE: trailers
Content-Length: 142
Content-Type: text/xml

<S:log-report xmlns:S="svn:">
<S:start-revision>3782</S:start-revision><S:end-revision>1</S:end-revision><S:path>asdf</S:path></S:log-report>
HTTP/1.1 200 OK
Date: Thu, 14 Nov 2002 05:44:31 GMT
Server: Apache/2.0.44-dev (Unix) mod_ssl/2.0.44-dev OpenSSL/0.9.6 DAV/2 SVN/0.14.5 (dev build)
Transfer-Encoding: chunked
Content-Type: text/xml; charset="utf-8"

130
<?xml version="1.0" encoding="utf-8"?>
<S:log-report xmlns:S="svn:" xmlns:D="DAV:">
<S:log-item>
<D:version-name>-1</D:version-name>
<D:creator-displayname></D:creator-displayname>
<S:date></S:date>
<D:comment>file not found: revision `3782', path `/trunk/asdf'</D:comment>
</S:log-item>
</S:log-report>

0

Note that the response body actually comes back gzipped. I used
ethereal to record the transaction (thanks Ben), then used telnet to
reply the response, leaving out the "Accept-Encoding: gzip" HTTP
header (which I assume Neon is generating). This allowed me to see
the plaintext of the response.

Here's a completely untested patch to mod_dav_svn (having difficulties
getting mod_dav_svn built), approximating the lazy writing of the DAV
XML header fix suggested by gstein. Even though we should always get
a SVN_ERR_FS_NOT_FOUND, I left in the old error handling for the case
where we've already started writing the response.

Some up for reviewing this/testing it out? If not, be certain that I
will corner YOU @ ApacheCon. ;-)

* subversion/mod_dav_svn/log.c

  (struct log_receiver_baton): Added a needs_header flag to the
  internal baton to indicate whether we've written the <S:log-report>
  header. Allows for lazy writes to support mod_dav-based error
  handling.

  (svn_types.h): Imported for svn_boolean_t for needs_header flag.

  (dav_svn__log_report): Initialized the baton's needs_header flag to
  true. Moved writing of DAV XML header into log_receiver. Return
  with a dav_error * if an error is detected before we start sending
  the response (e.g. when a we get a SVN_ERR_FS_NOT_FOUND from a
  request for a non-existent resource).

  (log_receiver): Send DAV XML header and <S:log-report> start if we
  still need to send the header (determined by the needs_header flag
  on our baton).

Index: subversion/mod_dav_svn/log.c
===================================================================
--- subversion/mod_dav_svn/log.c (revision 3793)
+++ subversion/mod_dav_svn/log.c (working copy)
@@ -26,6 +26,7 @@
 #include "svn_pools.h"
 #include "svn_repos.h"
 #include "svn_fs.h"
+#include "svn_types.h"
 #include "svn_xml.h"
 #include "svn_path.h"
 
@@ -40,6 +41,10 @@
 
   /* where to deliver the output */
   ap_filter_t *output;
+
+ /* Whether we've written the <S:log-report> header. Allows for lazy
+ writes to support mod_dav-based error handling. */
+ svn_boolean_t needs_header;
 };
 
 
@@ -65,6 +70,16 @@
 {
   struct log_receiver_baton *lrb = baton;
 
+ if (lrb->needs_header)
+ {
+ /* Start the log report. */
+ send_xml(lrb,
+ DAV_XML_HEADER DEBUG_CR
+ "<S:log-report xmlns:S=\"" SVN_XML_NAMESPACE "\" "
+ "xmlns:D=\"DAV:\">" DEBUG_CR);
+ lrb->needs_header = FALSE;
+ }
+
   send_xml(lrb,
            "<S:log-item>" DEBUG_CR
            "<D:version-name>%" SVN_REVNUM_T_FMT "</D:version-name>" DEBUG_CR
@@ -236,12 +251,12 @@
   lrb.bb = apr_brigade_create(resource->pool, /* not the subpool! */
                               output->c->bucket_alloc);
   lrb.output = output;
+ lrb.needs_header = TRUE;
 
- /* Start the log report. */
- send_xml(&lrb,
- DAV_XML_HEADER DEBUG_CR
- "<S:log-report xmlns:S=\"" SVN_XML_NAMESPACE "\" "
- "xmlns:D=\"DAV:\">" DEBUG_CR);
+ /* Our svn_log_message_receiver_t sends the <S:log-report> header in
+ a lazy fashion. Before writing the first log message, it assures
+ that the header has already been sent (checking the needs_header
+ flag in our log_receiver_baton structure). */
 
   /* Send zero or more log items. */
   serr = svn_repos_get_logs(repos->repos,
@@ -256,27 +271,32 @@
 
   if (serr)
     {
- /* ### We've definitely generated some content into the output
- ### filter, which means that we cannot return an error here.
- ### In the future, mod_dav may specify a way to signal an
- ### error even after the response stream has begun.
- ###
- ### So for now, we "return" the error by invoking the
- ### log_receiver on the error message itself.
- ###
- ### http://subversion.tigris.org/issues/show_bug.cgi?id=816
- ### describes a situation where this helps. */
-
- /* Don't bother to check for error; we can't do anything with it
- if we get one. */
- log_receiver(&lrb,
- NULL,
- SVN_INVALID_REVNUM,
- "", "",
- serr->message,
- resource->pool);
-
- serr = NULL;
+ /* If we can, report a DAV error. */
+ if (lrb.needs_header)
+ {
+ /* Bail out before writing any of <S:log-report>. */
+ return dav_svn_convert_err(serr, HTTP_BAD_REQUEST, serr->message);
+ }
+ else
+ {
+ /* ### We've sent some content to the output filter, meaning
+ ### that we cannot simply return an error here. In the
+ ### future, mod_dav may specify a way to signal an error
+ ### even after the response stream has begun.
+
+ ### For now we punt, sending the error message to the
+ ### client as a <S:log-item> (using its <D:version-name>
+ ### and <D:comment> children).
+
+ ### http://subversion.tigris.org/issues/show_bug.cgi?id=816
+ ### describes a situation where this helps.*/
+ log_receiver(&lrb,
+ NULL,
+ SVN_INVALID_REVNUM,
+ "", "",
+ serr->message,
+ resource->pool);
+ }
     }
   
   /* End the log report. */

-- 
Daniel Rall <dlr@finemaltcoding.com>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org