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

Max Horn max at quendi.de
Thu Jun 2 13:29:02 CEST 2011


Am 01.06.2011 um 18:38 schrieb Marcus Comstedt:

> 
> Max Horn <max at quendi.de> writes:
> 
> [...]
>> While I am at it: What I would have disliked about manually cherry picking from the release branch to stable:
>> (a) I would have to find all relevant commits manually (not *that* much work with git, but still considerably more work than what I had to invest now). 
>> (b) Since I would have transplanted commits of people, taking them out of context, there is a fair likelihood for messing something up (esp. for a multi-commit series). E.g. if I forgot one of five commmits, then this is not visible in the history; but "git blame" would lead to the original author. 
> 
> Yes, as mentioned in the other thread, rebase -i provides a better
> interface here, since it lists all unmerged commits, and allows you to
> remove those you don't want, and rearrange or even combine related
> commits, without the risk of forgetting some commit.

Thanks Marcus, good suggestion. To be honest, it never occurred to me to use "rebase -i" for this, but it certainly makes some sense! So I wanted to give this approach a try. After some initial confusion (mostly caused by me taking Marcus' mentioning of "--onto" in the other thread too literal ;), I ended up doing this:

1) I made a new test-branch "master-old" on commit a4d105c9, the commit just before I did the merge

2) Likewise, I made a copy "branch-1-3-0-old" of "branch-1-3-0", just to make sure I don't mess up my repository ;).

3) I checked out branch-1-3-0-old, and then run
  git rebase -i master-old

Now, this presented me with the following list of commits (read on below): 

pick 40df14a RELEASE: Tag branch-1-3-0 and set version
pick e467468 DC: Disable serial for release
pick cf41ac0 ANDROID: Disable the savegame parachute
pick 066b388 BUILD: Disable OpenGL backend code.
pick 92a71f7 DS: Port of changes from branch-1-2-0 that I should really have moved into the trunk.
pick a9b5d5e DS: Fix some OPL data which was incorrectly freed from the main heap on the DS port
pick 987d966 DS: Prevent arrays from growing by 32 elements inside 'operator='.  I'm concerned that this could increase memory usage on the DS, but too scared to make the change for all builds of ScummVM.
pick 6fdec4d DS: Various changes - Enable libmad - Remove forced include of scummsys.h, it caused problems in the forbidden symbols code.
pick 3ce4b76 DS/SAGA: Due to what looks like a compiler bug, having one Common::Array template inside another causes the DS build to crash during Common::Array::resize().  The only fix I can find is to make the internal byte array a normal malloc'ed() buffer.  This way, the code runs fine.  Need to dig into the assembly output for this to find out what's truly going on with the original code.
pick f190300 GUI: Prevent the GUI code from incorrectly reloading the theme when the builtin theme is used.
pick 4076a04 GUI/DS: Make 99 the maximum number of save slots displayed on the GMM load/save screens on the DS port.
pick e7a006f SCI: Hardcode parser output in one case in QfG2 to fix #3288328
pick 8fb0d27 SCI: Replace QfG2 parser hack with simpler variant
pick 793849c TINSEL: Fix bug #3306020, DW2: Crash On Entering Sewers
pick 7a9fe3f BUILD: Add msvc8/9/10 build files.
pick 96519ad PS2 backend: Modified and tidied up Makefiles for 1.3.0 release
pick 48b4c6c SAMSUNGTV: Update port
pick 725db14 SAMSUNGTV: Fix build on non-SDL platforms
pick 709f0de SAMSUNGTV: Fix build
pick 2b11228 I18N: Update translations.dat
pick 1c2e531 RELEASE: Specified release date for 1.3.0
pick 625f6d3 WINCE: Update of port-related README
pick e42e83d CREATE_PROJECT: Disable PNG, Theora and OpenGL by default.
pick afe1a77 VS: Disabled libpng, libtheora and opengl in the VS solution files

# Rebase a4d105c..afe1a77 onto a4d105c
...


The first problem now was / is that I had to review each of these commits. Even if the commit name suggests that it should be fine to merge or to skip, I just can't rely on that. So I decided to use github.com's compare view to aid me, and I opened up this in my browser:

  <https://github.com/scummvm/scummvm/compare/a4d105c...afe1a77>

At first I was confused because this showed me 80 commits instead of the 24 git listed. But I quickly realized that this was because it ignored all the commits we cherry-picked from master to the release branch. Nifty!


So I proceeded to go through the list of commits, reviewing each using github.com. It all went fine, until I reached:
  pick e7a006f SCI: Hardcode parser output in one case in QfG2 to fix #3288328
which github.com compare view did not list. My guess is that this accidental merge commit is the culprit: <https://github.com/scummvm/scummvm/commit/92d0216db18c96c97bb4b57ada76cf532838cf5b>. Alas, nothing to worry about, I can still review commits on github.com as long as I have their commit id (of course I can also still use git from the command line. but it's nice to also see any discussions that took place on a commit, etc.).

Anyway, e7a006f turned out to be a "stop gap measure" for the release branch, so I knew I could drop it and also 8fb0d27 following it.

One major difference compare to what I did with the "merge": This time I just dropped the DC "noserial" commit, instead of replacing it by commented out code. That might be better, or worse, depending on how you look at it. Likewise, I had to decide what to do with the PS2 tweaks. For this try, to keep things simple, I decided to also just drop the PS2 commit. But of course I could also have kept it, or edited it (the later to match what I did for the merge).

I ended up with this selection of commits for the rebase command:

pick 92a71f7 DS: Port of changes from branch-1-2-0 that I should really have moved into the trunk.
pick a9b5d5e DS: Fix some OPL data which was incorrectly freed from the main heap on the DS port
pick 6fdec4d DS: Various changes - Enable libmad - Remove forced include of scummsys.h, it caused problems in the forbidden symbols code.
pick 3ce4b76 DS/SAGA: Due to what looks like a compiler bug, having one Common::Array template inside another causes the DS build to crash during Common::Array::resize().  The only fix I can find is to make the internal byte array a normal malloc'ed() buffer.  This way, the code runs fine.  Need to dig into the assembly output for this to find out what's truly going on with the original code.
pick f190300 GUI: Prevent the GUI code from incorrectly reloading the theme when the builtin theme is used.
pick 4076a04 GUI/DS: Make 99 the maximum number of save slots displayed on the GMM load/save screens on the DS port.
pick 725db14 SAMSUNGTV: Fix build on non-SDL platforms
pick 709f0de SAMSUNGTV: Fix build
pick 625f6d3 WINCE: Update of port-related README

I then had to clean up a few conflicts, but all were pretty easy to handle.


4) Finally, I had a "branch-1-3-0-old" branch that now contained master-old plus select changes from branch-1-3-0. I now could have moved the "master(-old)" branch up to this new head, and called this the complete "merge" (well, rebase ;).

You can view the result here: <https://github.com/fingolfin/scummvm/commits/branch-1-3-0-old>

A final "diff" against a4610df (my merge commit) revealed only the expected differences: The PS2 and DC release-only stuff as well as the Android JNI "stable only" is gone, as are some "nice" PS2 changes (because they were mixed up with the release-only changes, and I decided not to edit the commit).



Summary: All in all, this also went pretty smooth for me. I had to do some extra work, because I had to review individual commits. But since it was clever about cherry picks, that already took care of the majority. Plus, looking at individual commits helped me notice that the Android change should *not* be merged / cherry-picked / rebased. Using "git blame" works fine now, and on github it is *also* clearly visible that I cherry-picked/rebased those commmits, and when. In the end, I actually had to do less merging work overall, because I could just skip the version change and MSVC project file commits. And there were no conflicting changes in configure, either (due to dropping the DC "noserial" changes).

One thing that this rebase'd branch is missing are the fixes for the bad whitespaces some commits introduced ;) but that's hardly an argument for anything.


All in all, I could most certainly live with this approach, too. It would allow us to keep cherry-picking fixes to the release branch as we did now.

We should then still think a little bit about how we handle "tagging" of the final release, esp. since in the past we had to re-tag very often, and that's not so easily done with git, I understand (because once you made a tag public, you don't really want to change it anymore. nor can you, really, right?!?)
However, this is really *independant* of the merge vs. rebase debate; but it *is* part of the larger "workflow" debate.


Cheers,
Max

Cheers,
Max






More information about the Scummvm-devel mailing list