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

[Junio C Hamano <gitster@pobox.com>]

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

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