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
