search

DavidKinder / Git

Fast Glulx interpreter, originally written by Iain Merrick.
MIT License
46 stars 13 forks source link

@saveundo does not return 1 if it cannot save #2

Closed curiousdannii closed 10 years ago

curiousdannii commented 10 years ago

If your game is too big for Git to save undos, @saveundo should return 1, but it doesn't. From the looks of the source code it isn't programmed to do that, the only return number it has is 0.

Also, I notice that gMaxUndoSize is set to 256Kb. That won't make your recent increases to the buffer size pointless will it? (I'm guessing that's the max size of an individual undo slot? Should it perhaps be increased too just in case?)

(Also also, when you said "Increased the default undo buffer size in all ports" you only changed code for Windows, had you already changed it for the others? Because it doesn't report failures I'm thinking I'll have to detect the version of Git in order to switch on Ultra Undo, and I'm wondering if 1.3.2 is safe or not.)

DavidKinder commented 10 years ago

I now have a patch for Git that means that @saveundo keeps at least one undo state around, even if that one state exceeds the undo cache size. As even in the current implementation that undo state is generated and discarded, this is an improvement - combined with the increased undo cache size this should banish such problems.

gMaxUndoSize is only initially set to 256Kb - during program initialization (in initUndo()) it's set to a value passed in from the main() entry point. For Windows this is 2Mb - I've checked that this is definitely working. The entry point code for other ports that are included in the git source distribution aren't on GitHub, which is an oversight on my part. I'll check them in when I have a chance.

Trying to auto-detect Git in Ultra Undo is probably not a good idea. If someone recompiles GIt 1.3.2 (or earlier) with a larger undo buffer than the default, then the existing undo mechanism will work with CM, and there's no way for your Git detection logic to know that. If it were me, I would change the test in Ultra Undo to do a @saveundo and then do an @undo. If the @undo fails you'll know you should turn on your undo mechanism, and if it succeeds the flow of control will just back to just after the @saveundo, but with a result_code of -1.

curiousdannii commented 10 years ago

That all makes sense thanks. And it will be much better to directly check whether the undo system is working... I had thought it might be too complex to test but thinking again now I don't think it will be.