Re: [PATCH v6 27/39] try_merge_strategy(): use a statically-allocated lock_file object

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Sep 30, 2014, 14:04 UTC
Message-ID: <542AB853.5000303@alum.mit.edu>
In-Reply-To: <xmqqh9zu8ax5.fsf@gitster.dls.corp.google.com>

On 09/26/2014 09:00 PM, Junio C Hamano wrote:

Show 26 quoted lines

> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> Even the one lockfile object needn't be allocated each time the
>> function is called.  Instead, define one statically-allocated
>> lock_file object and reuse it for every call.
>>
>> Suggested-by: Jeff King <peff@peff.net>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
>> ...
>> -	hold_locked_index(lock, 1);
>> +	hold_locked_index(&lock, 1);
>>  	refresh_cache(REFRESH_QUIET);
>>  	if (active_cache_changed &&
>> -	    write_locked_index(&the_index, lock, COMMIT_LOCK))
>> +	    write_locked_index(&the_index, &lock, COMMIT_LOCK))
> 
> I wondered if the next step would be to lose the "lock" parameter
> from {hold,write}_locked_index() and have them work on a
> process-global lock, but that would not work well.
> 
> The reason why this patch works is because we are only working with
> a single destination (i.e. $GIT_INDEX_FILE typically .git/index),
> right?
> 
> Interesting.

Ummm, this patch wasn't supposed to be interesting. If it is then maybe I made a mistake...

My reasoning was that after the lock is acquired, it is released unconditionally before the function exits. Therefore, it should be no problem to reuse it each time the function is called.

Of course, one gap in this argument is the possibility that this function calls itself recursively. The fact that it calls merge_recursive() should have alerted me to this possibility. So let me check...

* try_merge_strategy() is only called by cmd_merge()
* cmd_merge() is only called by the command dispatcher

So I don't see a way for this function to call itself recursively within a single process.

A second possible gap in this argument would be if this function can be called from multiple threads. But hardly any of our code is thread-safe, so I hardly think that is likely.

What am I missing?

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu