{"thread":{"id":"23471","subject":"[PATCH] Make rev_compare_tree less confusing.","startedAt":"2010-04-15T08:46:11Z","lastAt":"2010-04-16T20:23:30Z","messageCount":3,"participants":["Bo Yang","Thomas Rast","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"139565","messageId":"1271321171-12176-1-git-send-email-struggleyb.nku@gmail.com","threadId":"23471","inReplyTo":null,"subject":"[PATCH] Make rev_compare_tree less confusing.","fromName":"Bo Yang","fromEmail":"struggleyb.nku@gmail.com","sentAt":"2010-04-15T08:46:11Z","receivedAt":"2010-04-15T08:46:11Z","isPatch":true,"sender":{"key":"struggleyb.nku@gmail.com","avatar":"https://avatars.githubusercontent.com/u/233030?v=4"},"body":"diff_tree_sha1 always return 0, so comparing the return value\nof it make no sense. Just delete the comparison to make code\nreader clear.\n\nSigned-off-by: Bo Yang <struggleyb.nku@gmail.com>\n---\n revision.c |    4 +---\n 1 files changed, 1 insertions(+), 3 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex f4b8b38..8caca99 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -329,9 +329,7 @@ static int rev_compare_tree(struct rev_info *revs, struct commit *parent, struct\n \n \ttree_difference = REV_TREE_SAME;\n \tDIFF_OPT_CLR(&revs->pruning, HAS_CHANGES);\n-\tif (diff_tree_sha1(t1->object.sha1, t2->object.sha1, \"\",\n-\t\t\t   &revs->pruning) < 0)\n-\t\treturn REV_TREE_DIFFERENT;\n+\tdiff_tree_sha1(t1->object.sha1, t2->object.sha1, \"\", &revs->pruning);\n \treturn tree_difference;\n }\n \n-- \n1.6.0.4\n"},{"id":"139662","messageId":"201004161131.06880.trast@student.ethz.ch","threadId":"23471","inReplyTo":"1271321171-12176-1-git-send-email-struggleyb.nku@gmail.com","subject":"Re: [PATCH] Make rev_compare_tree less confusing.","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2010-04-16T09:31:06Z","receivedAt":"2010-04-16T09:31:06Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Bo Yang wrote:\n> -\tif (diff_tree_sha1(t1->object.sha1, t2->object.sha1, \"\",\n> -\t\t\t   &revs->pruning) < 0)\n> -\t\treturn REV_TREE_DIFFERENT;\n> +\tdiff_tree_sha1(t1->object.sha1, t2->object.sha1, \"\", &revs->pruning);\n\nAck on the patch contents (though we could also make the function\n'void' to reduce further confusion), but I'd word the commit message\ndifferently:\n\n> Make rev_compare_tree less confusing.\n> \n> diff_tree_sha1 always return 0, so comparing the return value\n> of it make no sense. Just delete the comparison to make code\n> reader clear.\n\nSomething like\n\n  rev_compare_tree: do not check return value of diff_tree_sha1\n\n  diff_tree_sha1() unconditionally inherits its return value from\n  diff_tree(), which always returns 0.  Hence, pretending that its\n  return value carries any information about the tree difference is\n  extremely misleading.\n\n  The point of the call is in the side effects on revs->pruning, so\n  simply drop the dead 'return'.\n\nInterestingly enough, this call survived with slight changes here and\nthere from all the way back in cf48454 (Teach git-rev-list to follow\njust a specified set of files, 2005-10-20), where it was added in\nrev-list.c.  Even back then diff_tree() would always return 0.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"139695","messageId":"7v1vefcfjx.fsf@alter.siamese.dyndns.org","threadId":"23471","inReplyTo":"201004161131.06880.trast@student.ethz.ch","subject":"Re: [PATCH] Make rev_compare_tree less confusing.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-04-16T20:23:30Z","receivedAt":"2010-04-16T20:23:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast <trast@student.ethz.ch> writes:\n\n> Interestingly enough, this call survived with slight changes here and\n> there from all the way back in cf48454 (Teach git-rev-list to follow\n> just a specified set of files, 2005-10-20), where it was added in\n> rev-list.c.  Even back then diff_tree() would always return 0.\n\nThe idea always have been that diff_tree() may some day start returning\nnon-fatal errors (otherwise it calls die() so that the caller does not\neven have a chance to worry about the return value), and the particular\ncodepath the patch touches would treat such \"one of the trees is unreadble\nso we cannot say if/how different they are\" case as \"there may be a change\nworth showing (even if the actual change couldn't be shown)\".\n\nSo I'm a bit reluctant to accept this patch under discussion.  It doesn't\nchange the behaviour, it doesn't make the _interface_ any simpler, and it\nremoves the documentation value of what _should_ happen if diff_tree()\nwere to be updated in such a way.\n\nIt is entirely a different matter if the patch were to change the function\nsignature of diff_tree() to return \"void\" and adjust all the callers\ninvolved.  That will make the _interface_ simpler without changing the\nbehaviour, and it makes it absolutely clear that we would _never_ enhance\ngit to say \"some necessary data for comparison is missing---we report\nerror and allow the caller to err on the safe side and say 'there could be\ndifference but the details we cannot say'\".\n"}]}