On 11 November 2017 at 00:13, Joel Teichroeb [off-list ref] wrote:

If the merge does not have anything to do, it does not unlock the index,
causing any further index operations to fail. Thus, always unlock the index
regardless of outcome.

        if (clean < 0)
                return clean;

Do we need to roll back the lock also if `clean` is negative? The
current callers are built-ins which will error out, but future callers
might be caught off guard by this.

-       if (active_cache_changed &&
-           write_locked_index(&the_index, &lock, COMMIT_LOCK))
-               return err(o, _("Unable to write index."));
+       if (active_cache_changed) {
+               if (write_locked_index(&the_index, &lock, COMMIT_LOCK))
+                       return err(o, _("Unable to write index."));
+       } else {
+               rollback_lock_file(&lock);
+       }

        return clean ? 0 : 1;
 }

Looks correct. A simpler change which would still match the commit
message would be to unconditionally call `rollback_lock_file()` just
before returning. That would perhaps be slightly more future-proof,
since it will always leave the lock unlocked, even if the if-else grows
more complicated.

Well, "always" modulo returning early and forgetting to roll back the
lock. ;-) Looking at existing code, it's not obvious which way we should
prefer. Just a thought.

Thanks for spotting this. I was poking around here recently, but failed
to notice this lax lock-handling.

Martin