<div dir="ltr">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></div><br><div class="gmail_quote gmail_quote_container"><div dir="ltr" class="gmail_attr">On Tue, Jan 27, 2026 at 5:38 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"><div><br id="m_-553236987804703074lineBreakAtBeginningOfMessage"><div><br><blockquote type="cite"><div>On Jan 27, 2026, at 12:19, Stefan Koch <<a href="mailto:stefan.koch.micro@gmail.com" target="_blank">stefan.koch.micro@gmail.com</a>> wrote:</div><br><div><div dir="ltr">Inside qofinstance.cpp, the guid functionality is only used inside the following four functions.  Those only seem to be used by the Split.cpp (in particular Peer splits.) The add_guid only handle the kvp frame not the list.  The others work with either.<br><br>1. qof_instance_kvp_add_guid --- only used in split: xaccSplitAddPeerSplit<br>   Only does kvpframe version.<br>2. qof_instance_kvp_has_guid  --- Only used in split: xaccSplitIsPeerSplit<br>3. qof_instance_kvp_remove_guid --- Only used in split: xaccSplitRemovePeerSplit<br>4. qof_instance_kvp_merge_guids --- Only used in xaccSplitMergePeerSplits<br><br><br>I have removed the glist parts of the other three methods, and run the full test suite. There were no failed tests.<br><br>I'm glad you suggested removing the glist code.  I was kinda stuck on setting up an object manually just to test the other functions.  It is not worth it, unless there is some concern that some real code is doing that. (I'm not sure I know how to look for that, so have not.)<br><br>I will add the glist removal to the pending pull request (after I get the rest of qofinstance.cpp tested).<br><br></div></div></blockquote></div><br><div>Stefan,</div><div><br></div><div>No test touches qof_instance_kvp_remove_guid or qof_instance_kvp_merge_guids, see <a href="https://gnucash.github.io/gnucash/Coverage-HTML/libgnucash/engine/qofinstance.cpp.gcov.html" target="_blank">https://gnucash.github.io/gnucash/Coverage-HTML/libgnucash/engine/qofinstance.cpp.gcov.html</a>, so it doesn’t provide any assurance that my analysis is correct. I think it’s a pretty safe bet anyway given that the functions are all called from only one place.</div><div><br></div><div>I realized in reviewing that code that it assumes that a split should have only one peer relationship at a time, and that’s not necessarily true. </div><div>So I went to search in BZ to see if anyone had reported a problem with that (didn’t find any, but...) I found <a href="http://bugs.gnucash.org/show_bug.cgi?id=798873" target="_blank">bugs.gnucash.org/show_bug.cgi?id=798873</a>, and there’s a stack trace showing the crash originates at qofinstance.cpp line 1188. The version of qofinstance.cpp in 5.0, the GnuCash version of the bug, is <a href="https://github.com/Gnucash/gnucash/blob/af02dae28684f1e31c6937dc5a30df4d0e7adb01/libgnucash/engine/qofinstance.cpp" target="_blank">https://github.com/Gnucash/gnucash/blob/af02dae28684f1e31c6937dc5a30df4d0e7adb01/libgnucash/engine/qofinstance.cpp</a> and line 1188 is the second deletion of v in qof_instance_kvp_remove_guid, so you get to claim your first bug fix! The protocol for bug fix commits is that the summary line should be a copy of the bug title, in this case "Bug 798873 - Crash when scrubbing after "undoing” changes”.  Please use that for your commit.</div><div><br></div><div>Regards,</div><div>John Ralls</div><div><br></div><div><br></div></div></blockquote></div>