OK, so I think the qofinstance (and lower) code relative to guid
functionality is mostly right. What I think the expected behaviour
is:
1. qof_instance_kvp_add_guid(path, key):
- If path is not empty, it is removed to make room for the new one.
- if path is empty it adds the guid under key and time under "date"
as a frame on path.
2. The {path} is either a FRAME with key and "date" fields, or a glist
with one entry with "date" and key fields, and the rest with just
key fields.
3. The merge merges all these into the target either as a glist of
kvp, and removes from the donor.
I do think there is a bug in the merge where it ignores the merging
being done by KvpVallue::add instead of setting it. Specifically:
modified libgnucash/engine/qofinstance.cpp @@ -1257,7 +1257,7 @@
qof_instance_kvp_merge_guids (const QofInstance *target, { case
KvpValue::Type::FRAME: if (target_val)
- target_val->add(v);
+ target->kvp_data->set({path}, target_val->add(v)); //Contents moved,
Don't delete! else target->kvp_data->set_path({path}, v);
donor->kvp_data->set({path}, nullptr); //Contents moved, Don't
delete!
With this fix at the only caller of KvpValue::add that function works
fine. Without this the add only works if it is a list, which it never
is. I cannot fix this in KvpValue::add. If it is a frame, cannot make
itself a list because it would be doubly referenced both in the frame
that it is in, and in the list that it put itself in. (FYI: I'm
looking forward to the c++ conversion and using smart pointers as
memory management. The memory management is really hard to follow/get
right.)
I don't know if this fixes any real bugs or not. I do know that with
this the qofinstance can store a list of guids, and the following test
covers all the interesting functionality for both lists and frames.
TEST_F(OqfInstanceTest, merge_guids)
{
auto inst3 = static_cast<QofInstance*>(g_object_new(QOF_TYPE_INSTANCE,
nullptr));
auto inst4 = static_cast<QofInstance*>(g_object_new(QOF_TYPE_INSTANCE,
nullptr));
auto gncGuid2 = guid_new();
auto gncGuid3 = guid_new();
auto gncGuid4 = guid_new();
auto gncGuid5 = guid_new();
// Add merge from 1 to 0 guids.
qof_instance_kvp_add_guid(m_inst2, "donor", 456u, "donortime",
gncGuid2);
qof_instance_kvp_merge_guids(m_inst, m_inst2, "donor");
EXPECT_EQ(qof_instance_get_path_kvp<Time64>(m_inst, {"donor",
"date"}).value().t, 456u);
EXPECT_TRUE(qof_instance_kvp_has_guid(m_inst, "donor", "donortime",
gncGuid2));
EXPECT_FALSE(qof_instance_kvp_has_guid(m_inst2, "donor", "donortime",
gncGuid2));
// Merge from 1 to 1 guids.
qof_instance_kvp_add_guid(m_inst2, "donor", 456u, "donortime",
gncGuid3);
qof_instance_kvp_merge_guids(m_inst, m_inst2, "donor");
EXPECT_TRUE(qof_instance_kvp_has_guid(m_inst, "donor", "donortime",
gncGuid3));
EXPECT_FALSE(qof_instance_kvp_has_guid(m_inst2, "donor", "donortime",
gncGuid3));
// Merge from 2 (m_inst2) to to 2 guids.
qof_instance_kvp_add_guid(m_inst2, "donor", 456u, "donortime",
gncGuid4);
qof_instance_kvp_add_guid(inst3, "donor", 456u, "donortime", gncGuid5);
qof_instance_kvp_merge_guids(m_inst2, inst3, "donor");
qof_instance_kvp_merge_guids(m_inst, m_inst2, "donor");
EXPECT_TRUE(qof_instance_kvp_has_guid(m_inst, "donor", "donortime",
gncGuid5));
EXPECT_FALSE(qof_instance_kvp_has_guid(m_inst2, "donor", "donortime",
gncGuid5));
// Merge from 4 to p guids.
EXPECT_TRUE(qof_instance_kvp_has_guid(m_inst, "donor", "donortime",
gncGuid5));
qof_instance_kvp_merge_guids(inst4, m_inst, "donor");
EXPECT_TRUE(qof_instance_kvp_has_guid(inst4, "donor", "donortime",
gncGuid5));
EXPECT_FALSE(qof_instance_kvp_has_guid(m_inst, "donor", "donortime",
gncGuid5));
qof_instance_kvp_remove_guid(inst4, "donor", "donortime", gncGuid5);
EXPECT_FALSE(qof_instance_kvp_has_guid(inst4, "donor", "donortime",
gncGuid5));
EXPECT_TRUE(qof_instance_kvp_has_guid(inst4, "donor", "donortime",
gncGuid4));
EXPECT_TRUE(qof_instance_kvp_has_guid(inst4, "donor", "donortime",
gncGuid3));
g_object_unref(inst3);
g_object_unref(inst4);
}
I don't know if this is the expected behavior for the higher level
code. It seems a little funky that sometimes the date/time is not
stored in the list, or that the keys could be different, but that is
likely OK.
I also don't know how this would affect real usage. There is no
mentions (that a quick google search could find) of peer splits in the
documentation. I did chance this up that the merge is only used in
xaccScrubMergeLotSubSplits. But that is called in more places I am
not familiar with. If you find a bug for this I can make a pull
request with this.
You mentioned capgains testing. If you point me in the right
direction, I can take a look, but I don't know where to start at this
point.
On Wed, Jan 28, 2026 at 11:07 PM John Ralls <[email protected]> wrote:
>
>
> > On Jan 28, 2026, at 05:35, Stefan Koch <[email protected]>
> wrote:
> >
> > There is one more issue related to this in qof_instance_kvp_merge_guids
> here:
> > if (target_val)
> > target_val->add(v);
> >
> > This does not work if the target_val is not already a glist. In that
> case, the KvpValueImpl::add creates a new GList which it returns, that the
> above code ignores. (If it was already a list it should work OK.).
> >
> > Since the qof_instance_kvp_merge_guids is only used in the peer splits
> we looked at yesterday, I think it is safe (readonable?) to assume that it
> will not use the glist functionality here.
> >
> > I will add the following change to the removal of glist cases in
> qofinstance guid functionality.
> >
> > modified libgnucash/engine/qofinstance.cpp
> > @@ -1220,7 +1220,7 @@ qof_instance_kvp_merge_guids (const QofInstance
> *target,
> > if(v->get_type() == KvpValue::Type::FRAME)
> > {
> > if (target_val)
> > - target_val->add(v);
> > + PWARN ("Instance KVP on path %s already exists.", path);
> > else
> > target->kvp_data->set_path({path}, v);
> > donor->kvp_data->set({path}, nullptr); //Contents moved, Don't
> delete!
> >
> > This still leaves the tricky to use KvpValueImpl::add. But it turns out
> that this is only used here, and so when I remove it here, it has no other
> uses (besides the unit test). I will delete that functionality in the
> same commit.
> >
> > I have created the pull request for this change (with the trivial idata
> property fix as a previous commit).
>
> Stefan,
>
> No, KvpVallue::add does work if it doesn’t contain a GLIst: It creates a
> new KvpValue of type GLIST and populates its Glist with the original value
> and the new one. It doesn’t matter that gnc_instance_kvp_merge_guid
> discards the returned KvpValue* because it doesn’t have any use for it:
> What’s important is that the Split’s peer slot has the new GLIST KvpValue
> and subsequent calls to retrieve that slot or KVP path will get it.
>
> This also changes the equation for testing because it means that there is
> a way to get a GUID-GLIST Kvp that you can use to exercise the GLIST
> branches of gnc_instance_kvp_has_guid and gnc_instance_kvp_remove_guid.
>
> Looks like we need better capgains testing before we muck with this code.
>
> Regards,
> John Ralls
>
>
_______________________________________________
gnucash-devel mailing list
[email protected]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
