Hi,
Ramkumar Ramachandra wrote:
> If the read_cache() call succeeds, the function must call
> discard_cache() before returning to the caller.
>
> Suggested-by: Jonathan Nieder <jrnieder@xxxxxxxxx>
Sorry, I was unclear. What I meant is this: currently all three
callers to write_cache_as_tree() call die() or exit() if it fails.
When a new caller wants to carry on after failure, that requires some
careful thinking about what the state should be when it resumes.
Toward that end, your patch "[PATCH 1/8] revert: Improve error
handling by..." included
> - if (write_cache_as_tree(head, 0, NULL))
> - die (_("Your index file is unmerged."));
> + if (write_cache_as_tree(head, 0, NULL)) {
> + discard_cache();
but it doesn't feel right. Why after trying to write-tree do we undo
the _reading_ of a tree? It seemed strange.
So let's think carefully about what the state should be after
write-tree fails. Does it discard_cache() to make valgrind happier?
Or do we keep the in-core index cached for use in later operations?
Whichever is the right choice would be likely to apply equally well to
"git cherry-pick" and other write_cache_as_tree callers.
I foolishly did not ask "do you really want to discard_cache() here?"
and instead said something to the effect of "if you are going to
discard_cache(), won't that apply equally to all callers?", while the
former is a better question. Of course this would all be easier if we
had an example to think about that cared about the index state on
error one way or another.
If I understand correctly, the sequencer does not care about the state
at all, and just wants to write a few files under .git and print a
message. It could do that just as well by keeping the die() and
setting up a die handler.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html