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

[Thomas Gummerer <t.gummerer@gmail.com>]

On 08/30, Junio C Hamano wrote:
> 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.

Looking at the other callsites, we seem to do something similar
everywhere, and usually fail if the index has unmerged entries.  So
the refreshed index would only not be written out in the case where
there's unmerged entries, and we fail later, which I think is okay.

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

With this do you mean what you quoted above, or that the lockfile is
not rolled back?  I agree that the lockfile not being rolled back if
'refresh_cache()' fails is indeed the bigger issue, and I'll fix that
in v3.  I can also add something like the above to the commit message,
just wanted to make sure I'm not missing something subtle in what you
quoted above.