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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
