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).
On Tue, Jan 27, 2026 at 5:38 PM John Ralls <[email protected]> wrote:
>
>
> On Jan 27, 2026, at 12:19, Stefan Koch <[email protected]>
> wrote:
>
> 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.
>
> 1. qof_instance_kvp_add_guid --- only used in split: xaccSplitAddPeerSplit
> Only does kvpframe version.
> 2. qof_instance_kvp_has_guid --- Only used in split: xaccSplitIsPeerSplit
> 3. qof_instance_kvp_remove_guid --- Only used in split:
> xaccSplitRemovePeerSplit
> 4. qof_instance_kvp_merge_guids --- Only used in xaccSplitMergePeerSplits
>
>
> I have removed the glist parts of the other three methods, and run the
> full test suite. There were no failed tests.
>
> 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.)
>
> I will add the glist removal to the pending pull request (after I get the
> rest of qofinstance.cpp tested).
>
>
> Stefan,
>
> No test touches qof_instance_kvp_remove_guid or
> qof_instance_kvp_merge_guids, see
> https://gnucash.github.io/gnucash/Coverage-HTML/libgnucash/engine/qofinstance.cpp.gcov.html,
> 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.
>
> 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.
> So I went to search in BZ to see if anyone had reported a problem with
> that (didn’t find any, but...) I found
> bugs.gnucash.org/show_bug.cgi?id=798873, 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
> https://github.com/Gnucash/gnucash/blob/af02dae28684f1e31c6937dc5a30df4d0e7adb01/libgnucash/engine/qofinstance.cpp
> 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.
>
> Regards,
> John Ralls
>
>
>
_______________________________________________
gnucash-devel mailing list
[email protected]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
