Johannes Schindelin <Johannes.Schindelin@xxxxxx> writes: >> With the current set of callers, a caller that notices an error from >> this function will immediately exit without doing any further >> damage. >> >> So in that sense, this is a "safe" conversion. >> >> But is it a sensible conversion? When the caller wants to do >> anything else (e.g. clean-up and try something else, perhaps read >> the index again), the caller can't, as the index is still locked, >> because even though the code knows that the lock will not be >> released until the process exit, it chose to return error without >> releasing the lock. > > It depends what the caller wants to do. The case about which I care most > is when some helpful advice should be printed (see e.g. 3be18b4 (t5520: > verify that `pull --rebase` shows the helpful advice when failing, > 2016-07-26)). Those callers do not need to care, as the atexit() handler > will clean up the lock file. > > However, I am sympathetic to your angle, even if I do not expect any such > caller to arise anytime soon. We just fixed a similar "why are we allowing the 'if the_index hasn't been read, read unconditionally from $GIT_INDEX_FILE" that is reached by a codepath that is specifically designed to read from a temporary index file while reviewing a separate topic, and that is where my reaction "this is not very helpful for other callers" comes from.