On 04/14/2014 08:29 PM, Ronnie Sahlberg wrote: > refs.c:ref_transaction_commit() intermingles doing updates and checks with > actually applying changes to the refs in loops that abort on error. > This is done one ref at a time and means that if an error is detected that > will fail the operation partway through the list of refs to update we > will end up with some changes applied to disk and others not. > > Without having transaction support from the filesystem, it is hard to > make an update that involves multiple refs to guarantee atomicity, but we > can do a somewhat better than we currently do.
It took me a moment to understand what you were talking about here, because the code for ref_transaction_commit() already seems superficially to do reference modifications in phases. The problem is that write_ref_sha1() internally contains additional checks that can fail in "normal" circumstances. So the most important part of this patch series is allowing those checks to be done before committing anything. > These patches change the update and delete functions to use a three > call pattern of > > 1, lock > 2, update, or flag for deletion > 3, apply on disk (rename() or unlink()) > > When a transaction is commited we first do all the locking, preparations > and most of the error checking before we actually start applying any changes > to the filesystem store. > > This means that more of the error cases that will fail the commit > will trigger before we start doing any changes to the actual files. > > > This should make the changes of refs in refs_transaction_commit slightly > more atomic. > [...] Yes, this is a good and important goal. I wonder, however, whether your approach of changing callers from lock = lock_ref_sha1_basic() (or varient of) write_ref_sha1(lock) to lock = lock_ref_sha1_basic() (or varient of) write_ref_sha1(lock) unlock_ref(lock) | commit_ref_lock(lock) is not doing work that we will soon need to rework. Would it be jumping the gun to change the callers to transaction = ref_transaction_begin(); ref_transaction_{update,delete,etc}(transaction, ...); ref_transaction_{commit,rollback}(transaction, ...); instead? Then we could bury the details of calling write_ref_sha1() and commit_lock_ref() inside ref_transaction_commit() rather than having to expose them in the public API. I suspect that the answer is "no, ref transactions are not yet powerful enough to do everything that the callers need". But then I would suggest that we *make* them powerful enough and *then* make the change at the callers. I'm not saying that we shouldn't accept your change as a first step [1] and do the next step later, but wanted to get your reaction about making the first step a bit more ambitious. Michael [1] Though I still need to review your patch series in detail. -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html