Thanks for the response, Daniel. I'm away and it will be a few days before I can check and respond completely.
(Off the top of my head, did sub tests 1-8 fail with "already locked" and 9 is the odd one out?) 30 Jul 2020 00:00:32 Daniel Shahaf <d...@daniel.shahaf.name>: > (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. >
