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

From: Martin Ågren <martin.agren@gmail.com>
Date: Aug 28, 2019, 15:49 UTC
Message-ID: <CAN0heSptSEa6tcRZ3DVZjr7L=A2n7=U9fbnfYOvW0bBJ-M3WKQ@mail.gmail.com>
In-Reply-To: <20190827101408.76757-2-t.gummerer@gmail.com>

On Tue, 27 Aug 2019 at 12:14, Thomas Gummerer <t.gummerer@gmail.com> wrote:

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

Show 6 quoted lines

> +/*
> + * Refresh the index and write it to disk.
> + *
> + * Return 1 if refreshing the cache failed, -1 if writing the cache to
> + * disk failed, 0 on success.
> + */

Thank you for documenting. :-) Should we say something about how this doesn't explicitly print any error in case refreshing fails (that is, we leave it to `refresh_index()`), but that we *do* explicitly print an error if writing the index fails? That caught me off-guard as I looked at how you convert the callers.

And do we actually want that asymmetry? Maybe we do.

Might be worth pointing out as you convert the callers how some (all?) of them now emit different error messages from before, but that it shouldn't matter(?) and it makes sense to unify those messages.

> +int repo_refresh_and_write_index(struct repository*, unsigned int refresh_flags, unsigned int write_flags, const struct pathspec *, char *seen, const char *header_msg);

Show 15 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))
> +               return 1;
> +       if (write_locked_index(repo->index, &lock_file, write_flags))
> +               return error(_("unable to write index file"));
> +       return 0;
> +}

If `flags` doesn't contain `COMMIT_LOCK`, the lockfile will be closed "gently", meaning we still need to either commit it, or roll it back. Or let the exit handler roll it back, which is what would happen here, no? We lose our handle on the stack and there's no way for anyone to say "ok, now I'm done, commit it please" (or "roll it back").

In short, I think calling this function without providing `COMMIT_LOCK` would be useless at best. We should probably let this function provide `COMMIT_LOCK | write_flags` or `COMMIT_LOCK | extra_write_flags` or whatever. Most callers would just provide "0". Hm?

Or, we could BUG if the COMMIT_LOCK bit isn't set, but that seems like a less good choice to me. If we're so adamant about the bit being set -- which we should be, IMHO -- we might as well set it ourselves.

Martin