Fixing a bug in Moodle core: the mechanics

Several people at work have asked me about this, so I thought I would write it out as a blog post. In this post, I want to focus on the mechanics of using git and the Moodle tracker to prepare a bug-fix and submit it. Therefore I need a really simple bug. Fortunately one was reported recently: MDL-32039. If you go and look at that, you can see that the mistake is that I failed to write coherent English when working on the code to upgrade from Moodle 2.0 to 2.1.

What we need to do

This bug was introduced in Moodle 2.1, and affects every version since then. Since it is a bug, it needs to be fixed in all supported versions, which means on the 2.1 and 2.2 stable branches, and on the master branch.

My development set-up

I need to fix then test code on three branches. The way I handle this is to have a separate install of Moodle for each stable branch, and one for master.

I use Eclipse as my IDE, so these three copies of Moodle are separate projects in my Eclipse workspace .../workspace/moodle_head, .../workspace/moodle_22, and .../workspace/moodle_21. Each of these folders is a git repository. In each repositor, I have two remotes set up, for example:

timslaptop:moodle_head tim$ git remote -v
moodle git://git.moodle.org/moodle (fetch)
moodle git://git.moodle.org/moodle (push)
origin git@github.com:timhunt/moodle.git (fetch)
origin git@github.com:timhunt/moodle.git (push)

One is called moodle and points to the master copy of the code on moodle.org. The other is called origin and points to my area on github, where I publish my changes.

In order to test the code, I have three different Moodle installs, each pointing at one of these copies of the code. Each install uses an different database prefix in config.php, so they can share one database.

Getting ready to fix the bug on the master branch

The way I normally work is to fix the bug on the master branch first, and then transfer the fix to previous branches.

The first thing I need to do is to make sure my master branch is up to date, which I do using git:

timslaptop:workspace tim$ cd moodle_head
timslaptop:moodle_head tim$ git fetch moodle
remote: Counting objects: 1442, done.
remote: Compressing objects: 100% (213/213), done.
remote: Total 817 (delta 624), reused 790 (delta 602)
Receiving objects: 100% (817/817), 197.21 KiB | 111 KiB/s, done.
Resolving deltas: 100% (624/624), completed with 225 local objects.
From git://git.moodle.org/moodle
   8925a12..09f011a  MOODLE_19_STABLE -> moodle/MOODLE_19_STABLE
   1cd62bf..a280d40  MOODLE_20_STABLE -> moodle/MOODLE_20_STABLE
   a7899ca..c54172b  MOODLE_21_STABLE -> moodle/MOODLE_21_STABLE
   a81e8c4..58db57a  MOODLE_22_STABLE -> moodle/MOODLE_22_STABLE
   c856a1f..a280078  master     -> moodle/master
timslaptop:moodle_head tim$ git checkout master
Switched to branch 'master'
timslaptop:moodle_head tim$ git merge --ff-only moodle/master
Updating c856a1f..a280078
Fast-forward
 ... lots diff --stat output ...
timslaptop:moodle_head tim$ git push origin master
Everything up-to-date

(Why is everything already up-to-date on github? because I was fixing bugs at work today, and so had already updated my github space from there.)

Since there were updates, I now need to go to http://localhost/moodle_head/admin/index.php and let Moodle upgrade itself.

Fixing the bug on the master branch

First, I want to create a new branch for this bug fix, starting from where the master branch currently is. My convention is to use the issue id as the branch name, so:

timslaptop:moodle_head tim$ git checkout -b MDL-32039 master
Switched to a new branch 'MDL-32039'

Other people use other conventions. For example they might call the branch MDL-32039_qeupgradehelper_typos. That is a much better name. It helps you see immediately what that branch is about, but I am too lazy to type long names like that.

To fix this bug it is just a matter of going into admin/tool/qeupgradehelper/lang/en/tool_qeupgradehelper.php and editing the two strings that were wrong.

Except that, if I screwed up those two strings, it is quite likely that I made other mistakes nearby. I therefore spent a bit of time proof-reading all the strings in that language file (it is not very long). That was worthwhile. I found and fixed two extra typos. This sort of thing is always worth doing. When you see one bug report, spend a bit of time thinking about and checking whether other similar things are also broken.

OK, so here is the bug fix:

timslaptop:moodle_head tim$ git diff -U1
diff --git a/admin/tool/qeupgradehelper/lang/en/tool_qeupgradehelper.php b/admin
index 3010666..7bd7c13 100644
--- a/admin/tool/qeupgradehelper/lang/en/tool_qeupgradehelper.php
+++ b/admin/tool/qeupgradehelper/lang/en/tool_qeupgradehelper.php
@@ -50,3 +50,3 @@ $string['gotoresetlink'] = 'Go to the list of quizzes that can
 $string['includedintheupgrade'] = 'Included in the upgrade?';
-$string['invalidquizid'] = 'Invaid quiz id. Either the quiz does not exist, or 
+$string['invalidquizid'] = 'Invalid quiz id. Either the quiz does not exist, or
 $string['listpreupgrade'] = 'List quizzes and attempts';
@@ -57,5 +57,5 @@ $string['listtodo_desc'] = 'This will show a report of all the
 $string['listtodointro'] = 'These are all the quizzes with attempt data that st
-$string['listupgraded'] = 'List already upgrade quizzes that can be reset';
+$string['listupgraded'] = 'List already upgraded quizzes that can be reset';
 $string['listupgraded_desc'] = 'This will show a report of all the quizzes on t
-$string['listupgradedintro'] = 'These are all the quizzes that have attempts th
+$string['listupgradedintro'] = 'These are all the quizzes that have attempts th
 $string['noquizattempts'] = 'Your site does not have any quiz attempts at all!'
@@ -82,2 +82,2 @@ $string['upgradedsitedetected'] = 'This appears to be a site t
 $string['upgradedsiterequired'] = 'This script can only work after the site has
-$string['veryoldattemtps'] = 'Your site has {$a} quiz attempts that were never 
+$string['veryoldattemtps'] = 'Your site has {$a} quiz attempts that were never

(Note that:

• I would not normally use the -U1 option. That just makes the output smaller, for the benefit of this blog post.
• The diff is chopped off at 80 characters wide, which is the size of my terminal window.)

Now I need to test that the fix actually works. I go to Site administration ▶ Question engine upgrade helper in my web browser, and verify that the strings now look OK.

OK, so I have a good bug-fix and I need to commit it:

timslaptop:moodle_head tim$ git add admin/tool
timslaptop:moodle_head tim$ git status
# On branch MDL-32039
# Changes to be committed:
#   (use "git reset HEAD ..." to unstage)
#
# modified:   admin/tool/qeupgradehelper/lang/en/tool_qeupgradehelper.php
#
timslaptop:moodle_head tim$ git commit -m "MDL-32039 qeupgradehelper: fix typos in the lang strings"
[MDL-32039 9e45982] MDL-32039 qeupgradehelper: fix typos in the lang strings
 1 files changed, 4 insertions(+), 4 deletions(-)

Notice that I followed the approved style for Moodle commit comments. First the issue id, then a brief indication of which part of the code is affected, then a colon, then a brief summary of what the fix was. This first line of the commit comment is meant to be less than about 70 characters long, which can be a challenge!

If this had been a more complex fix, I would probably have added some additional paragraphs to the commit comment to explain things (and so I would have typed the comment in my editor, rather than giving it on the command-line with the -m option). In this case, however, the one line commit comment says enough.

Now I need to publish this change to github so others can see it:

timslaptop:moodle_head tim$ git push origin MDL-32039
Counting objects: 15, done.
Delta compression using up to 2 threads.
Compressing objects: 100% (7/7), done.
Writing objects: 100% (8/8), 653 bytes, done.
Total 8 (delta 5), reused 0 (delta 0)
To git@github.com:timhunt/moodle.git
 * [new branch]      MDL-32039 -> MDL-32039

Now I can go to a URL like https://github.com/timhunt/moodle/compare/master...MDL-32039, and see the bug-fix through the github web interface.

More to the point, I can go to the tracker issue, click Request peer review, and fill in the details of this git branch, including that compare URL.

If this was a complex fix, I would then want for someone else to review the changes and confirm that they are OK. In this case, however, the fix is simple and I will just carry on without waiting for a review.

Transferring the fix to the 2.2 stable branch

So, now I want to apply the same fix to my moodle_22 code. First I need to update that install. Since this is similar to what we did above to update master, I will not show the output of these commands, just what I typed:

timslaptop:moodle_head tim$ cd ../moodle_22
timslaptop:moodle_22 tim$ git fetch moodle
timslaptop:moodle_22 tim$ git checkout MOODLE_22_STABLE
timslaptop:moodle_22 tim$ git merge --ff-only moodle/MOODLE_22_STABLE
timslaptop:moodle_22 tim$ git push origin MOODLE_22_STABLE

Then I visit http://localhost/moodle_22/admin/index.php to complete the upgrade.

(This may seem a bit laborious, but look out for a future blog post where I intend to talk about how I automate some of this. I only typed out the commands in full this time because I was writing this blog post.)

I want to apply the bug-fix I did on master on top of MOODLE_22_STABLE, and fortunately the command git cherry-pick is designed to do exactly that. (Since we are back in new territory, I will start showing the output of commands again.)

timslaptop:moodle_22 tim$ git fetch -p origin
remote: Counting objects: 1138, done.
remote: Compressing objects: 100% (118/118), done.
remote: Total 586 (delta 451), reused 569 (delta 434)
Receiving objects: 100% (586/586), 189.65 KiB, done.
Resolving deltas: 100% (451/451), completed with 176 local objects.
From github.com:timhunt/moodle
 * [new branch]      MDL-32039  -> origin/MDL-32039
   2117dcb..a280078  master     -> origin/master
timslaptop:moodle_22 tim$ git checkout -b MDL-32039_22 MOODLE_22_STABLE
Switched to a new branch 'MDL-32039_22'
timslaptop:moodle_22 tim$ git cherry-pick origin/MDL-32039
[MDL-32039_22 2c92dc7] MDL-32039 qeupgradehelper: fix typos in the lang strings
 1 files changed, 4 insertions(+), 4 deletions(-)
timslaptop:moodle_22 tim$ git push origin MDL-32039_22
Counting objects: 15, done.
Delta compression using up to 2 threads.
Compressing objects: 100% (6/6), done.
Writing objects: 100% (8/8), 662 bytes, done.
Total 8 (delta 5), reused 3 (delta 1)
To git@github.com:timhunt/moodle.git
 * [new branch]      MDL-32039_22 -> MDL-32039_22

Notice that the convention I use is to append _22 to the branch name to distinguish the branch for Moodle 2.2 stable from the branch for master. Other people use different conventions, but this one is simple and works for me.

Of course, in the middle of that, I checked that the fix actually worked in Moodle 2.2. In this case, there is not much to worry about, but with more complex changes, you really have to check. For example the fix you did on the master branch might have used a new API that is not available in Moodle 2.2. In that case, you would have had to redo the fix to work on the stable branch.

Transferring the fix to the 2.1 stable branch

Now I rinse and repeat for the 2.1 branch. (I will supress the command output again, until the last command, when something interesting happens.)

timslaptop:moodle_22 tim$ cd ../moodle_21
timslaptop:moodle_21 tim$ git fetch moodle
timslaptop:moodle_21 tim$ git checkout MOODLE_21_STABLE
timslaptop:moodle_21 tim$ git merge --ff-only moodle/MOODLE_21_STABLE
timslaptop:moodle_21 tim$ git push origin MOODLE_21_STABLE

timslaptop:moodle_21 tim$ git fetch -p origin
timslaptop:moodle_21 tim$ git checkout -b MDL-32039_21 MOODLE_21_STABLE
timslaptop:moodle_21 tim$ git cherry-pick origin/MDL-32039
error: could not apply 9e45982... MDL-32039 qeupgradehelper: fix typos in the lang strings
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add ' or 'git rm '
hint: and commit the result with 'git commit'

So, git cherry-pick could not automatically apply the bug fix. To see what is going on, I use git status to get more information:

timslaptop:moodle_21 tim$ git status
# On branch MDL-32039_21
# Unmerged paths:
#   (use "git add/rm ..." as appropriate to mark resolution)
#
# deleted by us: admin/tool/qeupgradehelper/lang/en/tool_qeupgradehelper.php
#
no changes added to commit (use "git add" and/or "git commit -a")

That may or may not make things clear. Fortunately, I know the history behind this. What is going on here is that in Moodle 2.1, this code was in local/qeupgradehelper, and in Moodle 2.2 it moved to admin/tool/qeupgradehelper, and this confuses git. Therefore, I will have to sort things out ourselves.

In this case, we can just move the altered file to the right place

timslaptop:moodle_21 tim$ mv admin/tool/qeupgradehelper/lang/en/tool_qeupgradehelper.php local/qeupgradehelper/lang/en/local_qeupgradehelper.php

Then use git diff to check the changes are just what we expect:

timslaptop:moodle_21 tim$ git diff -U1
diff --git a/local/qeupgradehelper/lang/en/local_qeupgradehelper.php b/local/qeu
index ac883b5..7bd7c13 100644
--- a/local/qeupgradehelper/lang/en/local_qeupgradehelper.php
+++ b/local/qeupgradehelper/lang/en/local_qeupgradehelper.php
@@ -19,3 +19,3 @@
  *
- * @package    local
+ * @package    tool
  * @subpackage qeupgradehelper
@@ -50,3 +50,3 @@ $string['gotoresetlink'] = 'Go to the list of quizzes that can
 $string['includedintheupgrade'] = 'Included in the upgrade?';
-$string['invalidquizid'] = 'Invaid quiz id. Either the quiz does not exist, or 
+$string['invalidquizid'] = 'Invalid quiz id. Either the quiz does not exist, or
 $string['listpreupgrade'] = 'List quizzes and attempts';
@@ -57,5 +57,5 @@ $string['listtodo_desc'] = 'This will show a report of all the
 $string['listtodointro'] = 'These are all the quizzes with attempt data that st
-$string['listupgraded'] = 'List already upgrade quizzes that can be reset';
+$string['listupgraded'] = 'List already upgraded quizzes that can be reset';
 $string['listupgraded_desc'] = 'This will show a report of all the quizzes on t
-$string['listupgradedintro'] = 'These are all the quizzes that have attempts th
+$string['listupgradedintro'] = 'These are all the quizzes that have attempts th
 $string['noquizattempts'] = 'Your site does not have any quiz attempts at all!'
@@ -82,2 +82,2 @@ $string['upgradedsitedetected'] = 'This appears to be a site t
 $string['upgradedsiterequired'] = 'This script can only work after the site has
-$string['veryoldattemtps'] = 'Your site has {$a} quiz attempts that were never 
+$string['veryoldattemtps'] = 'Your site has {$a} quiz attempts that were never 

Actually, you can see that there is one wrong change there (the change to @package, so I need to undo that. The easy way to undo that would be to edit the file in Eclipse, but I want to show off another git trick:

timslaptop:moodle_21 tim$ git checkout -p local
diff --git a/local/qeupgradehelper/lang/en/local_qeupgradehelper.php b/local/qeupgradehelper/lang/en/local_qeupgradehelper.php
index ac883b5..7bd7c13 100644
--- a/local/qeupgradehelper/lang/en/local_qeupgradehelper.php
+++ b/local/qeupgradehelper/lang/en/local_qeupgradehelper.php
@@ -17,7 +17,7 @@
 /**
  * Question engine upgrade helper langauge strings.
  *
- * @package    local
+ * @package    tool
  * @subpackage qeupgradehelper
  * @copyright  2010 The Open University
  * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
Discard this hunk from worktree [y,n,q,a,d,/,j,J,g,e,?]? y
@@ -48,16 +48,16 @@
 ... lots more diff output here ...
Discard this hunk from worktree [y,n,q,a,d,/,K,j,J,g,s,e,?]? q

Now, git diff will confirm that the change is just what we want, so we can test the change, and then finish up. (output suppressed again):

timslaptop:moodle_21 tim$ git add local
timslaptop:moodle_21 tim$ git commit
timslaptop:moodle_21 tim$ git push origin MDL-32039_21

Submitting the fix for integration

Now I have tested versions of the fix on all three of the branches where the bug needed to be fixed. So, I can go back to the Tracker issue and submit it for integration.

When I get to the bug, I see that Jim Tittsler, who reported the bug, has seen my request for peer review, and added a comment:

Fantastic! It really makes a difference when people follow-up on the bugs they report, and supply extra information, or just say thank you. In this case, although I intended to carry on without a peer reveiw, I got one.

Now I press the Submit for integration... button, and fill in the fix version, the details of the other branches, and most important, we write some testing instructions, so that someone else can test the bug next Wednesday as part of the integration process.

Finally, we are done. Now we sit back and wait for next Monday, when the weekly integration cycle starts. Our change will be reviewed, tested, and, all being well, included in the next weekly build of Moodle.

Reflection

Is this an overly laborious process? Well, it is if you try to describe every detail in a blog post! In normal circumstances, however, it really does not take long. In normal circumstances I could probably have done this fix in ten to fifteen minutes.

What usually takes the time is thinking about the problem, and writing and testing the code. This takes much longer than typing the git commands and completing the tracker issue. Writing the testing instructions can be laborious, particularly if it is a complex issue, but that is normally time well spent. It forces you to think carefully about the changes you have made, and what needs to be done to verify that they fix the bug that was reported without breaking anything else. As I said in my previous blog post, I think testing instructions are a really good discipline.

I hope this rather long blog post was interesting, or at least useful to somebody.