(I wasn't going to comment on this since I don't know this part of the code too well, but given the crickets…)
So, the undesired behaviour is this: > This commit failed unexpectedly: > Traceback (most recent call last): > File > "/home/julianfoad/tmp/svn/wd-nv-7983/test-release-proto-rev-lock-7/test-commits.py", > line 59, in <module> > fs.commit_txn(txn2) > File > "/home/julianfoad/build/subversion-c/subversion/bindings/swig/python/libsvn/fs.py", > line 324, in svn_fs_commit_txn > return _fs.svn_fs_commit_txn(*args) > svn.core.SubversionException: > at /home/julianfoad/src/subversion-c/subversion/libsvn_fs/fs-loader.c:885 > at /home/julianfoad/src/subversion-c/subversion/libsvn_fs_fs/tree.c:2358 > at > /home/julianfoad/src/subversion-c/subversion/libsvn_fs_fs/transaction.c:4010 > at /home/julianfoad/src/subversion-c/subversion/libsvn_fs_fs/fs_fs.c:368 > at /home/julianfoad/src/subversion-c/subversion/libsvn_fs_fs/fs_fs.c:240 > at /home/julianfoad/src/subversion-c/subversion/libsvn_fs_fs/fs_fs.c:228 > at > /home/julianfoad/src/subversion-c/subversion/libsvn_fs_fs/transaction.c:3813 > at > /home/julianfoad/src/subversion-c/subversion/libsvn_fs_fs/transaction.c:475 > 2 - Can't open file > '/home/julianfoad/tmp/svn/wd-nv-7983/repo/db/txn-protorevs/1-a.rev': No such > file or directory > at /home/julianfoad/src/subversion-c/subversion/libsvn_subr/io.c:3931 > + echo 'FAILED: sub-test 9' > FAILED: sub-test 9 First of all, you said the error message read "already locked". The error above is ENOENT. Can you clarify this? The change proposed is this: Julian Foad wrote on Wed, 10 Jun 2020 17:10 +0100: > @@ -3881,30 +3898,32 @@ commit_body(void *baton, apr_pool_t *poo > /* Move the finished rev file into place. > > ### This "breaks" the transaction by removing the protorev file > ### but the revision is not yet complete. If this commit does > ### not complete for any reason the transaction will be lost. */ > old_rev_filename = svn_fs_fs__path_rev_absolute(cb->fs, old_rev, pool); > rev_filename = svn_fs_fs__path_rev(cb->fs, new_rev, pool); > proto_filename = svn_fs_fs__path_txn_proto_rev(cb->fs, txn_id, pool); > - SVN_ERR(svn_fs_fs__move_into_place(proto_filename, rev_filename, > + ERR_PROTO_REV_LOCKED(svn_fs_fs__move_into_place(proto_filename, > rev_filename, > old_rev_filename, ffd->flush_to_disk, > pool)); > + TEST_COMMIT_FAIL(9); > > /* Now that we've moved the prototype revision file out of the way, > we can unlock it (since further attempts to write to the file > will fail as it no longer exists). We must do this so that we can > remove the transaction directory later. */ > SVN_ERR(unlock_proto_rev(cb->fs, txn_id, proto_file_lockcookie, pool)); > @@ -3939,12 +3958,20 @@ commit_body(void *baton, apr_pool_t *poo > SVN_ERR(promote_cached_directories(cb->fs, directory_ids, pool)); > > /* Remove this transaction directory. */ > SVN_ERR(svn_fs_fs__purge_txn(cb->fs, cb->txn->id, pool)); > > return SVN_NO_ERROR; > + > + > +on_err_proto_rev_locked: > + /* If any error occurred while the proto-rev file was writable (locked), > + * unlock it before returning to clean up as best we can. */ > + err1 = svn_error_compose_create(err1, > + unlock_proto_rev(cb->fs, txn_id, proto_file_lockcookie, pool)); > + return err1; > } > Why is it correct to call unlock_proto_rev() without checking the error code svn_fs_fs__move_into_place() returned? HTH, Daniel P.S. Suggest to pass the name of the local variable ERR1 as a parameter to the macro to avoid action at a distance.
