OK. I have split the glist removal out of the bug fix and updated the pull request for that. I am still thinking about the glist stuff. I don't think it is working currently, but I think it is a simple fix. I will investigate some more.
Stefan 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
