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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 5, 2019, 22:00 UTC
Message-ID: <xmqqwoemo131.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190903191041.10470-2-t.gummerer@gmail.com>

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

Show 17 quoted lines

> 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.

Show 22 quoted lines

> +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)