[Scummvm-devel] Merged branch-1-3-0 into master

[Max Horn]

Am 01.06.2011 um 16:02 schrieb A. Milburn:

> On Wed, Jun 01, 2011 at 03:42:20PM +0200, Max Horn wrote:
>> I *hope* that I didn't mess up anything this time around, I really tried hard, but please everybody review and test this.
> 
> I'm a bit surprised this was pushed without review, given I've been
> expressing my concerns about it on-list.. we've gained an Android
> commit labelled stable-only and a nasty engine-specific hack for
> the DS, which is the kind of thing I am worried about (and the
> RELEASE_BUILD issue which you've already fixed). None of these are
> a big deal, of course, and as you say we have a lot of eyes to keep
> a look out.

You are right, I should probably have first pushed this into my private fork for review. My bad.

Now, let's go through the specific criticism:
1) the PS2 RELEASE_BUILD thing was very silly; but also would have easily been caught by review

2) the Android commit, stable only: I missed this one (or rather, i saw it, but didn't follow up the "git blame", which would have lead me to the commit message. I'll take care of it, thanks. Still, review obviously would have caught it (you did catch it, after all ;).

3) the DS "nasty hack": I picked this one consciously and on purpose. Yes, it is nasty, but it is also required for the NDS. And it's quite likely that it will still be required by the time 1.4.0 is released. At least, nobody has been working on an alternative "clean" solution for the problem, and I see no signs that this will change; instead, it's likely this problem would be forgotten over time. Therefore, it seems most logical to merge it in. If nobody touches the SAGA code anymore, there is no harm. If somebody is bothered by it, they might take it as an incentive to develop a better solution. The alternative would have been for Neil to re-apply the hack during the next release cycle; quite likely after first spending some "quality time" re-debuggint the problem before remembering the hack-fix that was made months before. 


Anyway, if that's all the problematic parts you found, then I for one am overall quite happy with the outcome. Except for that part that I indeed should have put this through review first. (Hm, can one have merges like that in a pull request? I hope so).

> 
>> For the PS2 and Dreamcast ports, there were changes that looked like they were meant only for the release builds. I moved this into commented out lines, with a comment "For release builds:" next to them.
> 
> But if the goal of this branching system is really to make history clearer,
> this kind of merging does not have that effect, imo - we now have a bunch
> of changes 'hidden' inside a merge commit.
> 
> I don't think this is anything to do with how you did it, but simply my
> issues with doing these stable->master merges at all.

I am not so sure, actually: Maybe my method was not optimal. Maybe it would have been better if I had first done the merge, and then had done minimal conflict resolution -- picking always either the HEAD or the branch version of some code. Then, in a separate following commit, i could have changed things to be as we want them on master.

So, the DC & PS2 "release only" changes would have been on master in the merge, and in the next commit(s) I would have changed them the way I now changed them with the merge commit. Likewise, I could have removed the MSVC project files with a separate commit. That way, "git blame" could then have been used to properly trace the origin of any change to either my merge commit, or a commit on the branch, or a commit on master.



> I really worry that
> we're going to encourage nasty hacks - which are only committed due to the
> need to have things work for the stable release - being committed to master
> because it's the 'right way' to do it (since simply reverting these fixes
> seems to be advised against?),

See above... Two mistakes, and one where I think there is no foreseeable better alternative... In these cases, I prefer a pragmatic approach. Wanting a "proper" and "correct" solution is noble and important, but sacrificing the ability to use an engine over that when a workaround is available seems wrong, even if it nasty; esp. if it is cleanly separated. 

> and that we're going to end up with
> unclear history because the only way to keep master relatively sane is to
> make a bunch of modifications in the merge commit itself.

I am not sure if it is necessary to have "master sane" at all times. With the above multi-step merge, it would still be buildable at all times, and since I would have pushed (resp. merged the pull request with the merge + cleanup commits) in one go, nobody would have actually used the intermediate "somewhat unclean" commits.

> 
> Having tried this and so being able to see the concrete results is a good
> way to actually see what the effects of these merges would be, though.

That was the main point of the experiment, actually. It's easier to see and understand the pros and cons of the whole thing with a concrete example. I think that part worked out as desired, too ;).


Cheers,
Max