Re: [PATCH v3 1/3] factor out refresh_and_write_cache function

[Junio C Hamano <gitster@pobox.com>]

Thomas Gummerer <t.gummerer@gmail.com> writes:

> Getting the lock for the index, refreshing it and then writing it is a
> pattern that happens more than once throughout the codebase, and isn't
> trivial to get right.  Factor out the refresh_and_write_cache function
> from builtin/am.c to read-cache.c, so it can be re-used in other
> places in a subsequent commit.
>
> Note that we return different error codes for failing to refresh the
> cache, and failing to write the index.  The current caller only cares
> about failing to write the index.  However for other callers we're
> going to convert in subsequent patches we will need this distinction.
>
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
>  builtin/am.c | 16 ++--------------
>  cache.h      | 16 ++++++++++++++++
>  read-cache.c | 19 +++++++++++++++++++
>  3 files changed, 37 insertions(+), 14 deletions(-)

I think this goes in the right direction, but obviously conflicts
with what Dscho wants to do in the builtin-add-i series, and needs
to be reconciled by working better together.

For now, I'll eject builtin-add-i and queue this for a few days to
give it a bit more exposure, but after that requeue builtin-add-i
and discard these three patches.  By that time, hopefully you two
would have a rerolled version of this one and builtin-add-i that
agree what kind of refresh-and-write-index behaviour they both want.

The differences I see that need reconciling are:

 - builtin-add-i seems to allow 'gentle' and allow returning an
   error when we cannot open the index for writing by passing false
   to 'gentle'; this feature is not used yet, though.

 - This version allows to pass pathspec, seen and header_msg, while
   the one in builtin-add-i cannot limit the part of the index
   getting refreshed with pathspec.  It wouldn't be a brain surgery
   to use this version and adjust the caller (there only is one) in
   the builtin-add-i topic.

 - This version does not write the index back when refresh_index()
   returns non-zero, but the one in builtin-add-i ignores the
   returned value.  I think, as a performance measure, it probably
   is a better idea to write it back, even when the function returns
   non-zero (the local variable's name is has_errors, but having an
   entry in the index that does not get refreshed is *not* an error;
   e.g. an unmerged entry is a normal thing in the index, and as
   long as we refreshed other entries while having an unmerged and
   unrefreshable entry, we are making progress that is worth writing
   out).

Thanks.

> +int repo_refresh_and_write_index(struct  repository *repo,
> +				 unsigned int refresh_flags,
> +				 unsigned int write_flags,
> +				 const struct pathspec *pathspec,
> +				 char *seen, const char *header_msg)
> +{
> +	struct lock_file lock_file = LOCK_INIT;
> +
> +	repo_hold_locked_index(repo, &lock_file, LOCK_DIE_ON_ERROR);
> +	if (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg)) {
> +		rollback_lock_file(&lock_file);
> +		return 1;
> +	}
> +	if (write_locked_index(repo->index, &lock_file, COMMIT_LOCK | write_flags))
> +		return -1;
> +	return 0;
> +}
> +
> +
>  int refresh_index(struct index_state *istate, unsigned int flags,
>  		  const struct pathspec *pathspec,
>  		  char *seen, const char *header_msg)