Re: [RFC PATCH] cache-tree: Teach write_cache_as_tree to discard_cache

[Jonathan Nieder <jrnieder@gmail.com>]

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@gmail.com>

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.