Possible bug in qofinstance.cpp: qof_instance_kvp_remove_guid

[Stefan Koch]

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 <jralls at ceridwen.us> wrote:

>
>
> > On Jan 28, 2026, at 05:35, Stefan Koch <stefan.koch.micro at gmail.com>
> 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.gnucash.org/pipermail/gnucash-devel/attachments/20260130/64b570b0/attachment.htm>