compare_by_commodity: Always return the result of the recursive call

Commit 501fbc08ae5493db77bb34f4c4fbe1f3a3bc14e3 changed the behavior
of this function to not return the "equal" result (==0) from the
recursive call. Previously, the function returned the result of the
recursive call unconditionally.

The current behavior causes an assertion error for certain
postings. The regression test added in this commit shows such a
posting.

I found through Travis CI that the old behavior was incomplete and
caused unstable orderings, so reverting to the old behavior doesn't
work. Instead, this change adds a fallback: If the recursive call that
compares the prices numerically returns "equal", then compare the
prices with their original commodity as a tie breaker.

This commit does not change any existing ordering, it only adds
deterministic ordering in a case that currently triggers an assertion
error.

This fixes issue #1998.

1 files changed, 11 insertions, 6 deletions

diff --git a/src/commodity.cc b/src/commodity.cc
index bdf6ee54..ebe388c8 100644
--- a/src/commodity.cc
+++ b/src/commodity.cc
@@ -465,18 +465,23 @@ int commodity_t::compare_by_commodity::operator()(const amount_t * left,
     amount_t rightprice(*arightcomm.details.price);
 
     if (leftprice.commodity() != rightprice.commodity()) {
-      // Since we have two different amounts, there's really no way
-      // to establish a true sorting order; we'll just do it based
-      // on the numerical values.
-      leftprice.clear_commodity();
-      rightprice.clear_commodity();
+      // Since we have two different amounts, there's really no way to
+      // establish a true sorting order; we'll just do it based on the
+      // numerical values, before falling back to comparing the prices
+      // with their original commodity.
+      amount_t leftpricenumeric(leftprice);
+      amount_t rightpricenumeric(rightprice);
+      leftpricenumeric.clear_commodity();
+      rightpricenumeric.clear_commodity();
       DEBUG("commodity.compare",
             "both have price, commodities don't match, recursing");
-      int cmp2 = commodity_t::compare_by_commodity()(&leftprice, &rightprice);
+      int cmp2 = commodity_t::compare_by_commodity()(&leftpricenumeric, &rightpricenumeric);
       if (cmp2 != 0) {
         DEBUG("commodity.compare", "recursion found a disparity");
         return cmp2;
       }
+      DEBUG("commodity.compare", "recursion found no difference, comparing prices with commodity");
+      return commodity_t::compare_by_commodity()(&leftprice, &rightprice);
     } else {
       if (leftprice < rightprice) {
         DEBUG("commodity.compare", "left price is less");