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

[John Ralls]

On May 8, 2014, at 1:40 PM, Geert Janssens <janssens-geert at telenet.be> wrote:

> On Wednesday 07 May 2014 22:46:17 John Ralls wrote:
>> On May 7, 2014, at 5:08 PM, Geert Janssens <janssens-geert at telenet.be> 
> wrote:
>>> 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.
>> 
> From superficially reading through the commits it's unclear how commit 
> 1f3fbf4b could have compiled at all. You moved qof_instance_set_dirty to 
> qofinstance-p.h, but src/register/ledger-core/split-register-model-
> save.c still calls that function. It may be qofinstance-p.h gets 
> included via one or the other header file though. I haven't looked too 
> deeply. In any case I take from this your intention was clearly to make 
> qof_instance_set_dirty a private function.
> 
> But that's not the key point I want to argue here. I'm interested to get 
> to the bottom of why your merge went wrong. That could help us in the 
> future to avoid similar unexpected situations.
> 
>> 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.
>> 
> I did the merge the other way around. I merged private-kvp into master 
> and resolved the merge conflicts by reading the source files in private-
> kvp HEAD to learn what your intention was. There were 3 non-obvious 
> changes (the other conflicts were trivial):
> - some functions got moved into Account.c from import-match-map.c/h
>  from what I could see you didn't alter the functions themselves so
>  I could still drop the files without making tweaks to Account.c. This
>  is something that's not trivial to follow with git diff or friends.
> - one merge resulted in declarations to follow code. I think this
>  was in a merge conflict but I hadn't spotted it while resolving the
>  conflict
> - one extern "C" block had to be moved down manually way below a newly
>  added set of function declarations.
> 
> And I hope I didn't miss any more subtle changes. I was worried about 
> the file name changes in qof (qofbook.c->qofbook.cpp for example) but 
> apparently git managed that part just fine.
> 
>> 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 wonder indeed if rerere did something odd here or whether we're making 
> wrong assumptions on how it works. I'll wait for your analysis when you 
> get around to it.
>> 
>> 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.
>> 
> Perhaps a question for a git mailing list ?
> 
>> Thanks, Geert, for cleaning up my mess for me.
>> 
> You're welcome. I was too curious to let it pass and it was a good 
> opportunity to learn something about git merge in the process.
> 

The only actual damage seems to have been in src/report/report-gnome/gnc-plugin-page-report.c; at least that is the only file with significant diffs in `git diff 9d40512..49a5909`, which shows the net change from your reverting the merge, remerging, and re-doing the C++ fixups. All of the changes from this year were reversed in the original merge commit, so I must have messed up when handling the last or next-to-last back-merge and blithely picked the branch version for every diff. git rerere remembered and did the same when I did the forward-merge.

So I’m going to revise my earlier worry about git being able to safely merge a long running commit: I’m not sure *I* can safely merge a long-running and complex branch in the face of dozens of conflicts. I guess that’s an argument for merging frequently to keep the conflicts under control.

Regards,
John Ralls