threads / patch / 23471

patchMake rev_compare_tree less confusing.

Subject: [PATCH] Make rev_compare_tree less confusing.

## tl;dr

3 messages between Apr 15, 2010 and Apr 16, 2010. Diffs are folded; open one to read it.

replies: 2people: 3as markdown or json

Bo Yang· Apr 15, 2010, 08:46 UTC · lore

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(-)

Show changes to revision.c +1 −3

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· Apr 16, 2010, 09:31 UTC · re: Bo Yang · lore

Re: [PATCH] Make rev_compare_tree less confusing.

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:

Show 5 quoted lines

> 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· Apr 16, 2010, 20:23 UTC · re: Thomas Rast · lore

Re: [PATCH] Make rev_compare_tree less confusing.

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