As a refresher I got Claude Opus to review the latest patch and it found a
reasonably sized list of issues which at a glance all seem accurate and
match up to some of the issues you've encountered. Although some of the
listed issues are minor, there are some major issues that I'm not sure I
have enough codebase knowledge to confidently fix correctly.
What are your thoughts on the issues below before I look any deeper into
this?
Thanks,
Jordan
Attempt 2 at formatting...
Summary of the Change
The patch introduces a new RA capability, commit-allow-rev-props, that lets
svnsync set svn:author and svn:date directly during a commit (rather than
as a follow-up pre-revprop-change round trip). Mechanically, it does this
by:
1.
Adding a new public API pair, bumping versions in the standard SVN way:
-
svn_repos_get_commit_editor5 → svn_repos_get_commit_editor6 (new
txn_flags parameter)
-
svn_repos_fs_begin_txn_for_commit2 →
svn_repos_fs_begin_txn_for_commit3 (new flags parameter)
2.
Adding SVN_RA_CAPABILITY_COMMIT_ALLOW_REV_PROPS to svn_ra.h.
3.
Wiring SVN_FS_TXN_CLIENT_DATE through the commit editor in libsvn_repos.
4.
Making ra_local advertise the capability and pass SVN_FS_TXN_CLIENT_DATE
when the caller supplied svn:date; also making ra_local preserve a
caller-supplied svn:author instead of overwriting it.
5.
Teaching svnsync to check the capability and, when present, push
author/date in the commit and skip the post-commit revprop sync for them.
6.
Updating all internal callers (svnserve, mod_dav_svn) to the new
*_commit_editor6 / *_begin_txn_for_commit3 signatures.
The deprecation pattern, doc comments, and ABI wrappers all match
Subversion's normal conventions. The version bumps to 1.16 match the
current trunk in CHANGES. So far so good.
Now the problems.
------------------------------
CRITICAL ISSUES1. svnsync will hard-fail against svn:// and http(s)://
destinations
This is the most serious issue and would be release-blocking.
The new svnsync code unconditionally queries the new capability:
svnsync.cLines 1597-1602
/* Check if the destination supports setting author/date directly in
the commit, avoiding the need to update them as a second step. */
SVN_ERR(svn_ra_has_capability(rb->to_session,
&rb->has_commit_allow_rev_props_capability,
SVN_RA_CAPABILITY_COMMIT_ALLOW_REV_PROPS,
pool));
But the new capability is only registered in libsvn_ra_local. The other RA
layers behave as follows when queried for an unrecognized capability:
-
ra_svn (svn://):
subversion/libsvn_ra_svn/client.c::ra_svn_has_capability walks a static
lookup table and, if the capability isn't there, returns
SVN_ERR_UNKNOWN_CAPABILITY. The new constant is not in that table.
-
ra_serf (http(s)://):
subversion/libsvn_ra_serf/options.c::svn_ra_serf__has_capability looks up
the capability in session->capabilities, populated by parse_capabilities.
The new constant is never inserted (neither as capability_yes nor
capability_no), so the lookup returns NULL after the OPTIONS exchange and
the function returns SVN_ERR_UNKNOWN_CAPABILITY.
SVN_ERR() propagates this error fatally, so svnsync sync will abort at
startup for every non-file:// destination — i.e., the dominant real-world
case (svnsync is almost always run against a remote target). This silently
regresses all existing svnsync deployments.
The Subversion header itself spells out the rule the patch violates:
svn_ra.hLines 2302-2312
/* *** PLEASE READ THIS IF YOU ADD A NEW CAPABILITY ***
*
* RA layers generally fetch all capabilities when asked about any
* capability, to save future round trips. So if you add a new
* capability here, make sure to update the RA layers to remember
* it after any capabilities query.
*
* Also note that capability strings should not include colons,
* because we pass a list of client capabilities to the start-commit
* hook as a single, colon-separated string.
*/
Minimum required follow-up work:
-
Add {SVN_RA_CAPABILITY_COMMIT_ALLOW_REV_PROPS,
SVN_RA_SVN_CAP_COMMIT_ALLOW_REV_PROPS} to the table in
subversion/libsvn_ra_svn/client.c (~line 3081), and define
SVN_RA_SVN_CAP_COMMIT_ALLOW_REV_PROPS in subversion/include/svn_ra_svn.h.
-
Initialize the capability to capability_no in
subversion/libsvn_ra_serf/options.c::parse_capabilities (~line 397), and
define a SVN_DAV_NS_DAV_SVN_* symbol for it in
subversion/include/svn_dav.h. If you ever want HTTP to support this, also
advertise it in subversion/mod_dav_svn/version.c and recognize the header
in capabilities_headers_iterator_callback.
Until then, the safest local fix is to wrap the new check so that
SVN_ERR_UNKNOWN_CAPABILITY is swallowed and treated as "no":
svn_error_t *err = svn_ra_has_capability(rb->to_session,
&rb->has_commit_allow_rev_props_capability,
SVN_RA_CAPABILITY_COMMIT_ALLOW_REV_PROPS,
pool);
if (err && err->apr_err == SVN_ERR_UNKNOWN_CAPABILITY)
{
svn_error_clear(err);
rb->has_commit_allow_rev_props_capability = FALSE;
}
else
SVN_ERR(err);
…but this is only a workaround for the missing registration in the other RA
layers. The clean fix is to register it everywhere.
2. Server-side support is only partial; the capability is partially "lying"
Even on the file:// happy path, the capability is broader than the actual
implementation:
-
svnserve/serve.c is updated to call svn_repos_get_commit_editor6 but
always passes txn_flags = 0. So svnserve will never honor a caller-supplied
svn:date and never advertises the capability — consistent.
-
mod_dav_svn/activity.c and mod_dav_svn/lock.c are updated to call
svn_repos_fs_begin_txn_for_commit3 with only SVN_FS_TXN_CHECK_LOCKS. No
SVN_FS_TXN_CLIENT_DATE, no advertised capability — also consistent.
-
However, svn_repos__get_commit_ev2 (Ev2 path used by the second commit
editor in ra_plugin.c::svn_ra_local__get_commit_ev2_editor) is not updated
and still hardcodes SVN_FS_TXN_CHECK_LOCKS:
commit.cLines 1451-1454
SVN_ERR(svn_fs__editor_create(&eb->inner, &eb->txn_name,
repos->fs, SVN_FS_TXN_CHECK_LOCKS,
cancel_func, cancel_baton,
result_pool, scratch_pool));
So an Ev2 caller over ra_local sees has_capability(COMMIT_ALLOW_REV_PROPS)
== TRUE and supplies svn:date in revprops — but the FS layer overwrites the
date because SVN_FS_TXN_CLIENT_DATE was never set. svnsync today happens to
use the Ev1 commit editor (svn_ra_get_commit_editor3), so this misalignment
doesn't bite svnsync, but it does make the capability contract dishonest
for any other Ev2 client.
Either (a) plumb a flags parameter through svn_repos__get_commit_ev2 and
svn_ra_local__get_commit_ev2_editor (mirroring the Ev1 changes), or (b)
document that the capability only applies to the Ev1 commit editor
(uncomfortable).
------------------------------
SIGNIFICANT ISSUES3. Silent behavior change: pre-revprop-change hook no
longer fires for svn:author / svn:date on synced revisions
Today, every svnsync of a revision triggers a change_rev_prop for
svn:author and svn:date, which fires the destination's pre-revprop-change
hook. Many shops use that hook for audit logging, validation, ACLs, or even
rewriting author names during a sync.
After this patch, when the destination supports commit-allow-rev-props,
author and date are baked into the commit's initial revprops via the FS
layer's apply_revprops path inside the transaction. That path does not
invoke pre-revprop-change (it's only invoked from
svn_repos_fs_change_rev_prop*). And the replay_rev_finished change
short-circuits the entire revprop write step:
svnsync.cLines 1457-1463
if (apr_hash_count(filtered) > 0)
SVN_ERR(write_revprops(&filtered_count, rb->to_session, revision,
filtered,
NULL, subpool));
…and filtered was just unconditionally reset to an empty hash on the
commit-allow-rev-props branch.
Result: hooks that previously inspected/mutated author or date during a
sync stop being called. This is a quiet semantic change that deserves a
CHANGES note and probably an opt-in or opt-out flag (e.g.,
--use-commit-revprops/--no-commit-revprops) so that operators who depend on
the hook behavior aren't broken.
4. ra_local now permits any caller to spoof svn:author
This change in ra_local's Ev1 path applies to every caller, not just
svnsync:
ra_plugin.cLines 896-907
/* Copy the revprops table so we can add/modify properties. */
revprop_table = apr_hash_copy(pool, revprop_table);
/* If the caller hasn't provided an author, use the session username.
This allows tools like svnsync to specify the original author. */
if (! svn_hash_gets(revprop_table, SVN_PROP_REVISION_AUTHOR))
svn_hash_sets(revprop_table, SVN_PROP_REVISION_AUTHOR,
svn_string_create(sess->username, pool));
…and the equivalent change in the Ev2 path (~line 1818). Before the patch,
ra_local always overwrote svn:author with the session username. After the
patch, any caller can preempt that by putting an svn:author of their choice
into the revprop table.
For file:// access this isn't a meaningful security boundary (the caller
can already touch the FS directly). But it is an API contract change, and
libsvn_client (the regular svn ci path) does not currently set svn:author
in the revprop table, so this is silent today — until some unrelated client
starts populating it. Worth at least guarding with the new capability or a
documented contract that says "non-svnsync callers should not pre-populate
svn:author over ra_local".
5. Implicit dependency on commit-revprops capability
The new branch in replay_rev_started:
svnsync.cLines 1309-1320
if (rb->has_commit_allow_rev_props_capability)
filtered = filter_props(&filtered_count, rev_props,
filter_exclude_sync_only,
pool);
else if (rb->has_commit_revprops_capability)
filtered = filter_props(&filtered_count, rev_props,
filter_exclude_date_author_sync,
pool);
else
filtered = filter_props(&filtered_count, rev_props,
filter_include_log,
pool);
silently assumes a server that has commit-allow-rev-props also has
commit-revprops (otherwise the editor will simply drop the revprops on the
floor). This is true today (only ra_local advertises both, and it always
has), but the branches should either AND the two capabilities together
(has_commit_allow_rev_props_capability && has_commit_revprops_capability)
or document/assert the implication so a future RA layer can't bring back a
sneaky bug.
------------------------------
MEDIUM ISSUES6. Misleading / dead code in replay_rev_finished
if (rb->has_commit_allow_rev_props_capability)
{
/* Author and date were set during commit; we may still need to
handle any svnsync-specific properties if present (though they
should have been filtered out). */
filtered_count = 0;
filtered = apr_hash_make(subpool);
}
The comment says "we may still need to handle any svnsync-specific
properties if present" but the code then sets filtered to an empty hash and
skips write_revprops entirely. Either the comment is wrong, or the code is.
If there's a real concern that svnsync-prefixed revprops might leak
through, this branch needs actual handling; otherwise, drop the comment.
Also, the now-empty filtered is still threaded through
svnsync_normalize_revprops (and increments rb->normalized_rev_props_count
by zero) — harmless but pointless.
7. UX/notification regressions
-
"Copied properties for revision X" is suppressed unconditionally on the
commit-allow-rev-props path. That message is the only sign in svnsync's
output that revprops were synchronized for a given revision; users scanning
logs will notice the difference, and operators with log parsers may break.
-
The new banner "Destination supports commit-time author and date; no
post-commit revprop sync needed." is slightly inaccurate —
remove_props_not_in_source does still run a post-commit revprop pass to
delete leftover properties on the destination.
8. build.conf change is unmotivated by the rest of the patch
build.confLines 9-11
-libs = libsvn_test libsvn_wc libsvn_ra libsvn_ra_svn libsvn_fs
libsvn_delta libsvn_subr
+libs = libsvn_test libsvn_wc libsvn_ra libsvn_repos libsvn_ra_svn
libsvn_fs libsvn_delta libsvn_subr
apriconv apr
ra-test.c is not modified in this diff, so adding libsvn_repos to its link
line has no observable benefit here. It also looks like leftover
scaffolding — perhaps you intended to add a unit test for the new
API/capability and didn't end up writing it? If so, that gap is itself a
problem (see next item). If not, this hunk should be dropped from the
changeset.
9. No tests for the new functionality
There are no test additions in this diff for:
-
The new svn_repos_get_commit_editor6 /
svn_repos_fs_begin_txn_for_commit3 signatures (in particular, asserting
that SVN_FS_TXN_CLIENT_DATE actually preserves the caller-supplied svn:date
end-to-end through the commit editor).
-
The new RA capability being TRUE on ra_local and not crashing on the
other RA layers (which it currently will — see Issue #1).
-
The svnsync code paths under has_commit_allow_rev_props_capability.
-
Verifying that svn:author passed in the revprop table is preserved on
ra_local (i.e., the spoofing-by-design behavior from Issue #4).
For the ra_local case there's an obvious harness already:
subversion/tests/libsvn_ra/ra-test.c (which is why the build.conf change is
sensible, if a test were added). For the svnsync end-to-end behavior
there's subversion/tests/cmdline/svnsync_tests.py.
10. No CHANGES entry
A new public API plus a new RA capability plus a behavior change for
svnsync is exactly the sort of thing that should land with a CHANGES blurb
under the in-development Version 1.16.0 section. Right now there is none.
------------------------------
MINOR / NITS
-
Typo fix is correct: revplay_rev_finished → replay_rev_finished in the
existing comment. Good catch.
-
SVN_PROP_TXN_USER_AGENT / SVN_PROP_TXN_CLIENT_COMPAT_VERSION are still
set with svn_hash_sets unconditionally on the ra_local commit path, so a
caller-supplied value would be silently overwritten. This is pre-existing,
not new, but it's worth noting that the patch deliberately asymmetric: it
switched svn:author to "only if missing", but kept the other two
unconditional. The current behavior is fine (those props are
internal/ephemeral), just inconsistent.
-
Doc note in svn_repos.h for editor6 still says "Yes, repos_url_decoded
is a decoded URL. We realize that's sorta wonky." That comment was attached
to editor5; now it's attached to editor6. Fine, but consider tidying the
chained doc since you're touching it.
-
@deprecated Provided for backward compatibility with the 1.15 API is the
right wording given trunk is 1.16.0.
------------------------------
Recommended action order
If I were preparing this for commit:
1.
(Blocker) Register SVN_RA_CAPABILITY_COMMIT_ALLOW_REV_PROPS in ra_svn
and ra_serf so the capability returns FALSE instead of
SVN_ERR_UNKNOWN_CAPABILITY — at minimum add it to parse_capabilities and
the ra_svn lookup table as a "no". (Issue #1)
2.
(Blocker) Add a regression test that runs svnsync sync against svn://
and http:// mock destinations, since that's the case that the current
patch breaks.
3.
Decide whether to surface or hide the hook-skipping behavior change
(Issue #3); at minimum document it in CHANGES and the svnsync man page.
4.
Either plumb flags through svn_repos__get_commit_ev2 or scope the
capability docs to Ev1 (Issue #2).
5.
Tighten the commit-allow-rev-props branch in svnsync.c to also require
commit-revprops (Issue #5).
6.
Fix or remove the misleading comment block in replay_rev_finished (Issue
#6).
7.
Add unit tests for the new APIs and the spoofing-allowed semantics on
ra_local (Issues #4, #9).
8.
Add a CHANGES entry (Issue #10).
9.
Drop the build.conf hunk if no corresponding test is being added, or add
the test and keep it (Issue #8).
Issue #1 in particular is hard to overstate — as the patch stands today,
running svnsync sync against any non-file:// destination will abort with
SVN_ERR_UNKNOWN_CAPABILITY before doing any work, which is essentially
total breakage of the headline tool the change is supposed to improve.
On Mon, 25 May 2026 at 17:25, Jordan Peck <[email protected]> wrote:
> Hi guys, thanks for looking into this in depth. It's been a while since I
> made the change now but I can have a look into the issues you're
> discussing. I didn't run any of the tests when making the change so it's
> not too surprising.
>
> On Mon, 25 May 2026 at 17:09, C. Michael Pilato <[email protected]>
> wrote:
>
>> *ra*test linking:* Interesting... I wonder why it didn't bother my own
>> build?
>>
>> *deprecated.c:* I spotted the same inconsistency, but can't explain
>> it. (Agree on the need for a follow-up commit for relocating other
>> deprecated-destined methods.)
>>
>> *svnsync error: *Not sure why you're seeing the error. My nightly sync
>> of repositories does an https-to-file sync, too, and seems to work fine.
>> Here are the logs from Saturday morning's cron-driven run, which show the
>> new notification:
>>
>> Sat May 23 01:00:01 AM EDT 2026 Syncing repositories
>>> Syncing cmpilato.repos
>>> Destination supports commit-time author and date; no post-commit revprop
>>> sync needed.
>>> Transmitting file data .
>>> Committed revision 5206.
>>> Transmitting file data .
>>> Committed revision 5207.
>>> Transmitting file data .
>>> Committed revision 5208.
>>
>>
>> Ahhhhh, wait a second. You're doing the *opposite* direction --
>> file-to-https -- aren't you? Yeah, I didn't test that approach.
>>
>> Also, I noticed on a clean build that there remain some other places
>> where now-deprecated methods are still used.
>>
>> subversion/libsvn_repos/fs-wrap.c: In function
>>> 'svn_repos_fs_begin_txn_for_commit':
>>> subversion/libsvn_repos/fs-wrap.c:207:3: warning:
>>> 'svn_repos_fs_begin_txn_for_commit2' is deprecated
>>> [-Wdeprecated-declarations]
>>> 207 | return svn_repos_fs_begin_txn_for_commit2(txn_p, repos, rev,
>>> revprop_table,
>>> | ^~~~~~
>>> subversion/libsvn_repos/fs-wrap.c:181:1: note: declared here
>>> 181 | svn_repos_fs_begin_txn_for_commit2(svn_fs_txn_t **txn_p,
>>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>>
>> So overall, it seems more work may be needed here.
>>
>> Jordan, I don't want to "take over" your contribution -- my patch
>> revisions aimed to clean up just the kinds of administrivia that get
>> understandably overlooked. You clearly had the chops to make the initial
>> patch, so do you have the bandwidth to respond to the concerns in this
>> thread with a followup?
>>
>> -- Mike
>>
>> PS: Daniel, my God help you, 'cause if I'm your "only hope", you are, for
>> all intents and purposes, hopeless. :-)
>>
>> On Sun, May 24, 2026 at 4:57 PM Daniel Sahlberg <
>> [email protected]> wrote:
>>
>>> No need to be sorry.. I have enjoyed trying to figure this out, learnt a
>>> lot about the build system!
>>>
>>> Unfortunately I think the patch to build.conf is patching the wrong
>>> target, it seems to patch ra-test and not ra-local-test. I'm still running
>>> with the patch of build.conf in my earlier mail.
>>>
>>> One thing I still don't understand is why ra-local-test suddenly need
>>> the lib_repos library... if someone could explain it I would be happy!
>>>
>>> One more thing I don't understand: What is the purpose of deprecated.c?
>>> I thought compatibility wrappers for deprecated functions go there (and in
>>> that case, should the implementations of svn_repos_fs_begin_txn_for_commit2
>>> and svn_repos_get_commit_editor5 be moved there)? However I see we are not
>>> consistent, when svn_repos_fs_begin_txn_for_commit was deprecated in r
>>> 863946 <https://svn.apache.org/r863946>, it was kept in fs-wrap.c. If
>>> we should use deprecated.c, svn_repos_fs_begin_txn should probably move
>>> there in a separate commit...
>>>
>>> I continued to investigate the davautocheck errors and tried to svnsync
>>> to a repo on my main server (running Debian 12 based Subversion 1.14.2). I
>>> got the following error message:
>>> [[[
>>> dsg@devi-25-01:~/svn_trunk4/subversion/svnsync$ ./svnsync initialize
>>> https://svn.example.com/svn/synctest file:///home/dsg/repo/
>>> Copied properties for revision 0.
>>> dsg@devi-25-01:~/svn_trunk4/subversion/svnsync$ ./svnsync sync
>>> https://svn.example.com/svn/synctest file:///home/dsg/repo/
>>> subversion/svnsync/svnsync.c:1670,
>>> subversion/svnsync/svnsync.c:1599,
>>> subversion/libsvn_ra_serf/options.c:785:
>>> (apr_err=SVN_ERR_UNKNOWN_CAPABILITY)
>>> svnsync: E200026: Don't know anything about capability
>>> 'commit-allow-rev-props'
>>> dsg@devi-25-01:~/svn_trunk4/subversion/svnsync$
>>> ]]]
>>> I can read the error message well enough but I have absolutely know idea
>>> where to start next... I think the quote goes: "Help me C. Michael Pilato,
>>> you are my only hope" :-)
>>>
>>> (Until the error above is resolved, I'm inclined to say -1 to this
>>> patch. However I'm very much a fan of this idea so I'd love to see it
>>> worked out).
>>>
>>> Thanks
>>> Daniel
>>>
>>> PS. Full disclosure: './configure --prefix=/home/dsg/bin
>>> --with-serf=/home/dsg/bin --enable-maintainer-mode --enable-debug
>>> --enable-svnbrowse' and I'm running on a trunk build of Serf (2.0).
>>>
>>>
>>> Den sön 24 maj 2026 kl 04:23 skrev C. Michael Pilato <
>>> [email protected]>:
>>>
>>>> Okay, this patch includes the build.conf change, plus fixes some
>>>> deprecation warning caused by newly deprecated methods not getting revved,
>>>> and it passes "make check" for me.
>>>>
>>>> I know I *can* commit it outright, but I've just been away for too
>>>> long to have that kind of confidence right now. So I'm asking (along with
>>>> Jordan) for additional eyes on the work.
>>>>
>>>> -- Mike
>>>>
>>>> PS: I configured this working copy with './configure
>>>> --enable-maintainer-mode --with-lz4=internal --with-utf8proc=internal
>>>> --enable-shared', for whatever that's worth.
>>>>
>>>>
>>>> On Sat, May 23, 2026 at 10:05 PM C. Michael Pilato <
>>>> [email protected]> wrote:
>>>>
>>>>> Sorry about this. My update of the patch yesterday wasn't done with a
>>>>> working copy in hand. I was just literally editing the patch and crafting
>>>>> the log message (on a machine without a working copy, even).
>>>>>
>>>>> Now, as I sit on the original machine I initially reviewed the patch
>>>>> on, I see I made the same tweak to build.conf as well (back in February
>>>>> when I first replied). Let me clean this up a bit more and post a version
>>>>> of the patch that at least works on my machine. :facepalm:
>>>>>
>>>>> -- Mike
>>>>>
>>>>> On Sat, May 23, 2026 at 8:17 AM Daniel Sahlberg <
>>>>> [email protected]> wrote:
>>>>>
>>>>>> Thanks Mike for reviewing this.
>>>>>>
>>>>>> I couldn't apply the patch, I think this line is invalid:
>>>>>> [[[
>>>>>> o+ * of the revision via @a revprop_table.
>>>>>> ]]]
>>>>>> (Just noting for future reference)
>>>>>>
>>>>>> When building, I got a linking error
>>>>>> [[[
>>>>>> /usr/bin/x86_64-linux-gnu-ld.bfd:
>>>>>> ../../../subversion/libsvn_ra_local/.libs/libsvn_ra_local-1.so: undefined
>>>>>> reference to `svn_repos_get_commit_editor6'
>>>>>> ]]]
>>>>>>
>>>>>> I tried to figure it out and reverted the use of
>>>>>> svn_repos_get_commit_editor6 to svn_repos_get_commit_editor5 - that
>>>>>> worked.
>>>>>> The only way I could get it to work with svn_repos_get_commit_editor6 was
>>>>>> to change build.conf :
>>>>>> [[[
>>>>>> Index: build.conf
>>>>>> ===================================================================
>>>>>> --- build.conf (revision 1934529)
>>>>>> +++ build.conf (working copy)
>>>>>> @@ -1312,7 +1312,7 @@
>>>>>> path = subversion/tests/libsvn_ra_local
>>>>>> sources = ra-local-test.c
>>>>>> install = test
>>>>>> -libs = libsvn_test libsvn_wc libsvn_ra_local libsvn_ra libsvn_fs
>>>>>> libsvn_delta libsvn_subr
>>>>>> +libs = libsvn_repos libsvn_test libsvn_wc libsvn_ra_local libsvn_ra
>>>>>> libsvn_fs libsvn_delta libsvn_subr
>>>>>> apriconv apr
>>>>>>
>>>>>> #
>>>>>> ----------------------------------------------------------------------------
>>>>>> ]]]
>>>>>> Don't know if I messed up my WC in any way.
>>>>>>
>>>>>> make check run successfully.
>>>>>>
>>>>>> make davautocheck and make svnserveautocheck fails with:
>>>>>> [[[
>>>>>> FAIL: svnsync_authz_tests.py 1: verify that unreadable content is
>>>>>> not synced
>>>>>> W: Unexpected output
>>>>>> W: EXPECTED STDERR (match_all=True):
>>>>>> W: ACTUAL STDERR:
>>>>>> W: | svnsync: E200026: Don't know anything about capability
>>>>>> 'commit-allow-rev-props'
>>>>>> W: DIFF STDERR (match_all=True):
>>>>>> W: | --- EXPECTED STDERR (match_all=True)
>>>>>> W: | +++ ACTUAL STDERR
>>>>>> W: | @@ -0,0 +1 @@
>>>>>> W: | +svnsync: E200026: Don't know anything about capability
>>>>>> 'commit-allow-rev-props'
>>>>>> W: CWD: /home/dsg/svn_trunk4/subversion/tests/cmdline
>>>>>> W: EXCEPTION: SVNLineUnequal
>>>>>> ]]]
>>>>>> (and repeated for all tests)
>>>>>> Should it be resolved by updating test expectations or did I miss
>>>>>> something running the test?
>>>>>>
>>>>>> Cheers,
>>>>>> Daniel
>>>>>>
>>>>>>
>>>>>>
>>>>>> Den fre 22 maj 2026 kl 22:19 skrev C. Michael Pilato <
>>>>>> [email protected]>:
>>>>>>
>>>>>>> Okay, I've touched up the @since and @deprecated lines of the patch
>>>>>>> to point to 1.16 and 1.15, respectively. I also took a crack at
>>>>>>> writing a
>>>>>>> formal log message (which helped me also to more thoroughly review the
>>>>>>> change). I'm re-posting the patch (with inline log) here for something
>>>>>>> like safekeeping.
>>>>>>>
>>>>>>> -- Mike
>>>>>>>
>>>>>>> On Mon, Feb 2, 2026 at 6:51 PM C. Michael Pilato <
>>>>>>> [email protected]> wrote:
>>>>>>>
>>>>>>>> Actually, I did spot something that I think needs correction. The
>>>>>>>> "@since New in 1.15." annotations should say "New in 1.16", right?
>>>>>>>>
>>>>>>>> -- Mike
>>>>>>>>
>>>>>>>> On Sun, Feb 1, 2026 at 10:19 PM C. Michael Pilato <
>>>>>>>> [email protected]> wrote:
>>>>>>>>
>>>>>>>>> Today, I setup a Subversion development environment for the first
>>>>>>>>> time in ... more years than I can recall. I can confirm that the test
>>>>>>>>> suite passes with this patch, and that the patch itself makes sense
>>>>>>>>> to me.
>>>>>>>>> I also used it for my own svnsync activity (I do nightly backups of
>>>>>>>>> personal repositories), and it seems to work as advertised.
>>>>>>>>>
>>>>>>>>> Not gonna lie, I feel too "out of the game" to commit this patch
>>>>>>>>> outright. But, I *am* comfortable giving a +1 on it, for
>>>>>>>>> whatever that nuance might mean.
>>>>>>>>>
>>>>>>>>> -- Mike
>>>>>>>>>
>>>>>>>>> On Sat, Jan 31, 2026 at 2:48 PM Timofei Zhakov <[email protected]>
>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>> On Thu, Jan 29, 2026 at 10:47 PM Jordan Peck via dev <
>>>>>>>>>> [email protected]> wrote:
>>>>>>>>>>
>>>>>>>>>>> Hi all,
>>>>>>>>>>>
>>>>>>>>>>> I'd like to propose a patch that optimises svnsync when
>>>>>>>>>>> synchronizing to
>>>>>>>>>>> local filesystem repositories (file:// URLs). Currently, svnsync
>>>>>>>>>>> performs
>>>>>>>>>>> a two-step process for each revision:
>>>>>>>>>>>
>>>>>>>>>>> 1. Commit the revision content (author/date are set to the
>>>>>>>>>>> current
>>>>>>>>>>> user and current time)
>>>>>>>>>>> 2. Update svn:author and svn:date revision properties to match
>>>>>>>>>>> the
>>>>>>>>>>> source repository
>>>>>>>>>>>
>>>>>>>>>>> This works but is inefficient for file:// URLs where we have
>>>>>>>>>>> direct
>>>>>>>>>>> repository access and can set these properties correctly during
>>>>>>>>>>> the
>>>>>>>>>>> initial commit.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> THE PROBLEM
>>>>>>>>>>> -----------
>>>>>>>>>>>
>>>>>>>>>>> When svnsync replays a revision from a source repository, the
>>>>>>>>>>> commit
>>>>>>>>>>> is created with the local user as the author and the current
>>>>>>>>>>> time as
>>>>>>>>>>> the date. After the commit completes, svnsync then makes separate
>>>>>>>>>>> svn_ra_change_rev_prop2() calls to update svn:author and
>>>>>>>>>>> svn:date to
>>>>>>>>>>> match the source repository.
>>>>>>>>>>>
>>>>>>>>>>> For remote servers (svn://, http://), this two-step process is
>>>>>>>>>>> necessary
>>>>>>>>>>> because the server enforces its own author/date policies.
>>>>>>>>>>> However, for
>>>>>>>>>>> file:// URLs via ra_local, we have direct access to the
>>>>>>>>>>> repository and
>>>>>>>>>>> can bypass this limitation.
>>>>>>>>>>>
>>>>>>>>>>> We have an SVN server acting as the slave via the Apache module
>>>>>>>>>>> proxy,
>>>>>>>>>>> the slave server is kept in sync with the master using svnsync.
>>>>>>>>>>> This
>>>>>>>>>>> two-step approach has caused issues with various tools that
>>>>>>>>>>> monitor the
>>>>>>>>>>> slave SVN server for new revisions, they will occasionally get
>>>>>>>>>>> the data
>>>>>>>>>>> for a synced revision that hasn't yet had the revision
>>>>>>>>>>> properties
>>>>>>>>>>> copied. Having the sync be a single atomic operation means this
>>>>>>>>>>> can
>>>>>>>>>>> never happen.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> THE SOLUTION
>>>>>>>>>>> ------------
>>>>>>>>>>>
>>>>>>>>>>> This patch introduces a new RA capability
>>>>>>>>>>> "commit-allow-rev-props" that
>>>>>>>>>>> ra_local advertises. When svnsync detects this capability on the
>>>>>>>>>>> destination repository, it:
>>>>>>>>>>>
>>>>>>>>>>> 1. Includes svn:author and svn:date in the initial commit's
>>>>>>>>>>> revprop
>>>>>>>>>>> table (instead of filtering them out)
>>>>>>>>>>>
>>>>>>>>>>> 2. ra_local now conditionally preserves these properties:
>>>>>>>>>>> - svn:author is only set to the session user if not already
>>>>>>>>>>> provided
>>>>>>>>>>> - If svn:date is provided, the SVN_FS_TXN_CLIENT_DATE flag
>>>>>>>>>>> is passed
>>>>>>>>>>> to svn_fs_begin_txn2() so the filesystem uses that date
>>>>>>>>>>>
>>>>>>>>>>> 3. Skips the post-commit revprop update step since author/date
>>>>>>>>>>> are
>>>>>>>>>>> already correct
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> CHANGES OVERVIEW
>>>>>>>>>>> ----------------
>>>>>>>>>>>
>>>>>>>>>>> subversion/include/svn_ra.h:
>>>>>>>>>>> - Added SVN_RA_CAPABILITY_COMMIT_ALLOW_REV_PROPS capability
>>>>>>>>>>> definition
>>>>>>>>>>>
>>>>>>>>>>> subversion/include/svn_repos.h:
>>>>>>>>>>> - Added svn_repos_fs_begin_txn_for_commit3() with flags
>>>>>>>>>>> parameter
>>>>>>>>>>> - Added svn_repos_get_commit_editor6() with txn_flags parameter
>>>>>>>>>>>
>>>>>>>>>>> subversion/libsvn_repos/fs-wrap.c:
>>>>>>>>>>> - Implemented svn_repos_fs_begin_txn_for_commit3() which
>>>>>>>>>>> passes flags
>>>>>>>>>>> (including SVN_FS_TXN_CLIENT_DATE) to svn_fs_begin_txn2()
>>>>>>>>>>>
>>>>>>>>>>> subversion/libsvn_repos/commit.c:
>>>>>>>>>>> - Added txn_flags to edit_baton structure
>>>>>>>>>>> - Implemented svn_repos_get_commit_editor6() to support
>>>>>>>>>>> txn_flags
>>>>>>>>>>> - Updated open_root() to use the new begin_txn function
>>>>>>>>>>>
>>>>>>>>>>> subversion/libsvn_ra_local/ra_plugin.c:
>>>>>>>>>>> - Advertises SVN_RA_CAPABILITY_COMMIT_ALLOW_REV_PROPS
>>>>>>>>>>> - Modified get_commit_editor to preserve caller-provided
>>>>>>>>>>> author/date
>>>>>>>>>>> - Sets SVN_FS_TXN_CLIENT_DATE when date is provided in revprops
>>>>>>>>>>>
>>>>>>>>>>> subversion/svnsync/svnsync.c:
>>>>>>>>>>> - Detects the new capability on the destination
>>>>>>>>>>> - When capability is present, includes author/date in commit
>>>>>>>>>>> revprops
>>>>>>>>>>> - Skips post-commit revprop sync when capability is present
>>>>>>>>>>> - Updated user-facing messages to reflect the optimization
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> USER-VISIBLE CHANGES
>>>>>>>>>>> --------------------
>>>>>>>>>>>
>>>>>>>>>>> When synchronizing to a file:// URL, svnsync now displays:
>>>>>>>>>>>
>>>>>>>>>>> Destination supports commit-time author and date; no
>>>>>>>>>>> post-commit
>>>>>>>>>>> revprop sync needed.
>>>>>>>>>>> Committed revision 1.
>>>>>>>>>>> Committed revision 2.
>>>>>>>>>>> ...
>>>>>>>>>>>
>>>>>>>>>>> Instead of the previous:
>>>>>>>>>>>
>>>>>>>>>>> Committed revision 1.
>>>>>>>>>>> Copied properties for revision 1.
>>>>>>>>>>> Committed revision 2.
>>>>>>>>>>> Copied properties for revision 2.
>>>>>>>>>>> ...
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> BENEFITS
>>>>>>>>>>> --------
>>>>>>>>>>>
>>>>>>>>>>> 1. Atomicity: Author and date are set in the same transaction as
>>>>>>>>>>> the
>>>>>>>>>>> commit, rather than being updated afterward.
>>>>>>>>>>>
>>>>>>>>>>> 2. No behavioral change for remote servers: The optimization only
>>>>>>>>>>> applies when the destination advertises the capability, so
>>>>>>>>>>> svn://
>>>>>>>>>>> and http:// synchronization continues to work as before.
>>>>>>>>>>>
>>>>>>>>>>> 3. Reduced I/O: Eliminates separate revprop change operations
>>>>>>>>>>> for each
>>>>>>>>>>> revision synced to a file:// destination.
>>>>>>>>>>>
>>>>>>>>>>> COMPATIBILITY
>>>>>>>>>>> -------------
>>>>>>>>>>>
>>>>>>>>>>> - The new capability is only advertised by ra_local, so this is a
>>>>>>>>>>> client-side optimization with no server protocol changes.
>>>>>>>>>>>
>>>>>>>>>>> - Existing svnsync mirrors continue to work unchanged.
>>>>>>>>>>>
>>>>>>>>>>> - The new API functions (svn_repos_fs_begin_txn_for_commit3 and
>>>>>>>>>>> svn_repos_get_commit_editor6) maintain backward compatibility
>>>>>>>>>>> through wrapper functions.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> TESTING
>>>>>>>>>>> -------
>>>>>>>>>>>
>>>>>>>>>>> I have tested this with svnsync to file:// URLs and verified
>>>>>>>>>>> that:
>>>>>>>>>>> - Revision properties (author, date, log) match the source
>>>>>>>>>>> repository
>>>>>>>>>>> - The new optimization message appears at sync start
>>>>>>>>>>> - No "Copied properties" messages appear
>>>>>>>>>>> - Subsequent incremental syncs work correctly
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Please review and let me know if you have any questions or
>>>>>>>>>>> suggestions.
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Jordan Peck
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> +1
>>>>>>>>>>
>>>>>>>>>> I didn't look into the patch in detail, but the idea sounds good
>>>>>>>>>> to me.
>>>>>>>>>>
>>>>>>>>>> --
>>>>>>>>>> Timofei Zhakov
>>>>>>>>>>
>>>>>>>>>
