Custom binding for key `p` is ignored when a commit has focus (behaviour reverts to `pull`)

[stephenmartindale]

Describe the bug

User customisation of the keybinding for p (lowercase) – i.e. using p for another action or unbinding p by mapping it to <disabled> – is ignored when the commit log is active.

Instead of obeying the user's keybindings, lazygit falls back to the default built-in behaviour which appears to be git pull --no-edit.

To Reproduce

1. Customise the key map, thus:

   keybinding:
     universal:
       pushFiles: <disabled>

2. In lazygit, focus the commit log and have a commit in the list selected.

3. Strike p (lowercase, no modifiers)

4. Observe that lazygit falls back to git pull --no-edit which is not the configured behaviour.

5. Change to the branches, remotes or tags pane and try striking p; observe that lazygit obeys its configuration.

Expected behavior

lazygit should heed user keybinding preferences – consistently – throughout the U.I.

Version info:

commit=, build date=, build source=unknown, version=0.51.1, os=linux, arch=amd64, git version=2.49.0

Additional Details

• If the commits window is not active (another lazygit pane has focus) then it appears that p will behave as configured.

• Mapping p to another action appears to exhibit the same incorrect behaviour. For example, I discovered this issue because I wished to swap p and P (to be push and pull, respectively, instead of the default) but this is presently impossible because the following does not work, consistently:

  keybinding:
    universal:
      pullFiles: p
      pushFiles: P

  • but this config does work for the branches pane, remotes or tags, too.

• The mapping for branches: pushTag: appears to have no bearing on this behaviour and mapping that to <disabled> does not resolve the issue. (grep found no other mentions of push or pull in the default keybindings as far as I could tell.)

[stefanhaller]

I'm a little confused by your report, it seems that you are swapping pullFiles and pushFiles in a few places.

Anyway, this is a well-known problem, the code is here.

We only just recently added a new mechanism that should allow solving this easily, I guess a fix might look as simple as this:

diff --git i/pkg/gui/controllers/local_commits_controller.go w/pkg/gui/controllers/local_commits_controller.go
index 2b6f3d5c0..d6d415775 100644
--- i/pkg/gui/controllers/local_commits_controller.go
+++ w/pkg/gui/controllers/local_commits_controller.go
@@ -623,9 +623,7 @@ func (self *LocalCommitsController) pick(selectedCommits []*models.Commit) error
 		return self.updateTodos(todo.Pick, selectedCommits)
 	}
 
-	// at this point we aren't actually rebasing so we will interpret this as an
-	// attempt to pull. We might revoke this later after enabling configurable keybindings
-	return self.pullFiles()
+	return gocui.ErrKeybindingNotHandled
 }
 
 func (self *LocalCommitsController) interactiveRebase(action todo.TodoCommand, startIdx int, endIdx int) error {

I need to test this a bit more, will make a PR tomorrow, hopefully.

[stefanhaller]

After looking closer, it turns out that the fix has to go a bit deeper than the simple patch above. See #4617 for a PR that should fix this properly. @stephenmartindale Would be good if you could test this.

[stephenmartindale]

After looking closer, it turns out that the fix has to go a bit deeper than the simple patch above. See #4617 for a PR that should fix this properly. @stephenmartindale Would be good if you could test this.

Thank you for that! I've been away from my desk (summer arrived, in Germany) but I'll definitely try to test out the latest code in the coming week and report my findings.

You intuited, correctly, that my intention was to swap p and P to be push and pull, respectively – I work largely on single dev. prototypes or research code-bases and push much more than pull.

I apologize for any modicum of confusion my issue report instilled – I was trying to be thorough and more general in order to be more helpful to the project in general – not just "get my use-case in". ;)

[stephenmartindale]

Thanks @stefanhaller , I've updated to 0.52.0 which includes #4617 and tested it – it appears to work as expected and I find that very satisfactory. (I'm also hugely appreciative of #4593 which is also in 0.52.0, for what it's worth. Thanks!)