<div dir="ltr"><div>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.  </div><div><br></div><div>Stefan</div></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>