GnuCash 3.8b running on Debian Testing shows incorrect balance of a security account in the Asset Chart. The problem is observed for a single account (values shown are roughly 2x the correct values), balances for all other accounts are shown correctly. The problem is observed no matter what price is being used (last, average, closest...). The problem is observed only if many (> ~30) accounts are selected to be included into the report. No particular account being included in the report triggers the problem. So far I could not produce a minimal example not involving too much personal information reproducing the issue. GnuCash 3.7 does not exhibit this problem. According to git bisect, 92509761a53d5126ef2bf77d819dabd1de690f49 is the first commit with the problem. GnuCash 3.8b with this commit reverted indeed does show correct balance of the account. Any ideas on what can be broken by this commit? I am happy to test potential fixes or do bisection inside the commit, if necessary. Just let me know.
This particular commit is designed to *FIX* things and I cannot see anything broken with it. Having said that, we can work with it. (1) you'll want to modify category-barchart.scm as per [1] below (2) run the report on current maint and attach the tracefile (3) revert the offending commit and, run report, and attach tracefile Make sure to label the tracefiles accurately. This will be a start to debugging. [1] patch to log: diff --git a/gnucash/report/standard-reports/category-barchart.scm b/gnucash/report/standard-reports/category-barchart.scm index 6911a7069..22efcfa19 100644 --- a/gnucash/report/standard-reports/category-barchart.scm +++ b/gnucash/report/standard-reports/category-barchart.scm @@ -241,7 +241,8 @@ developing over time")) (accounts (get-option gnc:pagename-accounts optname-accounts)) (account-levels (get-option gnc:pagename-accounts optname-levels)) - + (accts-subaccts + (gnc:pk 'accts-subaccts (gnc:accounts-and-all-descendants accounts))) (chart-type (get-option gnc:pagename-display optname-chart-type)) (stacked? (get-option gnc:pagename-display optname-stacked)) (show-fullname? (get-option gnc:pagename-display optname-fullname)) @@ -366,7 +367,7 @@ developing over time")) #:nosplit->elt (gnc:make-gnc-monetary comm 0))))) ;; all selected accounts (of report-specific type), *and* ;; their descendants (of any type) need to be scanned. - (gnc:accounts-and-all-descendants accounts))) + accts-subaccts)) ;; Creates the <balance-list> to be used in the function ;; below. @@ -461,9 +462,11 @@ developing over time")) ;; lookup should be distributed and done when actually ;; needed so as to amortize the cpu time properly. (gnc:report-percent-done 1) - (set! commodity-list (gnc:accounts-get-commodities - (gnc:accounts-and-all-descendants accounts) - report-currency)) + (set! commodity-list + (gnc:pk 'commodity-list + (gnc:accounts-get-commodities + accts-subaccts + report-currency))) (set! exchange-fn (gnc:case-exchange-time-fn price-source report-currency commodity-list to-date-t64
It seems that the output added by your patch goes to stderr, not to the tracefile. Tracefile is actually empty (unless I specify --debug --extra). Patch: d17.2966 t17.297: ('accts-subaccts (list Acc<DD> Acc<EE> Acc<GG> Acc<I> Acc<J> Acc<K> Acc<L> Acc<M> Acc<T> Acc<FF> Acc<HH> Acc<II> Acc<JJ> Acc<Assets> Acc<B> Acc<F> Acc<G> Acc<H> Acc<N> Acc<O> Acc<P> Acc<R> Acc<S> Acc<U> Acc<V> Acc<Y> Acc<Z> Acc<SS> Acc<TT> Acc<Q> Acc<C> Acc<D> Acc<KK> Acc<E> Acc<PP> Acc<RR> Acc<BB> Acc<CC> Acc<X> Acc<AA> Acc<W> Acc<A> Acc<PP> Acc<RR> Acc<X> Acc<W>)) d0.1071 t17.404: ('commodity-list (list AUD MAD USD S8 S1 S2 S3 S4 S5 S6 S7)) Patch + Revert: d17.3637 t17.364: ('accts-subaccts (list Acc<Assets> Acc<A> Acc<B> Acc<C> Acc<D> Acc<E> Acc<F> Acc<G> Acc<H> Acc<I> Acc<J> Acc<K> Acc<L> Acc<M> Acc<N> Acc<O> Acc<P> Acc<HH> Acc<II> Acc<JJ> Acc<KK> Acc<PP> Acc<SS> Acc<TT> Acc<Q> Acc<R> Acc<RR> Acc<S> Acc<T> Acc<U> Acc<V> Acc<W> Acc<X> Acc<Y> Acc<AA> Acc<BB> Acc<CC> Acc<DD> Acc<EE> Acc<Z> Acc<FF> Acc<GG>)) d0.0387 t17.402: ('commodity-list (list AUD MAD S1 S2 S3 S4 S5 S6 S7 S8 USD)) I guess, the duplicates in the accounts have to do something with the problem?
I have a feeling that xaccAccountOrder just does not implement a total order (consider two accounts with codes and one without — transitivity may fail) and is used for sorting and elimination of duplicates.
Your analysis is correct. xaccAccountOrder seems to be very reliable in ensuring a stable account ordering, but looks like it's buggy. IIRC it's possible to export your account structure to a new data file without any transactions. It'll be useful to debug. Or there's another tool to obfuscate the data file in the sources.
A simple fix would be to revert the account ordering function as follows, but I'm fastidious and wish to find the bug in xaccTransOrder. Would you mind exporting your anonymised account tree as a CSV file? No transactional data is needed. modified gnucash/report/report-system/report-utilities.scm @@ -155,7 +155,8 @@ construct gnc:make-gnc-monetary and use gnc:monetary->string instead.") (define (gnc:accounts-and-all-descendants accountslist) (sort-and-delete-duplicates (apply append accountslist (map gnc-account-get-descendants accountslist)) - (lambda (a b) (< (xaccAccountOrder a b) 0)) + (lambda (a b) (string<? (gnc-account-get-full-name a) + (gnc-account-get-full-name b))) equal?))
As I briefly mentioned above, the problem is in how account codes are used in the comparison: their integer values are sometimes ignored and sometimes not. Consider accounts: Name = A, Code = '20' Name = B, Code = '20_' Name = C, Code = 'a' Name = D, Code = '11' A < B, because '20' < '20_' lexicographically. B < C, because '20_' < 'a' lexicographically. C < D, because a (10) < 11 numerically. D < A, because 11 < 20 numerically. A possible fix (works for me): diff --git a/libgnucash/engine/Account.cpp b/libgnucash/engine/Account.cpp index fcacf1862..194976dbd 100644 --- a/libgnucash/engine/Account.cpp +++ b/libgnucash/engine/Account.cpp @@ -2239,18 +2239,6 @@ xaccAccountOrder (const Account *aa, const Account *ab) da = priv_aa->accountCode; db = priv_ab->accountCode; - /* If accountCodes are both base 36 integers do an integer sort */ - la = strtoul (da, &endptr, 36); - if ((*da != '\0') && (*endptr == '\0')) - { - lb = strtoul (db, &endptr, 36); - if ((*db != '\0') && (*endptr == '\0')) - { - if (la < lb) return -1; - if (la > lb) return +1; - } - } - /* Otherwise do a string sort */ result = g_strcmp0 (da, db); if (result) Do you still need the accounts from me, or you can construct a minimal example from the above explanation yourself?
Good skills... This explanation makes total sense. I do not know whether this behaviour is intended; it fails my assumption that xaccAccountOrder being *very* reliable in ordering accounts in a consistent way. My personal preference is to string-compare account->guid (a pointer dereference) but this is not currently exposed to guile.... a close second preference is to use the account-full-name as above (must be constructed every time).
If I would need to sort accounts for the sake of removing duplicates, I would just use guids: they are always there, easy to compare, and unique by definition. But xaccAccountOrder probably should be fixed anyway.
Fixing xaccAccountOrder will cause side effects... This weird base-36 sort has persisted for 19 years now [1] and is ready to leave home. I'm sure some older users have used it to create their own acc-code hierarchy. And I'm not a CPP coder and am struggling to create a simple GUID getter :-O. An existing macro is marked as deprecated [2] so unless a senior developer intervenes I think I'll just revert to use gnc-account-get-full-name. [1] 50646f04a9e77f3a0d2827670918b26928b47c8b from 2000 [2] c90c817e64fbad1c12c2793e37edbb352b22ee8c from 2003 - see xaccAccountGetGUID
I've pushed the change 5ac7f1bea to use gnc:account-path-less-p. Thank you very much for debugging this.
re: the weird sorting in xaccAccountOrder - I still believe it's buggy because, although some older users will rely on it, will not produce consistent sorting. I'd think it's better to remove the base-36 sorting myself and encourage older users to modernise their codes!