Re: [PATCH v2 0/5] roll back locks in various code paths
- From
Jeff King <peff@peff.net>
- Date
- Mar 1, 2018, 07:41 UTC
- Message-ID
- <20180301074129.GE31079@sigill.intra.peff.net>
- In-Reply-To
- <20180228195809.9145-1-martin.agren@gmail.com>
On Wed, Feb 28, 2018 at 08:58:09PM +0100, Martin Ågren wrote:
Show 10 quoted lines
> > 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:
Show 18 quoted lines
> - 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