Re: [PATCH/RFD] builtin-revert.c: release index lock when cherry-picking an empty commit

On 2009 Mar 7, Johannes Schindelin wrote:

Agreed.

I am not quite sure I follow your meaning here.

We have write_cache_as_tree in cache-tree.c. Is something like that what you had in mind for "write the index to a tree"?

Could you elaborate on how an "index to tree" function could be applied to the problem of the inconsistent lock releasing around commit_locked_index calls? Sorry, for my lack of a clue, I am fairly new to the code base. Or are you seeing a different code duplication problem here?

The general form of the code around calls to commit_locked_index seems to be of the this form:

	if (some_condition) /* not all call sites use this */
		if (write_cache(...) || commit_locked_index(...))
			die(...); /* or return error(...) */
	rollback_lock_file(...); /* sometimes missing or distant */

In most cases some_condition is active_cache_changed, but not always (builtin-apply.c, rerere.c).

The problem with cherry-picking empty commits was that active_cache_changed (used as some_condition in the general pattern shown above) would be false, so the write and commit was skipped, but there was also never any rollback. Later, when cherry-pick exec-ed commit, the lock file still existed and commit dies.

To make sure a commit or a rollback always happens at every call site, each one would have to unconditionally call some global function (write_and_commit_or_rollback_locked_index?, ick) that conditionally did the write and commit, but unconditionally did the rollback (basically a no-op if the commit went OK).

Indeed, would such a cleanup be worth the churn? I do not have any modifications for which this cleanup could be considered preparatory.

Thank you for your help!