From: Junio C Hamano <gitster@pobox.com>
Date: Wed, 02 Sep 2015 19:43:49 GMT
Subject: Re: [PATCH] rerere: release lockfile in non-writing functions
Message-ID: <xmqqpp20mwsq.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20150901221409.GA23274@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> There's a bug in builtin/am.c in which we take a lock on
> MERGE_RR recursively. But rather than fix am.c, this patch
> fixes the confusing interface from rerere.c that caused the
> bug. Read on for the gory details.
>
> The setup_rerere() function both reads the existing MERGE_RR
> file, and takes MERGE_RR.lock. In the rerere() and
> rerere_forget() functions, we end up in write_rr(), which
> will then commit the lock file.
>
> But for functions like rerere_clear() that do not write to
> MERGE_RR, we expect the caller to have handled
> setup_rerere(). That caller would then need to release the
> lockfile, but it can't; the lock struct is local to
> rerere.c.
>
> For builtin/rerere.c, this is OK. We run a single rerere
> operation and then exit immediately, which has the side
> effect of rolling back the lockfile.
>
> But in builtin/am.c, this is actively wrong. If we run "git
> am -3 --skip", we call setup-rerere twice without releasing
> the lock:
>
>   1. The "--skip" causes us to call am_rerere_clear(), which
>      calls setup_rerere(), but never drops the lock.
>
>   2. We then proceed to the next patch.
>
>   3. The "--3way" may cause us to call rerere() to handle
>      conflicts in that patch, but we are already holding the
>      lock. The lockfile code dies with:
>
>      BUG: prepare_tempfile_object called for active object

Thanks.  I've been re-acquainting with the internals of rerere and
agree with your approach to this issue.

Let's merge this before -rc1.