Fix test?
Mike Alexander
mta at umich.edu
Mon May 15 02:36:00 EDT 2006
--On May 13, 2006 9:34:24 PM -0600 Chris Lyttle <chris at wilddev.net>
wrote:
> Hi dev's
>
> Is anyone working on fixing the following test so we can release
> 1.9.6?
>
> Chris
>
>
> ** CRITICAL **: Error: xaccSplitComputeCapGains(): Malformed Lot "Lot
> 45"! (too thin!) opening amt=-32139756059553/100 split
> amt=5269956366770822423/100 baln=5269924227014762870/100
> aborting...
I've attached a patch that seems to fix the errors found by test-period
when automatic lot scrubbing is enabled. Even though it is no longer
enabled, these bugs should probably be fixed. I think the patch is ok,
but others who know more about the internals of the GnuCash engine
should take a look at it.
With these patches I get only one error when running test-period with
parameters from 10 to 299. At 207 it gets an "xaccSplitSetValue():
numeric error in converting the split value's denominator" error, but
that's because the test code generates a value that is too big to
represent with the chosen denominator of 1000000.
--
Mike Alexander mta at umich.edu
Ann Arbor, MI PGP key ID: BEA343A6
-------------- next part --------------
2006-05-15 Mike Alexander <mta at arbortext.com>
* src/engine/cap-gains.c (finder_helper): Don't add a split with
the wrong sign to a lot and don't add any split to a lot that is
fat.
* src/engine/cap-gains.c (xaccSplitAssignToLot): Set the value and
amount in the new split after it is in the transaction so the
correct denominator can be found.
* src/engine/cap-gains.c (xaccSplitAssign): Ignore splits with
zero amounts.
* src/engine/cap-gains.c (xaccSplitComputeCapGains): If the gain
is already recorded correctly do nothing. This happens when
called from gnc_book_partition_txn at which time things are
inconsistent.
* src/engine/policy.c (DirectionPolicyGetSplit): Don't add a split
to a lot unless it will be the new last split.
* src/engine/Scrub2.c (xaccScrubMergeSubSplits): Make sure the
splits are really related before merging them.
* src/engine/Account.c (xaccAccountInsertLot): Don't move the
splits in the lot into the new account. The caller does this if
necessary and this fails when called from gnc_book_close_period.
Index: src/engine/cap-gains.c
===================================================================
--- src/engine/cap-gains.c (revision 14072)
+++ src/engine/cap-gains.c (working copy)
@@ -133,16 +133,23 @@
Split *s;
Transaction *trans;
gnc_numeric bal;
+ gboolean opening_is_positive, bal_is_positive;
if (gnc_lot_is_closed (lot)) return NULL;
- /* We want a lot whose balance is of the correct sign */
- bal = gnc_lot_get_balance (lot);
- if (0 == (els->numeric_pred) (bal)) return NULL;
-
s = gnc_lot_get_earliest_split (lot);
if (s == NULL) return NULL;
+ /* We want a lot whose balance is of the correct sign. All splits
+ in a lot must be the opposite sign of the opening split. We also
+ want to ignore lots that are overfull, i.e., where the balance in
+ the lot is of opposite sign to the opening split in the lot. */
+ if (0 == (els->numeric_pred) (s->amount)) return NULL;
+ bal = gnc_lot_get_balance (lot);
+ opening_is_positive = gnc_numeric_positive_p (s->amount);
+ bal_is_positive = gnc_numeric_positive_p (bal);
+ if (opening_is_positive != bal_is_positive) return NULL;
+
trans = s->parent;
if (els->currency &&
(FALSE == gnc_commodity_equiv (els->currency,
@@ -546,12 +553,15 @@
gnc_kvp_bag_add (new_split->inst.kvp_data, "lot-split", now,
"peer_guid", xaccSplitGetGUID (split),
NULL);
-
- xaccSplitSetAmount (new_split, amt_b);
- xaccSplitSetValue (new_split, val_b);
xaccAccountInsertSplit (acc, new_split);
xaccTransAppendSplit (trans, new_split);
+ /* Set the amount and value after the split is in the transaction
+ so it can find the correct denominator to use. Otherwise it
+ uses 100000 which may cause an overflow in some of the tests
+ in test-period */
+ xaccSplitSetAmount (new_split, amt_b);
+ xaccSplitSetValue (new_split, val_b);
xaccTransCommitEdit (trans);
xaccAccountCommitEdit (acc);
return new_split;
@@ -600,6 +610,8 @@
acc = split->acc;
if (!xaccAccountHasTrades (acc))
return FALSE;
+ if (gnc_numeric_zero_p (split->amount))
+ return FALSE;
ENTER ("(split=%p)", split);
@@ -919,6 +931,8 @@
Transaction *trans;
Split *lot_split, *gain_split;
Timespec ts;
+ gboolean new_gain_split;
+ gnc_numeric negvalue = gnc_numeric_neg (value);
/* See if there already is an associated gains transaction.
* If there is, adjust its value as appropriate. Else, create
@@ -932,6 +946,8 @@
Account *lot_acc = lot->account;
QofBook *book = lot_acc->inst.book;
+ new_gain_split = TRUE;
+
lot_split = xaccMallocSplit (book);
gain_split = xaccMallocSplit (book);
@@ -977,42 +993,62 @@
{
trans = lot_split->parent;
gain_split = xaccSplitGetOtherSplit (lot_split);
- xaccTransBeginEdit (trans);
-
- /* Make sure the existing gains trans has the correct currency,
- * just in case someone screwed with it! */
- if (FALSE == gnc_commodity_equiv(currency,trans->common_currency))
+ /* If the gain is already recorded corectly do nothing. This is
+ * more than just an optimization since this may be called during
+ * gnc_book_partition_txn and depending on the order in which things
+ * happen some splits may be in the wrong book at that time. */
+ if (split->gains_split == lot_split &&
+ lot_split->gains_split == split &&
+ gain_split->gains_split == split &&
+ gnc_numeric_equal (xaccSplitGetValue (lot_split), value) &&
+ gnc_numeric_zero_p (xaccSplitGetAmount (lot_split)) &&
+ gnc_numeric_equal (xaccSplitGetValue (gain_split), negvalue) &&
+ gnc_numeric_equal (xaccSplitGetAmount (gain_split), negvalue))
{
- PWARN ("Resetting the transaction currency!");
- xaccTransSetCurrency (trans, currency);
+ new_gain_split = FALSE;
}
+ else
+ {
+ new_gain_split = TRUE;
+ xaccTransBeginEdit (trans);
+
+ /* Make sure the existing gains trans has the correct currency,
+ * just in case someone screwed with it! */
+ if (FALSE == gnc_commodity_equiv(currency,trans->common_currency))
+ {
+ PWARN ("Resetting the transaction currency!");
+ xaccTransSetCurrency (trans, currency);
+ }
+ }
}
- /* Common to both */
- ts = xaccTransRetDatePostedTS (split->parent);
- xaccTransSetDatePostedTS (trans, &ts);
- xaccTransSetDateEnteredSecs (trans, time(0));
-
- xaccSplitSetAmount (lot_split, zero);
- xaccSplitSetValue (lot_split, value);
-
- value = gnc_numeric_neg (value);
- xaccSplitSetAmount (gain_split, value);
- xaccSplitSetValue (gain_split, value);
-
- /* Some short-cuts to help avoid the above kvp lookup. */
- split->gains = GAINS_STATUS_CLEAN;
- split->gains_split = lot_split;
- lot_split->gains = GAINS_STATUS_GAINS;
- lot_split->gains_split = split;
- gain_split->gains = GAINS_STATUS_GAINS;
- gain_split->gains_split = split;
-
- /* Do this last since it may generate an event that will call us
- recursively. */
- gnc_lot_add_split (lot, lot_split);
-
- xaccTransCommitEdit (trans);
+ if (new_gain_split)
+ {
+ /* Common to both */
+ ts = xaccTransRetDatePostedTS (split->parent);
+ xaccTransSetDatePostedTS (trans, &ts);
+ xaccTransSetDateEnteredSecs (trans, time(0));
+
+ xaccSplitSetAmount (lot_split, zero);
+ xaccSplitSetValue (lot_split, value);
+
+ xaccSplitSetAmount (gain_split, negvalue);
+ xaccSplitSetValue (gain_split, negvalue);
+
+ /* Some short-cuts to help avoid the above kvp lookup. */
+ split->gains = GAINS_STATUS_CLEAN;
+ split->gains_split = lot_split;
+ lot_split->gains = GAINS_STATUS_GAINS;
+ lot_split->gains_split = split;
+ gain_split->gains = GAINS_STATUS_GAINS;
+ gain_split->gains_split = split;
+
+ /* Do this last since it may generate an event that will call us
+ recursively. */
+ gnc_lot_add_split (lot, lot_split);
+
+ xaccTransCommitEdit (trans);
+ }
}
LEAVE ("(lot=%s)", gnc_lot_get_title(lot));
}
Index: src/engine/policy.c
===================================================================
--- src/engine/policy.c (revision 14072)
+++ src/engine/policy.c (working copy)
@@ -53,6 +53,9 @@
gnc_commodity *common_currency;
gboolean want_positive;
gnc_numeric baln;
+ Split *osplit;
+ Transaction *otrans;
+ Timespec open_ts;
if (!pcy || !lot || !lot->account || !lot->splits) return NULL;
@@ -65,6 +68,13 @@
/* All splits in lot must share a common transaction currency. */
split = lot->splits->data;
common_currency = split->parent->common_currency;
+
+ /* Don't add a split to the lot unless it will be the new last
+ split in the lot. Otherwise our balance tests will be wrong
+ and the lot may end up too thin or too fat. */
+ osplit = gnc_lot_get_latest_split (lot);
+ otrans = osplit ? xaccSplitGetParent (osplit) : 0;
+ open_ts = xaccTransRetDatePostedTS (otrans);
/* Walk over *all* splits in the account, till we find one that
* hasn't been assigned to a lot. Return that split.
@@ -79,9 +89,22 @@
{
gboolean is_match;
gboolean is_positive;
+ Timespec this_ts;
split = node->data;
if (split->lot) goto donext;
+ /* Skip it if it's too early */
+ this_ts = xaccTransRetDatePostedTS ( xaccSplitGetParent (split));
+ if ((this_ts.tv_sec < open_ts.tv_sec) ||
+ ((this_ts.tv_sec == open_ts.tv_sec) &&
+ (this_ts.tv_nsec < open_ts.tv_nsec)))
+ {
+ if (reverse)
+ /* Going backwards, no point in looking further */
+ break;
+ goto donext;
+ }
+
/* Allow equiv currencies */
is_match = gnc_commodity_equiv (common_currency,
split->parent->common_currency);
Index: src/engine/Scrub2.c
===================================================================
--- src/engine/Scrub2.c (revision 14072)
+++ src/engine/Scrub2.c (working copy)
@@ -416,9 +416,17 @@
if (s->inst.do_free) continue;
/* OK, this split is in the same lot (and thus same account)
- * as the indicated split. It must be a subsplit (although
- * we should double-check the kvp's to be sure). Merge the
- * two back together again. */
+ * as the indicated split. Make sure it is really a subsplit
+ * of the split we started with. It's possible to have two
+ * splits in the same lot and transaction that are not subsplits
+ * of each other, the test-period test suite does this, for
+ * example. Only worry about adjacent sub-splits. By
+ * repeatedly merging adjacent subsplits, we'll get the non-
+ * adjacent ones too. */
+ if (gnc_kvp_bag_find_by_guid (split->inst.kvp_data, "lot-split",
+ "peer_guid", &s->inst.entity.guid) == NULL)
+ continue;
+
merge_splits (split, s);
rc = TRUE;
goto restart;
Index: src/engine/Account.c
===================================================================
--- src/engine/Account.c (revision 14072)
+++ src/engine/Account.c (working copy)
@@ -792,7 +792,6 @@
void
xaccAccountInsertLot (Account *acc, GNCLot *lot)
{
- GList *sl;
Account * old_acc = NULL;
if (!acc || !lot || lot->account == acc) return;
@@ -808,13 +807,11 @@
acc->lots = g_list_prepend (acc->lots, lot);
lot->account = acc;
- /* Move all splits over to the new account. At worst,
- * this is a no-op. */
- for (sl = lot->splits; sl; sl=sl->next) {
- Split *s = sl->data;
- if (s->acc != acc)
- xaccAccountInsertSplit (acc, s);
- }
+ /* Don't move the splits to the new account. The caller will do this
+ * if appropriate, and doing it here will not work if we are being
+ * called from gnc_book_close_period since xaccAccountInsertSplit
+ * will try to balance capital gains and things aren't ready for that. */
+
LEAVE ("(acc=%p, lot=%p)", acc, lot);
}
More information about the gnucash-devel
mailing list