"private-kvp" merge reverted other changes since November.

[John Ralls]

On May 7, 2014, at 5:08 PM, Geert Janssens <janssens-geert at telenet.be> wrote:

> On Wednesday 07 May 2014 16:22:27 Mike Alexander wrote:
>> --On May 7, 2014 6:41:03 PM +0200 Geert Janssens
>> 
>> <janssens-geert at telenet.be> wrote:
>>> On Tuesday 06 May 2014 18:16:51 Mike Alexander wrote:
>>>> Just to pick another random example, the merge also removed a call
>>>> to
>>>> qof_instance_set_dirty in gnc_template_register_save_xfrm_cell
>>>> which
>>>> is in register/ledger-core/split-register-model-save.c.  This call
>>>> was added in 613ba0d on December 7.  This is only one of a number
>>>> of
>>>> changes I noticed.
>>> 
>>> I think that is actually correct. From how I understand John's work
>>> qof_instance_set takes care of properly dirtying the kvp. If I'm
>>> mistaken here then there are many places in the new code that no
>>> longer  mark kvp's as dirty.
>> 
>> Perhaps it is a desired change, but then it should have been on the
>> private-kvp branch.  Instead it was introduced as a side effect of the
>> merge back to master.  Even if the call to qof_instance_set_dirty is
>> not needed after the private-kvp changes, it won't hurt anything.  If
>> it is to be removed it should be removed explicitly, not as a side
>> effect of the merge.
>> 
> It was actually hurting: it caused the build to fail. With the function still in there I got an 
> undefined function error. Perhaps this could have been resolved by adding the proper 
> header include but I didn't check that part.
> 
> Only John can tell why he did remove it during the merge and not beforehand.

I moved most of the QofInstance API into qofinstnace-p.h in 1f3fbf4b, very early in the private-kvp branch, because they involve direct access to what should be private implementation details of the class. In the case of mark dirty, one of the main motivations for the change is to make it impossible to change any persistent object member variable without marking the object dirty. This is actually done twice in those cases where there's a setter function as well as a GObject property if one uses  qof_instance_set() because qof_instance_set() does it to ensure that it's done for those properties that don't have setters and the setter does it as well to cover those cases where it's called directly. I wanted to also wrap the whole thing in a begin edit/commit wrapper, but that doesn't work for Split which is edited and committed by its parent transaction.

I made the actual changes in merge commits from merging master into private-kvp, but following the recommendations in https://www.kernel.org/pub/software/scm/git/docs/git-rerere.html and having been yelled at at length on the gtk-devel list a few years ago for pushing too many merge commits I used rerere to push all of those commits into the merge commit. Mike's concern that something like that should be explicitly called out in a separate commit didn't even occur to me. I was more concerned about keeping git from turning my dozen or so incremental changes into a single giant commit.

It may well have been that back-merging with rerere that ate some of the changes on the master side of the merge. I'll have to study the differences between Geert's merge commit and mine to figure out what was different and why.

I'm still not convinced that git can safely merge a long running branch in the face of the substantial divergence that we had in this case without special and careful handling. I *am* convinced that the several days work I did to make the merge work at all wasn't sufficient, but I don't yet understand why not, nor do I understand why Geert was able to get it to merge correctly when I failed.

Thanks, Geert, for cleaning up my mess for me.

Regards,
John Ralls