Re: [PATCH] Make rev_compare_tree less confusing.

From: Thomas Rast <trast@student.ethz.ch>
Date: Apr 16, 2010, 09:31 UTC
Message-ID: <201004161131.06880.trast@student.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:

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