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

From: Martin Ågren <martin.agren@gmail.com>
Date: Aug 30, 2019, 15:07 UTC
Message-ID: <CAN0heSqZOG6NMJE4=RReKzG3eD_w1mh8EcYaAQWN6WBY3WuZ1Q@mail.gmail.com>
In-Reply-To: <20190829182748.43802-2-t.gummerer@gmail.com>

On Thu, 29 Aug 2019 at 20:28, Thomas Gummerer <t.gummerer@gmail.com> wrote:

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, COMMIT_LOCK | write_flags))
> +               return -1;
> +       return 0;
> +}

AFAIU, the_repository->index == &the_index so this patch is a noop on the converted user as far as that aspect is concerned.

There's a difference in behavior that I'm not sure about: We used to ignore the return value of `refresh_cache()`, i.e. we didn't care whether it had any errors. I have no idea whether that's safe to do -- especially as we go on to write the index. So I don't know whether this patch fixes a bug by introducing the early return. Or if it *introduces* a bug by bailing too aggressively. Do you know more?

(This conversion provides REFRESH_QUIET, which seems to suppress certain errors, but not all.)

In any case, that early return introduces a bug with the lockfile, that much I know. We need to roll back the lockfile before doing the early return. I should have seen that already in your previous version.. :-(

The above makes me think that once this new function is in good shape, the commit introducing it could sell it as "this is hard to get right -- let's implement it correctly once and for all". ;-)

Martin