# [PATCH] Make rev_compare_tree less confusing. 3 messages from 2010-04-15 to 2010-04-16. Participants: Bo Yang, Thomas Rast, Junio C Hamano. Thread: https://gitlist.dev/t/23471 ## Bo Yang, 2010-04-15 08:46 Subject: [PATCH] Make rev_compare_tree less confusing. Message-ID: <1271321171-12176-1-git-send-email-struggleyb.nku@gmail.com> URL: https://gitlist.dev/e/1271321171-12176-1-git-send-email-struggleyb.nku%40gmail.com ``` diff_tree_sha1 always return 0, so comparing the return value of it make no sense. Just delete the comparison to make code reader clear. Signed-off-by: Bo Yang --- revision.c | 4 +--- 1 files changed, 1 insertions(+), 3 deletions(-) diff --git a/revision.c b/revision.c index f4b8b38..8caca99 100644 --- a/revision.c +++ b/revision.c @@ -329,9 +329,7 @@ static int rev_compare_tree(struct rev_info *revs, struct commit *parent, struct tree_difference = REV_TREE_SAME; DIFF_OPT_CLR(&revs->pruning, HAS_CHANGES); - if (diff_tree_sha1(t1->object.sha1, t2->object.sha1, "", - &revs->pruning) < 0) - return REV_TREE_DIFFERENT; + diff_tree_sha1(t1->object.sha1, t2->object.sha1, "", &revs->pruning); return tree_difference; } -- 1.6.0.4 ``` ## Thomas Rast, 2010-04-16 09:31 Subject: Re: [PATCH] Make rev_compare_tree less confusing. Message-ID: <201004161131.06880.trast@student.ethz.ch> URL: https://gitlist.dev/e/201004161131.06880.trast%40student.ethz.ch In-Reply-To: <1271321171-12176-1-git-send-email-struggleyb.nku@gmail.com> ``` Bo Yang wrote: > - if (diff_tree_sha1(t1->object.sha1, t2->object.sha1, "", > - &revs->pruning) < 0) > - return REV_TREE_DIFFERENT; > + diff_tree_sha1(t1->object.sha1, t2->object.sha1, "", &revs->pruning); Ack on the patch contents (though we could also make the function 'void' to reduce further confusion), but I'd word the commit message differently: > Make rev_compare_tree less confusing. > > diff_tree_sha1 always return 0, so comparing the return value > of it make no sense. Just delete the comparison to make code > reader clear. Something like rev_compare_tree: do not check return value of diff_tree_sha1 diff_tree_sha1() unconditionally inherits its return value from diff_tree(), which always returns 0. Hence, pretending that its return value carries any information about the tree difference is extremely misleading. The point of the call is in the side effects on revs->pruning, so simply drop the dead 'return'. 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. -- Thomas Rast trast@{inf,student}.ethz.ch ``` ## Junio C Hamano, 2010-04-16 20:23 Subject: Re: [PATCH] Make rev_compare_tree less confusing. Message-ID: <7v1vefcfjx.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v1vefcfjx.fsf%40alter.siamese.dyndns.org In-Reply-To: <201004161131.06880.trast@student.ethz.ch> ``` Thomas Rast 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'". ```