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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 30, 2019, 17:06 UTC
Message-ID: <xmqq8srazipr.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <CAN0heSqZOG6NMJE4=RReKzG3eD_w1mh8EcYaAQWN6WBY3WuZ1Q@mail.gmail.com>

Martin Ågren <martin.agren@gmail.com> writes:

Show 6 quoted lines

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

One common reason why refresh_cache() fails is because the index is unmerged (i.e. has one or more higher-stage entries). After an attempt to refresh, this would not wrote out the index in such a case, which might even be more correct thing to do than the original in the original context of "git am" implementation. The next thing that happens after the caller calls this function is to ask repo_index_has_changes(), and we'd say "the index is dirty" whether the index is written back or not from such a state.

> 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". ;-)

Yes, that is a more severe issue.