From: Chris Johnsen Date: Sat, 07 Mar 2009 22:57:40 GMT Subject: Re: [PATCH/RFD] builtin-revert.c: release index lock when cherry-picking an empty commit Message-ID: In-Reply-To: On 2009 Mar 7, Johannes Schindelin wrote: > I wonder, though, if the real root of the problem is that there is > copied code. Agreed. > IOW I think it would be better to introduce a global > function that writes the index to a tree. 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). > A quick "git grep commit_locked_index" reveals quite a few code > sites... 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! -- Chris