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

[Jeff King <peff@peff.net>]

On Wed, Feb 28, 2018 at 08:58:09PM +0100, Martin Ågren wrote:

> > I'll follow up with a patch to
> > address the confusing pattern which Peff mentioned and which fooled me
> > when I prepared v1.
> 
> Here is such a patch on top of the others. I'm not particularly proud of the
> name SKIP_IF_UNCHANGED, but don't think it's any worse than, e.g.,
> IGNORE_UNCHANGED or NO_UNCHANGED_WRITE. :-/ Suggestions welcome.
> 
> I think this makes the current users a bit more obvious, and should help future
> users get this optimization right.

IMHO the result is easier to follow. Except for one case:

> -	if (active_cache_changed || force_write) {
> -		if (newfd < 0) {
> -			if (refresh_args.flags & REFRESH_QUIET)
> -				exit(128);
> -			unable_to_lock_die(get_index_file(), lock_error);
> -		}
> -		if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
> -			die("Unable to write new index file");
> +	if (newfd < 0 && (active_cache_changed || force_write)) {
> +		if (refresh_args.flags & REFRESH_QUIET)
> +			exit(128);
> +		unable_to_lock_die(get_index_file(), lock_error);
>  	}
>  
> -	rollback_lock_file(&lock_file);
> +	if (write_locked_index(&the_index, &lock_file,
> +			       COMMIT_LOCK | (force_write ? 0 : SKIP_IF_UNCHANGED)))
> +		die("Unable to write new index file");

where I think the logic just ends up repeating itself. I guess you were
anxious to try to get rid of active_cached_changed, but I don't think
keeping it around is really that big a deal (and certainly another trick
is to just say "the_index.cache_changed").

-Peff