Re: [PATCH v2 10/12] read-cache: drop explicit `CLOSE_LOCK`-flag

From: Martin Ågren <martin.agren@gmail.com>
Date: Oct 6, 2017, 11:02 UTC
Message-ID: <CAN0heSrB4e4tNcuydztXHB9zg=y8poVSPLk_xNZssdGW3zYcxQ@mail.gmail.com>
In-Reply-To: <xmqqinftyryw.fsf@gitster.mtv.corp.google.com>

On 6 October 2017 at 03:39, Junio C Hamano <gitster@pobox.com> wrote:

Show 36 quoted lines

> Martin Ågren <martin.agren@gmail.com> writes:
>
>> diff --git a/read-cache.c b/read-cache.c
>> index 65f4fe837..1c917eba9 100644
>> --- a/read-cache.c
>> +++ b/read-cache.c
>> @@ -2343,14 +2343,13 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l
>>       int ret = do_write_index(istate, lock->tempfile, 0);
>>       if (ret)
>>               return ret;
>> -     assert((flags & (COMMIT_LOCK | CLOSE_LOCK)) !=
>> -            (COMMIT_LOCK | CLOSE_LOCK));
>>       if (flags & COMMIT_LOCK)
>>               return commit_locked_index(lock);
>> -     else if (flags & CLOSE_LOCK)
>> -             return close_lock_file_gently(lock);
>> -     else
>> -             return ret;
>> +     /*
>> +      * The lockfile already happens to have
>> +      * been closed, but let's be specific.
>> +      */
>> +     return close_lock_file_gently(lock);
>
> "already happens to have been" is quite a mouthful, and is not quite
> truthful, as we do not foresee ever wanting to change that (because
> of that stat(2) issue you mentioned).  It might be better to declare
> that do_write_index() closes the lockfile after successfully writing
> the data out to it.  I dunno if that reasoning is strong enough to
> remove this (extra) close, though.
>
> When any of the ce_write() calls in do_write_index() fails, the
> function returns -1 without hitting the close/stat (obviously).
> Somebody very high in the callchain (e.g. write_locked_index())
> would clean it up by calling rollback_lock_file() eventually, so
> that would not be a problem ;-)

When I wrote this, I was too stuck in the "it gets closed accidentally" world view. It would indeed be cleaner to specify that the close happens in `do_write_index()`. As you say, because of the stat-ing, we simply have to close.

It's still an implementation detail that closing the temporary file is the same as closing the lock. We might want to refactor to hand over the lock instead of its tempfile. Except the other caller has no suitable lock, only a temporary file. I guess that caller could use a lock instead, but it feels like the wrong solution to the wrong problem.

I'm sure that something could be done here to improve the cleanliness. For this series, I think I'll document better that `do_write_index()` closes the temporary file on success, that this might mean that it actually closes a *lock*file, but that the latter should not be relied upon. I'll get to this later today.

Thanks.

Martin