On Wed, Oct 2, 2024 at 10:02 AM Nathan Hartman <hartman.nat...@gmail.com> wrote:
>
> On Tue, Oct 1, 2024 at 10:03 AM Franz Sirl
> <franz.sirl-ker...@lauterbach.com> wrote:
> >
> > Hi,
> >
> > recently this svn log command started to fail like that:
> >
> > ```
> > > svn log --xml --verbose --search @ --use-merge-history --incremental
> > --revision 172342:{2024-09-30}
> > http://svn/svn/project/branches/cd/2024_09 >/dev/null
> > svn: E175009: The XML response contains invalid XML
> > svn: E130003: Malformed XML: mismatched tag at line 139122
> > ```
> >
> > Removing the --verbose lets the command succeed, so it seems related to
> > the sending of the changed pathes. Also running the command with a
> > file:// URL directly on the server works fine.
> >
> > Server and client are 1.14.3+issue4711-fix (BTW, when will there be an
> > official release with the fix for issue 4711?) running on x64-linux.
> > Actually the failure mode looks quite similar to issue 4711, but this
> > time the problem happens on the server side.
>
>
> I thought I remembered fix(es) for unbalanced XML closing tags being
> implemented, committed, and backported to the 1.14.x branch!! But
> looking through the history, I only see two regression tests that were
> committed and both are @XFail() (meaning, expected FAILs).
>
> Those tests were committed to trunk in r1877310 and r1897133.
>
> Now, it seems there are two issues in the issue tracker related to
> this: Issue 4711 [1] (which you referenced) and Issue 4856 [2]. I'm
> adding a note to both...
>
>
> > 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?
>
> 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?
>
>
> > Hope this helps to track down the bug,
> > Franz
> >
> > PS: While investigating that I found a small cosmetic bug in the
> > prototype (fortunately the code follows the definition) for do_logs() in
> > subversion/libsvn_repos/log.c:
> > ```
> > Index: subversion/libsvn_repos/log.c
> > ===================================================================
> > --- subversion/libsvn_repos/log.c (revision 1921065)
> > +++ subversion/libsvn_repos/log.c (working copy)
> > @@ -1719,8 +1719,8 @@
> > int limit,
> > svn_boolean_t strict_node_history,
> > svn_boolean_t include_merged_revisions,
> > + svn_boolean_t subtractive_merge,
> > svn_boolean_t handling_merged_revisions,
> > - svn_boolean_t subtractive_merge,
> > svn_boolean_t ignore_missing_locations,
> > const apr_array_header_t *revprops,
> > svn_boolean_t descending_order,
> > ```
>
>
> Good catch! The 11th and 12th arguments are swapped in the forward
> declaration. I'll commit this fix in a bit, but before I do that, I
> want to go over the call sites and verify they do not have the
> arguments swapped due to copying-and-pasting of the (wrong) forward
> declaration. Hopefully as you say the code follows the definition (and
> not the forward declaration).
>
> Thanks again,
> Nathan
Forgot to include the links... doh!
[1] Issue 4711: https://issues.apache.org/jira/browse/SVN-4711?issueNumber=4711
[2] Issue 4856: https://issues.apache.org/jira/browse/SVN-4856?issueNumber=4856
[3] Patch in issue tracker for Issue 4711:
https://issues.apache.org/jira/secure/attachment/13045647/4711_patch_new.txt
