Re: [PATCH 2/5] sequencer: always roll back lock in `do_recursive_merge()`

From: Jeff King <peff@peff.net>
Date: Feb 27, 2018, 22:15 UTC
Message-ID: <20180227221547.GD11187@sigill.intra.peff.net>
In-Reply-To: <CAN0heSr_4Sd5-yAUQ-SG+=NZrwFrAFqY66aYk0DgMEZ_s53iGQ@mail.gmail.com>

On Tue, Feb 27, 2018 at 11:08:12PM +0100, Martin Ågren wrote:

Show 13 quoted lines

> > So I think it's correct as-is, but I wonder if writing it as:
> >
> >   if (!active_cache_changed)
> >         rollback_lock_file(&index_lock);
> >   else if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
> >         return error(...);
> >
> > might be easier to follow. I'm OK with leaving it, too, but thought I'd
> > mention it in case it confused other reviewers.
> 
> I also hesitated at that one. There are some similar instances elsewhere, e.g.,
> in builtin/merge.c. There's also rerere.c, which does a variant of your
> suggestion.

Hmm, yeah, grepping shows quite a few of various forms.

I wonder if it is worth a helper like:

  /* like write_locked_index(), but optimize out unchanged writes */
  static int maybe_write_locked_index(struct index *index,
                                      struct lock_file *lock,
				      unsigned flags)
  {
	if (!index->cached_changed) {
		if (flags & COMMIT_LOCK)
			rollback_lock_file(lock);
		return 0;
	}
	return write_locked_index(index, lock, flags);
  }

Alternatively, it could just be a flag to write_locked_index() to enable the optimization.

I actually suspect that most callers would prefer to have it kick in by default (with an optional flag to disable it if some caller really needs to), but that would possibly be a subtle breakage for the caller that needs the flag.

-Peff