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

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

quoted

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