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

[Martin Ågren]

On 6 October 2017 at 03:39, Junio C Hamano  wrote:
> Martin Ågren  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

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

[Junio C Hamano]

Martin Ågren  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 ;-)