Golang Gotcha of the Day

This one has bitten me in the ass probably three times in the last month. The most recent bite occured just last night as I was debugging the rebase logic in lazygit. I had noticed that after selecting the 'abort' option in my rebase menu the rebase was not aborting, but instead moved along a couple of commits and then got stuck at some more conflicts.

Were not all aborts created equal? Maybe sometimes in order to abort you need to go forward first, and git was hitting conflicts along the way? I wasn't quite ready to give up my understanding of how rebasing works, so I took a look at the code

func (gui *Gui) handleCreateRebaseOptionsMenu(g *gocui.Gui, v *gocui.View) error {
    options := []string{"continue", "abort"}

    if gui.State.WorkingTreeState == "rebasing" {
        options = append(options, "skip")
    }

    menuItems := make([]*menuItem, len(options))
    for i, option := range options {
        menuItems[i] = &menuItem{
            displayString: option,
            onPress: func() error {
                return gui.genericMergeCommand(option)
            },
        }
    }
    ...

Can you spot the bug?

The logic was simple: create a slice of strings representing the different options available, and then create menu items to show in the GUI, where if you pressed say 'abort', lazygit would run git rebase --abort .

I checked for an off-by-one error on the index: perhaps the menu items started counting from 1 instead of zero? Nope.

Turns out lazygit actually ran 'git rebase --skip', which explained why it would continue the rebase and then stop again. And then it struck me: skip was the last string in the slice. It just so happens that when a function inside a loop closes over a loop variable, it ends up referring to the final value of that variable, because on each iteration of the loop the loop variable's value is reassigned. In this case, the last value was the last item in the options slice, i.e. 'skip'.

So I added a variable which was scoped to the inside of the loop, to assign the value of the loop variable for that iteration. Unlike closures which work on references, direct assignments just take the value of the variable at the time of assignment, meaning its value wasn't going to change no matter what happened in future iterations.

func (gui *Gui) handleCreateRebaseOptionsMenu(g *gocui.Gui, v *gocui.View) error {
    options := []string{"continue", "abort"}

    if gui.State.WorkingTreeState == "rebasing" {
        options = append(options, "skip")
    }

    menuItems := make([]*menuItem, len(options))
    for i, option := range options {
        // note to self. Never, EVER, close over loop variables in a function
        innerOption := option
        menuItems[i] = &menuItem{
            displayString: innerOption,
            onPress: func() error {
                return gui.genericMergeCommand(innerOption)
            },
        }
    }

And voilà! The bug was fixed, and lazygit's questionable reputation as a reliable git GUI was preserved for another day.

Lesson learnt: Never, EVER, close over loop variables in a function definition!