Skip to content

Commit

Permalink
SCUMM: Fix compilation
Browse files Browse the repository at this point in the history
  • Loading branch information
@sev-
sev- committed Aug 6, 2011
1 parent baf65bc commit 192b245
Show file tree
Hide file tree
Showing 2 changed files with 2 additions and 2 deletions.
2 changes: 1 addition & 1 deletion engines/scumm/scumm.cpp
View file
Expand Up @@ -1754,7 +1754,7 @@ void ScummEngine::setupMusic(int midi) {
_sound->_musicType = MDT_PCJR;
break;
case MT_CMS:
_musicType = MDT_CMS;
_sound->_musicType = MDT_CMS;
break;
case MT_TOWNS:
_sound->_musicType = MDT_TOWNS;
Expand Down
2 changes: 1 addition & 1 deletion engines/scumm/sound.cpp
View file
Expand Up @@ -2106,7 +2106,7 @@ int ScummEngine::readSoundResourceSmallHeader(ResId idx) {
_fileHandle->read(_res->createResource(rtSound, idx, wa_size + 6), wa_size + 6);
}
return 1;
} else if (_musicType == MDT_CMS) {
} else if (_sound->_musicType == MDT_CMS) {
if (_game.features & GF_OLD_BUNDLE) {
bool hasAdLibMusicTrack = false;

Expand Down

8 comments on commit 192b245

@lordhoto
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guess this means we got some (more) non-compilable commits in our history now :-/. Maybe we should adapt our commit guidelines to mention if one rebased commits on top of a changed master, one should (at least) compile test all commits, so we won't have broken commits, since that is rather annoying for regression checks via git bisect. Of course doing a merge and then compile testing the merge commit and in case something does not work doing a conflict resoval might be another idea.

@sev-
Copy link
Member Author

@sev- sev- commented on 192b245 Aug 6, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean that I had to compile it all 12, or at least 6 times when the code changed?

And in the past we often had non-compilable commits, especially with the ports.

Thoughts?

@clone2727
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, or just have done a merge from the beginning and amended the merge commit with the compile fixes.

Now that we're using git, we shouldn't have non-compilable (local) commits anymore, and especially not from a rebase.

@lordhoto
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean that I had to compile it all 12, or at least 6 times when the code changed?

If you want to do a rebase then IMHO yes.

And in the past we often had non-compilable commits, especially with the ports.

Sure we had that, but we also used CVS in the past... ;-). IMHO this it is not a reason to add broken commits when it could be rather easily avoided (by say using a merge).

Thoughts?

Do a merge instead and include compile fixes into the merge. If you want to do a rebase, then IMHO it should be assured nothing is broken at the compilation level at least. Of course at a behavior level things with a rebase (and merge!) can be easily broken too. If you use a merge you can furthermore check whether it was broken in the original commit or only introduced when the merge happened, which you cannot do with a rebase. This is why I myself favor the merge solution, except for really trivial pushes of at most a very few commits.

@fuzzie
Copy link
Contributor

@fuzzie fuzzie commented on 192b245 Aug 6, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think amending the commit guidelines to request that people just do merges is a better idea than the (unrealistic, imo) idea that people are going to compile-test them all - which is why I tend to be discouraging when people bring up the idea of recommending the default 'gi pull' behaviour be rebase.

@lordhoto
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think amending the commit guidelines to request that people just do merges is a better idea than the (unrealistic, imo) idea that people are going to compile-test them all - which is why I tend to be discouraging when people bring up the idea of recommending the default 'gi pull' behaviour be rebase.

In fact I wouldn't say that compile-test them all is realistic, but IMHO it's the only real option when you do a rebase.

I also have to agree telling people to default configure pull to do a rebase is definitely not the way to go.

@zeldin
Copy link
Contributor

@zeldin zeldin commented on 192b245 Aug 7, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the right forum for this discussion? ;-)

I agree that (automatic) rebase should be used only for short commit sequences, but if you're doing something non-trivial requiring several commits, then you shouldn't be doing it on you master anyway. Create a feature branch. Doing the development on master and having git pull creating merges will just result in spurious merge commits when you want to test something unrelated on upstream, and then you'll push those resulting in a needlessly messy history. If you use a feature branch (non-tracking), it won't matter if pull is set to merge or rebase, you do the merges/rebases manually when the time is right.

@lordhoto
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the right forum for this discussion? ;-)

Probably it should be rather on -devel yes.

Please sign in to comment.