[PATCH] Make rev_compare_tree less confusing.

[PATCH] Make rev_compare_tree less confusing.

[Bo Yang]

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 <struggleyb.nku@gmail.com>
---
 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

Re: [PATCH] Make rev_compare_tree less confusing.

[Thomas Rast]

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

Re: [PATCH] Make rev_compare_tree less confusing.

[Junio C Hamano]

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'".