Hi all, This is now in a branch (branches/svnsync-atomic-author) After reviewing the patch above again I made a few more changes before I committed. Mainly hooking up the new SVN_RA_SVN_CAP_COMMIT_PRESERVES_AUTHOR_DATE into the capabilities tables, instead of the having an edge case if branch for it. This also means that in the future other protocols outside file:// could implement this if needed without needing a client update.
The only thing I'm unsure about is the "Destination supports commit-time author and date; no post-commit revprop sync needed." message I added at the start of an svnsync to a file:// path. My rational for adding it was that before this feature the svnsync output would look like this: Committed revision 1. Copied properties for revision 1. Committed revision 2. Copied properties for revision 2. Now there is no "Copied properties..." lines I wanted a user facing explanation if they were concerned those lines no longer appeared in the output. But I'm not attached to this at all, if you think the new message is unnecessary, I'm happy to remove it. Thanks, Jordan On Fri, 29 May 2026 at 16:10, Jordan Peck <[email protected]> wrote: > > Hi, first off sorry for sending the massive wall of text in the last email, I > realise now I should've attached the Claude summary as its own file. > > I've had a go at polishing up this patch further, fixing the issues Claude > found and performing some more general code cleanup. I've also added tests to > cover the new feature. > > The patch should hopefully be in a more complete state now, let me know what > you think. > > Thanks, > Jordan > > On Wed, 27 May 2026 at 22:42, Daniel Sahlberg <[email protected]> > wrote: >> >> Hi, >> >> I'm really impressed by the summary! Without looking into exactly every >> detail, I think the action list seems sound. >> >> I couldn't help myself trying to investigate the failures when running >> against a remove DAV- or svnserve-server and what I came up with is very >> similar to what Claude suggest for #1, it should also remove the need for #2: >> [[[ >> Index: subversion/include/svn_ra_svn.h >> =================================================================== >> --- subversion/include/svn_ra_svn.h (revision 1934566) >> +++ subversion/include/svn_ra_svn.h (working copy) >> @@ -71,6 +71,8 @@ >> #define SVN_RA_SVN_CAP_GET_FILE_REVS_REVERSE "file-revs-reverse" >> /* maps to SVN_RA_CAPABILITY_LIST */ >> #define SVN_RA_SVN_CAP_LIST "list" >> +/* maps to SVN_RA_CAPABILITY_COMMIT_ALLOW_REV_PROPS */ >> +#define SVN_RA_SVN_CAP_COMMIT_ALLOW_REV_PROPS "commit-allow-rev-props" >> >> >> /** ra_svn passes @c svn_dirent_t fields over the wire as a list of >> Index: subversion/libsvn_ra_serf/options.c >> =================================================================== >> --- subversion/libsvn_ra_serf/options.c (revision 1934566) >> +++ subversion/libsvn_ra_serf/options.c (working copy) >> @@ -396,6 +396,8 @@ >> capability_no); >> svn_hash_sets(session->capabilities, SVN_RA_CAPABILITY_LIST, >> capability_no); >> + svn_hash_sets(session->capabilities, >> SVN_RA_CAPABILITY_COMMIT_ALLOW_REV_PROPS, >> + capability_no); >> >> /* Then see which ones we can discover. */ >> serf_bucket_headers_do(hdrs, capabilities_headers_iterator_callback, >> Index: subversion/libsvn_ra_svn/client.c >> =================================================================== >> --- subversion/libsvn_ra_svn/client.c (revision 1934566) >> +++ subversion/libsvn_ra_svn/client.c (working copy) >> @@ -3079,6 +3079,8 @@ >> {SVN_RA_CAPABILITY_GET_FILE_REVS_REVERSE, >> >> SVN_RA_SVN_CAP_GET_FILE_REVS_REVERSE}, >> {SVN_RA_CAPABILITY_LIST, SVN_RA_SVN_CAP_LIST}, >> + {SVN_RA_CAPABILITY_COMMIT_ALLOW_REV_PROPS, >> + >> SVN_RA_SVN_CAP_COMMIT_ALLOW_REV_PROPS}, >> >> {NULL, NULL} /* End of list marker */ >> }; >> ]]] >> >> I'm not sure SVN_DAV_NS_DAV_SVN_ is needed, unless we also support this over >> DAV (which I presume we wouldn't - as Claude rightly comment any user on >> ra_local could override a pre-revprop-change but on http/https and svn:// >> this is a valid security boundary). I would probably be OK just documenting >> this new behaviour. >> >> The other comments look valid but I didn't dig into them. It would be great >> if you could have a look! >> >> Thanks for your time preparing this! >> >> Cheers, >> Daniel >> >> Den tis 26 maj 2026 kl 00:38 skrev Jordan Peck via dev >> <[email protected]>: >>> >>> 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: >>> >>> 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) >>> >>> Adding SVN_RA_CAPABILITY_COMMIT_ALLOW_REV_PROPS to svn_ra.h. >>> >>> Wiring SVN_FS_TXN_CLIENT_DATE through the commit editor in libsvn_repos. >>> >>> 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. >>> >>> Teaching svnsync to check the capability and, when present, push >>> author/date in the commit and skip the post-commit revprop sync for them. >>> >>> 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 ISSUES >>> >>> 1. 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 ISSUES >>> >>> 3. 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 ISSUES >>> >>> 6. 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: >>> >>> (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) >>> >>> (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. >>> >>> Decide whether to surface or hide the hook-skipping behavior change (Issue >>> #3); at minimum document it in CHANGES and the svnsync man page. >>> >>> Either plumb flags through svn_repos__get_commit_ev2 or scope the >>> capability docs to Ev1 (Issue #2). >>> >>> Tighten the commit-allow-rev-props branch in svnsync.c to also require >>> commit-revprops (Issue #5). >>> >>> Fix or remove the misleading comment block in replay_rev_finished (Issue >>> #6). >>> >>> Add unit tests for the new APIs and the spoofing-allowed semantics on >>> ra_local (Issues #4, #9). >>> >>> Add a CHANGES entry (Issue #10). >>> >>> 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 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
