Re: [PATCH 1/2] repository: add core.indexLockTimeout

[Junio C Hamano <gitster@pobox.com>]

Jun Wu <quark.zju@gmail.com> writes:

> From: Jun Wu <quark@meta.com>
>
> When an IDE runs `git status` periodically in the background, `status`
> can take the index lock, and this prevents other commands like `add`,
> `commit` from running.
>
> Commit 27344d6a6c (git: add --no-optional-locks option, 2017-09-27)
> avoids the conflict by letting background `git --no-optional-locks status`
> skip the index lock, at the cost of losing refreshed stat info, so
> the next `status` has to re-hash files.
>
> Approach the conflict from the other side: let non-status commands wait
> for the index lock. Mercurial and Sapling have a similar "waiting for wlock"
> behavior, and Git already has `core.{config,filesRef}LockTimeout`.
>
> Ideally all non-status commands can wait for the index lock. However,
> some code paths (e.g. `commit`) load the index without locking, then lock and
> write changes. It is already racy and the wait behavior would make it much
> worse. As a defense, waiting is disabled if index was already read.
>
> Default timeout is 1s, effective but not too long to avoid breaking potential
> user expectation (e.g. "an index.lock error means I should check whether a
> commit message editor is open somewhere"). Tests that are slowed down by the
> default timeout now set the timeout to 0.

Need to change the existing tests is a sign that end-user workflows
that are emulated by them will be impacted, and we'd need to be
careful.

I am not sure if the change is making the right trade-off between
background tasks and end-user action that expects immediate
response.

> @@ -473,7 +473,22 @@ int repo_hold_locked_index(struct repository *repo,
>  			   struct lock_file *lf,
>  			   int flags)
>  {
> +	int timeout_ms;
> +
>  	if (!repo->index_file)
>  		BUG("the repo hasn't been setup");
> -	return repo_hold_lock_file_for_update(repo, lf, repo->index_file, flags);
> +
> +	if (repo->index->initialized) {
> +		/*
> +		 * Index was initialized (read) without lock. To minimize the
> +		 * chance of lost update, do not wait.
> +		 */
> +		timeout_ms = 0;
> +	} else {
> +		timeout_ms = 1000;
> +		repo_config_get_int(repo, "core.indexlocktimeout", &timeout_ms);
> +	}
> +
> +	return repo_hold_lock_file_for_update_timeout(repo, lf, repo->index_file,
> +						      flags, timeout_ms);
>  }

A few comments and/or questions..

 (1) In the unhappy case where the index was read already and then
     we try to take the lock, what should happen when we notice that
     the index is locked and better yet if we notice that the index
     has been updated before we write the in-core index we wanted to
     write out?  The question is what "should" happen, not "what
     happens with the current code".

 (2) Are there cases where we take lock only to make sure nobody
     will update the index without any intention to write out the
     contents of our in-core index?  IOW, we are holding the lock
     only to have a consistent world view while performing a
     read-only operation?  How would this change affect it?

 (3) When 'repo->index->initialized' is true, does it always mean we
     have read the on-disk index already, or are there cases where
     we created the in-core index from scratch and called this
     function with the intention to write over it?  For such a use
     case, what, if any, are the implications of this change?