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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: May 20, 2011, 18:07 UTC
Message-ID: <20110520180745.GC17177@elie>
In-Reply-To: <1305880223-7542-1-git-send-email-artagnon@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.