Re: [PATCH 4/4] revision: convert to using diff_tree_sha1()

From: Jeff King <peff@peff.net>
Date: Feb 5, 2014, 17:25 UTC
Message-ID: <20140205172511.GA7268@sigill.intra.peff.net>
In-Reply-To: <975fbde9bdd2c5aad7376e398ca8001b9a41d2d6.1391619218.git.kirr@mns.spb.ru>

On Wed, Feb 05, 2014 at 08:57:12PM +0400, Kirill Smelkov wrote:

Show 12 quoted lines

> Since diff_tree_sha1() can now accept empty trees via NULL sha1, we
> could just call it without manually reading trees into tree_desc and
> duplicating code.
> 
> Besides, that
> 
> 	if (!tree)
> 		return 0;
> 
> looked suspect - we were saying an invalid tree != empty tree, but maybe it is
> better to just say the tree is invalid here, which is what diff_tree_sha1()
> does for such case.

I think that is sensible. The assertion that "invalid != empty" is probably sane, because we handle the empty tree as internal magic. But I do not see any reason we should be hitting this code path regularly with an invalid tree, short of repository corruption, so in practice I don't think it matters.

This does introduce a die() where there was not one previously, and that can make things harder to diagnose/debug in a corrupted repository. But it looks like this is limited to the history-simplification code, and I suspect that it is not commonly used in the case of corruption.

So I think the patch looks fine.

-Peff