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

[Junio C Hamano <gitster@pobox.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:

>  	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.

> @@ -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.