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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 6, 2017, 01:39 UTC
Message-ID: <xmqqinftyryw.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <db82bb7dc0828a415e48e654031b3b0d0ed763a5.1507228170.git.martin.agren@gmail.com>

Martin Ågren <martin.agren@gmail.com> writes:

Show 21 quoted lines

> 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 ;-)