Re: [PATCH] Make rev_compare_tree less confusing.

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 16, 2010, 20:23 UTC
Message-ID: <7v1vefcfjx.fsf@alter.siamese.dyndns.org>
In-Reply-To: <201004161131.06880.trast@student.ethz.ch>

Thomas Rast <trast@student.ethz.ch> writes:

> Interestingly enough, this call survived with slight changes here and
> there from all the way back in cf48454 (Teach git-rev-list to follow
> just a specified set of files, 2005-10-20), where it was added in
> rev-list.c.  Even back then diff_tree() would always return 0.

The idea always have been that diff_tree() may some day start returning non-fatal errors (otherwise it calls die() so that the caller does not even have a chance to worry about the return value), and the particular codepath the patch touches would treat such "one of the trees is unreadble so we cannot say if/how different they are" case as "there may be a change worth showing (even if the actual change couldn't be shown)".

So I'm a bit reluctant to accept this patch under discussion. It doesn't change the behaviour, it doesn't make the _interface_ any simpler, and it removes the documentation value of what _should_ happen if diff_tree() were to be updated in such a way.

It is entirely a different matter if the patch were to change the function signature of diff_tree() to return "void" and adjust all the callers involved. That will make the _interface_ simpler without changing the behaviour, and it makes it absolutely clear that we would _never_ enhance git to say "some necessary data for comparison is missing---we report error and allow the caller to err on the safe side and say 'there could be difference but the details we cannot say'".