Re: [PATCH v2 0/5] roll back locks in various code paths

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 28, 2018, 23:20 UTC
Message-ID: <xmqqr2p4itvh.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180228195809.9145-1-martin.agren@gmail.com>

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

> A further upshot of this patch is that `active_cache_changed`, which is
> defined as `the_index.cache_changed`, now only has a few users left.

I am undecided if this is a *good* thing. In a few codepaths where we make a speculative update to the on-disk index file, I find that the explicit mention of active_cache_changed clarifies what is going on. Perhaps once my (and other old timers') eyes get used to this new SKIP_IF_UNCHANGED symbol, it would start serving the same purpose ;-)

> We could have made the new flag behave the other way round
> ("FORCE_WRITE"), but that could break existing users behind their backs.

Yes, that would break topics in flight that add new calls to write_locked_index(); they want to write out the index even when active_cache_changed says there is no update.

Looking at a typical pre/post image of this change like this place:

Show 8 quoted lines

>  	hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
>  	refresh_cache(REFRESH_QUIET);
> -	if (active_cache_changed &&
> -	    write_locked_index(&the_index, &lock, COMMIT_LOCK))
> +	if (write_locked_index(&the_index, &lock,
> +			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
>  		return error(_("Unable to write index."));
> -	rollback_lock_file(&lock);

this certainly simplifies what the caller needs to do.

Show 9 quoted lines

> @@ -638,6 +639,9 @@ extern int read_index_unmerged(struct index_state *);
>   * With `COMMIT_LOCK`, the lock is always committed or rolled back.
>   * Without it, the lock is closed, but neither committed nor rolled
>   * back.
> + *
> + * If `SKIP_IF_UNCHANGED` is given and the index is unchanged, nothing
> + * is written (and the lock is rolled back if `COMMIT_LOCK` is given).
>   */
>  extern int write_locked_index(struct index_state *, struct lock_file *lock, unsigned flags);

Good.