<div dir="ltr">OK, so I think the qofinstance (and lower) code relative to guid<br>functionality is mostly right. What I think the expected behaviour<br>is:<br>1. qof_instance_kvp_add_guid(path, key):<br> - If path is not empty, it is removed to make room for the new one.<br> - if path is empty it adds the guid under key and time under "date"<br> as a frame on path.<br>2. The {path} is either a FRAME with key and "date" fields, or a glist<br> with one entry with "date" and key fields, and the rest with just<br> key fields.<br>3. The merge merges all these into the target either as a glist of<br> kvp, and removes from the donor.<br><br>I do think there is a bug in the merge where it ignores the merging<br>being done by KvpVallue::add instead of setting it. Specifically:<br><br>modified libgnucash/engine/qofinstance.cpp @@ -1257,7 +1257,7 @@<br>qof_instance_kvp_merge_guids (const QofInstance *target, { case<br>KvpValue::Type::FRAME: if (target_val)<br>- target_val->add(v);<br>+ target->kvp_data->set({path}, target_val->add(v)); //Contents moved,<br> Don't delete! else target->kvp_data->set_path({path}, v);<br> donor->kvp_data->set({path}, nullptr); //Contents moved, Don't<br> delete!<br><br>With this fix at the only caller of KvpValue::add that function works<br>fine. Without this the add only works if it is a list, which it never<br>is. I cannot fix this in KvpValue::add. If it is a frame, cannot make<br>itself a list because it would be doubly referenced both in the frame<br>that it is in, and in the list that it put itself in. (FYI: I'm<br>looking forward to the c++ conversion and using smart pointers as<br>memory management. The memory management is really hard to follow/get<br>right.)<br><br>I don't know if this fixes any real bugs or not. I do know that with<br>this the qofinstance can store a list of guids, and the following test<br>covers all the interesting functionality for both lists and frames.<br><br>TEST_F(OqfInstanceTest, merge_guids)<br>{<br> auto inst3 = static_cast<QofInstance*>(g_object_new(QOF_TYPE_INSTANCE, nullptr));<br> auto inst4 = static_cast<QofInstance*>(g_object_new(QOF_TYPE_INSTANCE, nullptr));<br> auto gncGuid2 = guid_new();<br> auto gncGuid3 = guid_new();<br> auto gncGuid4 = guid_new();<br> auto gncGuid5 = guid_new();<br><br> // Add merge from 1 to 0 guids.<br> qof_instance_kvp_add_guid(m_inst2, "donor", 456u, "donortime", gncGuid2);<br> qof_instance_kvp_merge_guids(m_inst, m_inst2, "donor");<br> EXPECT_EQ(qof_instance_get_path_kvp<Time64>(m_inst, {"donor", "date"}).value().t, 456u);<br> EXPECT_TRUE(qof_instance_kvp_has_guid(m_inst, "donor", "donortime", gncGuid2));<br> EXPECT_FALSE(qof_instance_kvp_has_guid(m_inst2, "donor", "donortime", gncGuid2));<br><br> // Merge from 1 to 1 guids.<br> qof_instance_kvp_add_guid(m_inst2, "donor", 456u, "donortime", gncGuid3);<br> qof_instance_kvp_merge_guids(m_inst, m_inst2, "donor");<br> EXPECT_TRUE(qof_instance_kvp_has_guid(m_inst, "donor", "donortime", gncGuid3));<br> EXPECT_FALSE(qof_instance_kvp_has_guid(m_inst2, "donor", "donortime", gncGuid3));<br><br> // Merge from 2 (m_inst2) to to 2 guids.<br> qof_instance_kvp_add_guid(m_inst2, "donor", 456u, "donortime", gncGuid4);<br> qof_instance_kvp_add_guid(inst3, "donor", 456u, "donortime", gncGuid5);<br> qof_instance_kvp_merge_guids(m_inst2, inst3, "donor");<br> qof_instance_kvp_merge_guids(m_inst, m_inst2, "donor");<br> EXPECT_TRUE(qof_instance_kvp_has_guid(m_inst, "donor", "donortime", gncGuid5));<br> EXPECT_FALSE(qof_instance_kvp_has_guid(m_inst2, "donor", "donortime", gncGuid5));<br><br> // Merge from 4 to p guids.<br> EXPECT_TRUE(qof_instance_kvp_has_guid(m_inst, "donor", "donortime", gncGuid5));<br> qof_instance_kvp_merge_guids(inst4, m_inst, "donor");<br> EXPECT_TRUE(qof_instance_kvp_has_guid(inst4, "donor", "donortime", gncGuid5));<br> EXPECT_FALSE(qof_instance_kvp_has_guid(m_inst, "donor", "donortime", gncGuid5));<br> qof_instance_kvp_remove_guid(inst4, "donor", "donortime", gncGuid5);<br> EXPECT_FALSE(qof_instance_kvp_has_guid(inst4, "donor", "donortime", gncGuid5));<br> EXPECT_TRUE(qof_instance_kvp_has_guid(inst4, "donor", "donortime", gncGuid4));<br> EXPECT_TRUE(qof_instance_kvp_has_guid(inst4, "donor", "donortime", gncGuid3));<br><br> g_object_unref(inst3);<br> g_object_unref(inst4);<br>}<br><br>I don't know if this is the expected behavior for the higher level<br>code. It seems a little funky that sometimes the date/time is not<br>stored in the list, or that the keys could be different, but that is<br>likely OK.<br><br>I also don't know how this would affect real usage. There is no<br>mentions (that a quick google search could find) of peer splits in the<br>documentation. I did chance this up that the merge is only used in<br>xaccScrubMergeLotSubSplits. But that is called in more places I am<br>not familiar with. If you find a bug for this I can make a pull<br>request with this.<br><br>You mentioned capgains testing. If you point me in the right<br>direction, I can take a look, but I don't know where to start at this<br>point.<br></div><br><div class="gmail_quote gmail_quote_container"><div dir="ltr" class="gmail_attr">On Wed, Jan 28, 2026 at 11:07 PM John Ralls <<a href="mailto:jralls@ceridwen.us">jralls@ceridwen.us</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
<br>
> On Jan 28, 2026, at 05:35, Stefan Koch <<a href="mailto:stefan.koch.micro@gmail.com" target="_blank">stefan.koch.micro@gmail.com</a>> wrote:<br>
> <br>
> There is one more issue related to this in qof_instance_kvp_merge_guids here:<br>
> if (target_val)<br>
> target_val->add(v);<br>
> <br>
> 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.).<br>
> <br>
> 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.<br>
> <br>
> I will add the following change to the removal of glist cases in qofinstance guid functionality.<br>
> <br>
> modified libgnucash/engine/qofinstance.cpp<br>
> @@ -1220,7 +1220,7 @@ qof_instance_kvp_merge_guids (const QofInstance *target,<br>
> if(v->get_type() == KvpValue::Type::FRAME)<br>
> {<br>
> if (target_val)<br>
> - target_val->add(v);<br>
> + PWARN ("Instance KVP on path %s already exists.", path);<br>
> else<br>
> target->kvp_data->set_path({path}, v);<br>
> donor->kvp_data->set({path}, nullptr); //Contents moved, Don't delete!<br>
> <br>
> 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.<br>
> <br>
> I have created the pull request for this change (with the trivial idata property fix as a previous commit).<br>
<br>
Stefan,<br>
<br>
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.<br>
<br>
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.<br>
<br>
Looks like we need better capgains testing before we muck with this code.<br>
<br>
Regards,<br>
John Ralls<br>
<br>
</blockquote></div>