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

[Michael Haggerty <mhagger@alum.mit.edu>]

On 09/26/2014 09:00 PM, Junio C Hamano wrote:
> 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