Revist Bug798093

[jralls]

Reverts ed486a5 as that was based on seriously misunderstanding what was going on.

0f8d9f6 changes the way trading accounts are found from looking at their names and ignoring account types to looking for account-types and commodities and ignoring names in order to fix Bug 798093 with the added (supposed) advantage of allowing the top level trading account to have any name.

Sadly I overlooked a key issue in that commit: The top-level trading account and any placeholders were created with the transaction currency of the transaction that caused them to be created. The new logic wouldn't find the existing ones if the current transaction has a different currency, so it would make a new hierarchy.

This PR changes in two ways: When looking for the top-level trading account and namespace placeholder account it will accept any account currency, and if it needs to create one or both it will get the book's root account currency (if the book is old and doesn't have one it will use the currency of the first top-level income account). If, thanks to 0f8d96 there's more than one top-level trading account or namespace placeholder it will prefer one in the root account-currency but if that doesn't exist it will use the first one found.

[mtalexander]

The changes to the TransScrubOrphansFast function and reports/standard/test/test-bdget.scm seem unrelated and I didn't include them in my testing.

Both find_root_currency and xaccScrubUtilityGetOrMakeAccount seem to leak a GList sometimes.

It seems like it would be better in xaccScrubUtilityGetOrMakeAccount if checkName is false and there are two or more accounts that match type and commodity, then prefer one that matches the name too, if any. That way if the file has two trading accounts for the same commodity it will continue to use the one with the new name rather than (perhaps) switching back to the old one.

The transaction that can't be balanced still can't be balanced. I'll add something to the bug report about this.

[jralls]

find_root_currency can't leak anything because it doesn't allocate anything, but I spaced freeing the list returned by gnc_account_lookup_by_type_and_commodity. Fixed that along with removing the superfluous change to test-budget.scm.

This latest round picks up your suggestion from the bug about preferring the trading account whose name matches the commodity mnemonic where there's more than one trading account for a commodity. More significantly it addresses the balancing problem you have with transactions that have more than one trading account split because of an old name change by deleting all trading account splits at the beginning of xaccTransScrubImbalance() instead of trying to adjust existing ones.

[mtalexander]

find_root_currency calls gnc_account_get_children which returns a copy of the account list. It is this copy that I think is being leaked. Or it could just access the child list directly and not make a copy of it.

[jralls]

Ah, you're right, gnc_account_get_children does indeed make a copy of the list.

[mtalexander]

I tested your changes up to e7b94d7 (which I think is current) and everything seems to work much better. It seems to use the existing trading account, if any, in all cases. It also cleans up a transaction with multiple trading accounts when it is rebalanced. Thanks for cleaning this up. I clearly didn't think about renaming commodities when I wrote that code.

[jralls]

@mtalexander Thanks for the reviews and catching the leaks.