Am 2024-10-15 um 12:23 schrieb Daniel Sahlberg:
Den mån 14 okt. 2024 kl 13:07 skrev Franz Sirl <Franz.Sirl-
ker...@lauterbach.com <mailto:franz.sirl-ker...@lauterbach.com>>:
Hi,
the attached patch fixes both issue 4856 and the (different) bug
we were seeing in one of our own repositories.
The problem boils down to the fact that with `svn log --xml --verbose`
a log-item is opened every time the PATH_CHANGE_RECEIVER callback is
used. But if the revision is later declared "unimportant", the
corresponding log-item closure is never sent via the REVISION_RECEIVER
callback.
The patch fixes that by always using an intermediate callback routine
and recording the call of inner() in a new flag in
interesting_merge_baton_t. This flag is then used to decide if the
REVISION_RECEIVER callback has to be called to close the log-item.
With yet another flag in interesting_merge_baton_t record_inner_call()
could be merged into interesting_merge() if you prefer that.
Thank you for the patch!
In the discussion about issue 4177, Daniel Shahaf mentioned that there
was an inconsistency about the number of times the receiver callback was
called, so it seems likely you have found that issue.
I'm going to test this further when time permits, hopefully this week.
Hello Daniel,
in the meantime I noticed that the previous patch streams out a lot of
duplicated revisions. I've come up with v2 of the patch, which probably
has a bit of unnecessary code because I'm not so sure about when and how
this code recurses.
The patch avoids the duplicates, but it also triggers a strange behavior
with --limit. If I add for example a `--limit 1` to the log command, the
XML generated by file: and http: start to differ a lot on the repository
I used for testing. I don't know what to make of that...
BTW, the SVN_ERR_ASSERT() in subversion/libsvn_ra_serf/log.c was just
helpful to get a better error report. It's not necessary for the
functionality of the patch.
Hope this helps,
Franz
Index: subversion-1.14.x/subversion/libsvn_ra_serf/log.c
===================================================================
--- subversion-1.14.x/subversion/libsvn_ra_serf/log.c (revision 1921329)
+++ subversion-1.14.x/subversion/libsvn_ra_serf/log.c (working copy)
@@ -299,6 +299,7 @@
"subtractive-merge",
FALSE);
+ SVN_ERR_ASSERT(attrs != NULL);
rev_str = svn_hash_gets(attrs, "revision");
if (rev_str)
{
Index: subversion-1.14.x/subversion/libsvn_repos/log.c
===================================================================
--- subversion-1.14.x/subversion/libsvn_repos/log.c (revision 1921329)
+++ subversion-1.14.x/subversion/libsvn_repos/log.c (working copy)
@@ -1216,6 +1216,15 @@
/* Set to TRUE if we found it. */
svn_boolean_t found_rev_of_interest;
+ /* Set to TRUE if we called PATH_CHANGE_RECEIVER via INNER. */
+ svn_boolean_t inner_got_used;
+
+ /* Set to TRUE if we need the simple mode of interesting_merge() */
+ svn_boolean_t simple_mode;
+
+ svn_boolean_t handling_merged_revision;
+ svn_bit_array__t *nested_merges;
+
/* We need to invoke this user-provided callback if not NULL. */
svn_repos_path_change_receiver_t inner;
void *inner_baton;
@@ -1235,36 +1244,40 @@
interesting_merge_baton_t *b = baton;
apr_hash_index_t *hi;
- if (b->inner)
- SVN_ERR(b->inner(b->inner_baton, change, scratch_pool));
-
- if (b->found_rev_of_interest)
- return SVN_NO_ERROR;
-
- /* Look at each path on the log target's mergeinfo. */
- for (hi = apr_hash_first(scratch_pool, b->log_target_history_as_mergeinfo);
- hi;
- hi = apr_hash_next(hi))
+ if (!(b->found_rev_of_interest || b->simple_mode))
{
- const char *mergeinfo_path = apr_hash_this_key(hi);
- svn_rangelist_t *rangelist = apr_hash_this_val(hi);
- /* Check whether CHANGED_PATH at revision REV is a child of
- a (path, revision) tuple in LOG_TARGET_HISTORY_AS_MERGEINFO. */
- if (svn_fspath__skip_ancestor(mergeinfo_path, change->path.data))
+ /* Look at each path on the log target's mergeinfo. */
+ for (hi = apr_hash_first(scratch_pool,
b->log_target_history_as_mergeinfo);
+ hi;
+ hi = apr_hash_next(hi))
{
- int i;
+ const char *mergeinfo_path = apr_hash_this_key(hi);
+ svn_rangelist_t *rangelist = apr_hash_this_val(hi);
- for (i = 0; i < rangelist->nelts; i++)
+ /* Check whether CHANGED_PATH at revision REV is a child of
+ a (path, revision) tuple in LOG_TARGET_HISTORY_AS_MERGEINFO. */
+ if (svn_fspath__skip_ancestor(mergeinfo_path, change->path.data))
{
- svn_merge_range_t *range
- = APR_ARRAY_IDX(rangelist, i, svn_merge_range_t *);
- if (b->rev > range->start && b->rev <= range->end)
- return SVN_NO_ERROR;
+ int i;
+
+ for (i = 0; i < rangelist->nelts; i++)
+ {
+ svn_merge_range_t *range
+ = APR_ARRAY_IDX(rangelist, i, svn_merge_range_t *);
+ if (b->rev > range->start && b->rev <= range->end)
+ return SVN_NO_ERROR;
+ }
}
}
}
+ if (b->inner && !(b->nested_merges && b->handling_merged_revision &&
svn_bit_array__get(b->nested_merges, b->rev)))
+ {
+ SVN_ERR(b->inner(b->inner_baton, change, scratch_pool));
+ b->inner_got_used = TRUE;
+ }
+
b->found_rev_of_interest = TRUE;
return SVN_NO_ERROR;
@@ -1310,9 +1323,19 @@
{
svn_repos_log_entry_t log_entry = { 0 };
log_callbacks_t my_callbacks = *callbacks;
+ apr_pool_t *scratch_pool;
interesting_merge_baton_t baton;
+ baton.inner_got_used = FALSE;
+ baton.inner = callbacks->path_change_receiver;
+ baton.inner_baton = callbacks->path_change_receiver_baton;
+ baton.handling_merged_revision = handling_merged_revision;
+ baton.nested_merges = nested_merges;
+ my_callbacks.path_change_receiver = interesting_merge;
+ my_callbacks.path_change_receiver_baton = &baton;
+ callbacks = &my_callbacks;
+
/* Is REV a merged revision that is already part of
LOG_TARGET_HISTORY_AS_MERGEINFO? If so then there is no
need to send it, since it already was (or will be) sent.
@@ -1323,18 +1346,14 @@
&& apr_hash_count(log_target_history_as_mergeinfo))
{
baton.found_rev_of_interest = FALSE;
+ baton.simple_mode = FALSE;
baton.rev = rev;
baton.log_target_history_as_mergeinfo = log_target_history_as_mergeinfo;
- baton.inner = callbacks->path_change_receiver;
- baton.inner_baton = callbacks->path_change_receiver_baton;
-
- my_callbacks.path_change_receiver = interesting_merge;
- my_callbacks.path_change_receiver_baton = &baton;
- callbacks = &my_callbacks;
}
else
{
baton.found_rev_of_interest = TRUE;
+ baton.simple_mode = TRUE;
}
SVN_ERR(fill_log_entry(&log_entry, rev, fs, revprops, callbacks, pool));
@@ -1345,15 +1364,15 @@
revision. */
if (baton.found_rev_of_interest)
{
- apr_pool_t *scratch_pool;
-
/* Is REV a merged revision we've already sent? */
if (nested_merges && handling_merged_revision)
{
if (svn_bit_array__get(nested_merges, rev))
{
- /* We already sent REV. */
- return SVN_NO_ERROR;
+ if (!baton.inner_got_used)
+ /* We already sent REV and no log was streamed yet. */
+ return SVN_NO_ERROR;
+ log_entry.revision = SVN_INVALID_REVNUM;
}
else
{
@@ -1371,7 +1390,20 @@
&log_entry, scratch_pool));
svn_pool_destroy(scratch_pool);
}
+ else if (baton.inner_got_used)
+ {
+ svn_repos_log_entry_t empty_log_entry = { 0 };
+ /* Send the empty revision. */
+ empty_log_entry.revision = SVN_INVALID_REVNUM;
+ /* Pass a scratch pool to ensure no temporary state stored
+ by the receiver callback persists. */
+ scratch_pool = svn_pool_create(pool);
+ SVN_ERR(callbacks->revision_receiver(callbacks->revision_receiver_baton,
+ &empty_log_entry, scratch_pool));
+ svn_pool_destroy(scratch_pool);
+ }
+
return SVN_NO_ERROR;
}
@@ -1719,8 +1751,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,
