On Mon, Oct 02, 2017 at 02:15:30AM -0400, Jeff King wrote: > I'm not sure I follow here. It seems like write_locked_index() has three > outcomes: > > - if there was an error, the lock is rolled back > > - if we were successful and the caller asked for CLOSE_LOCK, we remain > locked > > - if we were successful and the caller asked for COMMIT_LOCK, we > commit the lock > > It sounds like you are considering the first outcome a bug, but I think > it is intentional. Without that, every caller of write_locked_index() > would need to release the lock themselves. That's especially cumbersome > if they called with COMMIT_LOCK, as they otherwise can assume that > write_locked_index() has released the lock one way or another. > > So I actually think that just switching to a "struct tempfile **" here > is a reasonable solution (I'd also be fine with doing this and then > having do_write_locked_index() rollback the lock itself on error). > > Or am I missing something? Well, one thing I was certainly missing was reading your patch 11. :) That fixes the COMMIT_LOCK case. We are still changing the behavior of CLOSE_LOCK on error, though. The existing callers all seem to die immediately so it wouldn't matter either way, but there could in theory be new ones in topics-in-flight. The other thing I was missing is that we are absolutely inconsistent about this "close on error". It only happens for _one_ of the error returns of do_write_index(), and the others would have left the file open and not rolled-back. So in retrospect, I think your approach is the right direction. -Peff
