[Scummvm-devel] Re: [Scummvm-cvs-logs] CVS: scummvm/simon simon-md5.h,NONE,1.7.2.1

[Max Horn]

Am 14.08.2004 um 16:26 schrieb Travis Howell:

> Max Horn wrote:
>> Am 14.08.2004 um 09:13 schrieb Travis Howell:
>>

[...]

> I'm getting a bit lost here, some of the changes you mention aren't 
> recent at all:

I think I only used "recent" to refer to the backports -> the changes 
may not be new on the trunk, but they still are "recent backports".



> Overall quite minor changes, none of those should cause any 
> regressions. They just made for one large change when back porting to 
> 0.6.0 branch.

The changes didn't seem that minor when I reviewed them. OTOH, it is's 
pretty tough to review one monolithic checkin with dozen changes (it 
also makes regression testing much harder).
[Off-topc: This is why I myself normally try to make small checkins, 
one for each change. Not always possible, of course]

I probably overreacted because of this - sorry for that :-/. But I 
still can't see for some of the changes how they affect the games, and 
while you say you checked disasm, I have no way to verify that they 
don't cause major regressions. As such, I react negative to them -- and 
I still don't feel comfortable with them. Quite frankly, I wouldn't 
want to make a release with the current 0.6.x branch... But maybe I am 
just skimpish -- Ender didn't hesitate to add that extrapath stuff to 
the 0.6.x branch either -- though were that led us is another story... 
<shrug>



Anyway, if you want to know some good rules how to backport changes 
without Max getting a fit over it, here's my take on what qualifies a 
change for backporting (rules of the thumb, there are always 
exceptions).


* "I can mathematically prove the change won't cause regressions"
Gee, check it in, a no-brainer. Sadly, I've never seen the like :-)

* "I did play through the following N games and exercised the change; 
my friend did the same on another platform; no regressions turned up"
Wow. That's about the best support you can get, I guess. Sadly one 
rarely has it :-/. Hard to counter that, though :-)

* The change is extremely localized in its effect. For example it only 
is enabled in one specific script in one particular game etc. -> not 
quite a proof of correctness, but when it's easy to see and understand 
how a change works and what effects it will have, then it's easy to get 
it accepted into the release branch.

* Big changes need big reviews -- break down big patches into multiple 
smaller independent parts, if possible. That will also make regression 
testing either (instead of "ok, one of the 37 changes in this commit 
broke things" versus "this small commit broke things, it's easy to see 
that the change in line 3 is at fault")

* The change only refactors things / renames vars / changes indention / 
etc.
  -> those changes don't affect program behavior, so they can go in.

* It doesn't matter much how long a given change has been in the trunk. 
Of course the longer it has been in, the more likely any regressions 
have been discovered. But since we didn't do any explicit QA session on 
the trunk recently, it very well may be that the code in question has 
not been exercised much. Conclusion: the value of this criterion is 
relatively small.

* "The [...] changes [...] are straight from [...] disasm".
It's a good starting place, but not a guarantee for correctness, either
- our code base differs in many subtle or not so subtle ways from the 
original engine. By directly emulating it, you can easily *break* 
things instead of fixing them
- the claim "I checked all places in the disassembly" is easy to make, 
but as easily turns out to be wrong :-).

* It fixes a major regression (crash, data corruption, bug which makes 
a game uncompleteable etc.):
That's not a free-ride for backport, esp. if the patch is big/hackish, 
but it gives very very strong support, of course



> I think having save game versions in sync is always worth it, when 
> possible. Since many users use CVS version and are annoyed to find the 
> next ScummVM version released isn't compatbile with the saved games.

Obviously that logic only works should we ever make a 0.6.2 release, 
since the problem you describe of course isn't solved by it for now -- 
0.6.1b still uses the "old" format.



Bye,

Max