Thomas Gummerer <t.gummerer@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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)
