From 601eb513619a9d8d9a609ac22d5e3b20527ed6ad Mon Sep 17 00:00:00 2001
From: John Ralls <jralls@ceridwen.us>
Date: Tue, 8 Jun 2021 09:49:46 -0700
Subject: [PATCH] Improve transaction sorting on effective num field.

Inspired by PR #983. Transaction sorting on num broke down if the
user had a non-numeric string or a number larger than an int in
the effective num field (might be split-action if the option is set).

The comparison first tries to use strtoull on the two strings and
compares the results. If they're both nonzero and different then the
numeric order is returned. If they're both nonzero but the same the
unconverted parts of each are passed to g_utf8_collate; if either is 0
then the whole strings are passed to g_utf8_collate. strtoull will
return 0 for a negative number.
---
 libgnucash/engine/Transaction.c              | 43 ++++++++++++++++----
 libgnucash/engine/test/utest-Transaction.cpp |  8 +++-
 2 files changed, 43 insertions(+), 8 deletions(-)

diff --git a/libgnucash/engine/Transaction.c b/libgnucash/engine/Transaction.c
index 2865b237ea..e7c3cba0af 100644
--- a/libgnucash/engine/Transaction.c
+++ b/libgnucash/engine/Transaction.c
@@ -1881,12 +1881,43 @@ xaccTransOrder (const Transaction *ta, const Transaction *tb)
     return xaccTransOrder_num_action (ta, NULL, tb, NULL);
 }
 
+/* Order a pair of potentially numeric string as numbers if both
+ * strings begin with numbers, ordering the remainder of the string
+ * lexically if the numeric parts are equal, and the whole strings
+ * lexically otherwise.
+ *
+ * Note that this won't work well for numbers > 10^18 and that
+ * negative numbers are treated as strings and will cause the pair to
+ * be ordered lexically.
+ */
+
+static int
+order_by_int64_or_string (const char* a, const char* b)
+{
+     char *end_a = NULL, *end_b = NULL;
+     int cmp = 0;
+     uint64_t na = strtoull(a, &end_a, 10);
+     uint64_t nb = strtoull(b, &end_b, 10);
+     if (na && nb)
+     {
+          if (na != nb)
+               return na < nb ? -1 : 1;
+          cmp = g_utf8_collate(end_a, end_b);
+     }
+     else
+     {
+          cmp = g_utf8_collate(a, b);
+     }
+     return cmp < 0 ? -1 : cmp > 0 ? 1 : 0;
+}
+
 int
 xaccTransOrder_num_action (const Transaction *ta, const char *actna,
                             const Transaction *tb, const char *actnb)
 {
     char *da, *db;
-    int na, nb, retval;
+    int retval;
+    int64_t na, nb;
 
     if ( ta && !tb ) return -1;
     if ( !ta && tb ) return +1;
@@ -1906,16 +1937,14 @@ xaccTransOrder_num_action (const Transaction *ta, const char *actna,
     /* otherwise, sort on number string */
     if (actna && actnb) /* split action string, if not NULL */
     {
-        na = atoi(actna);
-        nb = atoi(actnb);
+         retval = order_by_int64_or_string (actna, actnb);
     }
     else                /* else transaction num string */
     {
-        na = atoi(ta->num);
-        nb = atoi(tb->num);
+         retval = order_by_int64_or_string (ta->num, tb->num);
     }
-    if (na < nb) return -1;
-    if (na > nb) return +1;
+    if (retval)
+         return retval;
 
     if (ta->date_entered != tb->date_entered)
         return (ta->date_entered > tb->date_entered) - (ta->date_entered < tb->date_entered);
diff --git a/libgnucash/engine/test/utest-Transaction.cpp b/libgnucash/engine/test/utest-Transaction.cpp
index ec8de4c4b7..3cf10ebb82 100644
--- a/libgnucash/engine/test/utest-Transaction.cpp
+++ b/libgnucash/engine/test/utest-Transaction.cpp
@@ -1763,8 +1763,14 @@ test_xaccTransOrder_num_action (Fixture *fixture, gconstpointer pData)
     g_assert_cmpint (xaccTransOrder_num_action (txnA, NULL, txnB, NULL), ==, -1);
     txnB->num = static_cast<char*>(CACHE_INSERT ("101"));
     g_assert_cmpint (xaccTransOrder_num_action (txnA, NULL, txnB, NULL), ==, 1);
-    txnB->num = static_cast<char*>(CACHE_INSERT ("one-oh-one"));
+    txnA->num = static_cast<char*>(CACHE_INSERT ("12a"));
+    g_assert_cmpint (xaccTransOrder_num_action (txnA, NULL, txnB, NULL), ==, -1);
+    txnB->num = static_cast<char*>(CACHE_INSERT ("12c"));
+    g_assert_cmpint (xaccTransOrder_num_action (txnA, NULL, txnB, NULL), ==, -1);
+    txnB->num = static_cast<char*>(CACHE_INSERT ("12"));
     g_assert_cmpint (xaccTransOrder_num_action (txnA, NULL, txnB, NULL), ==, 1);
+    txnB->num = static_cast<char*>(CACHE_INSERT ("one-oh-one"));
+    g_assert_cmpint (xaccTransOrder_num_action (txnA, NULL, txnB, NULL), ==, -1);
     g_assert_cmpint (xaccTransOrder_num_action (txnA, "24", txnB, "42"), ==, -1);
     txnB->date_posted -= 1;
     g_assert_cmpint (xaccTransOrder_num_action (txnA, "24", txnB, "42"), ==, 1);