Am 2024-10-02 um 16:02 schrieb Nathan Hartman:
On Tue, Oct 1, 2024 at 10:03 AM Franz Sirl
<franz.sirl-ker...@lauterbach.com> wrote:
Looking at the transferred data with wireshark shows this at the end:
```
<S:log-item>
<S:modified-path node-kind="file" text-mods="true"
prop-mods="false">...</S:modified-path>
... (>40000 lines of modified-path/added-path redacted)
<S:modified-path node-kind="file" text-mods="true"
prop-mods="false">...</S:modified-path>
</S:log-report>
```
So the closing tag `</S:log-item>` is missing. This closing tag is only
sent by log_revision_receiver() in subversion/mod_dav_svn/reports/log.c
and is used as a callback in
subversion/libsvn_repos/log.c.
Looking at the code the only failure mode I could imagine so far is that
if the last revision processed for this transaction ends up with
`baton.found_rev_of_interest = FALSE`.
In this case send_log() doesn't call callbacks->revision_receiver() and
so the log-item tag never gets closed?
Thank you for the analysis.
Would you say that the Issue 4711 patch in the issue tracker [3]
improves the situation, even if it's still missing something?
To make it clear, all the tests I've done on client+server have
the patch [3] applied. It's in use for quite some time here without
bad effects (and fixed the problem for us, the reporter of 4711 is a
colleague of mine). I believe the log-item bug is totally separate
bug on server-side. I just meant to express that maybe a similar
missing corner case handling (like 4711 on client) is the reason
for this bug on the server-side.
If we suspect that baton.found_rev_of_interest being FALSE for the
last processed revision causes the missing tag, we'll need to come up
with a reproduction recipe that triggers that. Could you rerun the
command but with a revision range where the last revision *is* of
interest, and tell us if the XML is formed correctly?
The command worked well for a long time, only after one of my colleagues
committed a merge from the trunk to this release branch it stopped to work.
This merge contained mergeinfo about the merged trunk revisions and also
mergeinfo about the merges from the colleagues user branch to trunk.
Probably this changed internal processing order in a way that made a
baton.found_rev_of_interest=FALSE come as the last log-item.
With a http:// URL the command fails midway while outputting the XML
resulting in a truncated file (2.8M size). With a file:// URL the XML is
5.4M in size and passes (with --incremental removed) a validity check
with `jing -c subversion/svn/schema/log.rnc`.
After thinking about it some more it occurred to me that since file://
is OK, subversion/libsvn_repos/log.c should be fine and rather the
problem is in subversion/mod_dav_svn/reports/log.c.
So I came up with the attached tentative and so far untested patch.
I'm currently building packages and will test them as soon as build is
finished.
BTW, it would make comparing/diffing the XML easier if both file:// and
http:// outputs would use the same template to output the <path> tag.
Franz
Index: subversion-1.14.x/subversion/mod_dav_svn/reports/log.c
===================================================================
--- subversion-1.14.x/subversion/mod_dav_svn/reports/log.c (revision
1921084)
+++ subversion-1.14.x/subversion/mod_dav_svn/reports/log.c (working copy)
@@ -61,6 +61,9 @@
callbacks. */
svn_boolean_t needs_log_item;
+ /* Whether we've written the </S:log-item> closure for the current report. */
+ svn_boolean_t needs_log_item_closure;
+
/* How deep we are in the log message tree. We only need to surpress the
SVN_INVALID_REVNUM message if the stack_depth is 0. */
int stack_depth;
@@ -107,11 +110,28 @@
SVN_ERR(dav_svn__brigade_printf(lrb->bb, lrb->output,
"<S:log-item>" DEBUG_CR));
lrb->needs_log_item = FALSE;
+ lrb->needs_log_item_closure = TRUE;
}
return SVN_NO_ERROR;
}
+/* If LRB->needs_log_item_closure is true, send the "</S:log-item>"
+ end element and set LRB->needs_log_item_closure to zero.
+ Else do nothing. */
+static svn_error_t *
+maybe_end_log_item(struct log_receiver_baton *lrb)
+{
+ if (lrb->needs_log_item_closure)
+ {
+ SVN_ERR(dav_svn__brigade_printf(lrb->bb, lrb->output,
+ "</S:log-item>" DEBUG_CR));
+ lrb->needs_log_item_closure = FALSE;
+ }
+
+ return SVN_NO_ERROR;
+}
+
/* Utility for log_receiver opening a new XML element in LRB's brigade
for LOG_ITEM and return the element's name in *ELEMENT. Use POOL for
temporary allocations.
@@ -322,6 +342,7 @@
SVN_ERR(dav_svn__brigade_puts(lrb->bb, lrb->output,
"</S:log-item>" DEBUG_CR));
+ lrb->needs_log_item_closure = FALSE;
/* In general APR will flush the brigade every 8000 bytes through the filter
stack, but log items may not be generated that fast, especially in
@@ -493,6 +514,7 @@
lrb.output = output;
lrb.needs_header = TRUE;
lrb.needs_log_item = TRUE;
+ lrb.needs_log_item_closure = FALSE;
lrb.stack_depth = 0;
/* lrb.requested_custom_revprops set above */
@@ -536,6 +558,14 @@
goto cleanup;
}
+ if ((serr = maybe_end_log_item(&lrb)))
+ {
+ derr = dav_svn__convert_err(serr, HTTP_INTERNAL_SERVER_ERROR,
+ "Error during REPORT response.",
+ resource->pool);
+ goto cleanup;
+ }
+
if ((serr = dav_svn__brigade_puts(lrb.bb, lrb.output,
"</S:log-report>" DEBUG_CR)))
{
