Fix pasting multi-line text into commit message panel #4234

stefanhaller · 2025-02-05T11:22:29Z

PR Description

When pasting a multi-line commit message into the subject field of the commit editor, we would interpret the first newline as the confirmation for closing the editor, and then all remaining characters as whatever command they are bound to, resulting in executing all sorts of arbitrary commands.

Now we recognize this being a paste, and interpret the first newline as moving to the description.

Also, prevent tabs in the pasted content from switching to the respective other panel; simply insert four spaces instead, which should be good enough for the leading indentation in pasted code snippets, for example.

Finally, disable pasting text into non-editable views; my assumption is that this is always a mistake, as it would execute arbitrary commands depending on what's in the clipboard.

This depends on the terminal emulator supporting bracketed paste; I didn't find one on Mac that doesn't (I tested with Terminal.app, iTerm2, Ghostty, kitty, Alacritty, WezTerm, and VSCode's builtin terminal. It works well in all of them).

I couldn't get it to work in Windows Terminal though, and I don't understand why, as it does seem to support bracketed paste (it works in bash).

Fixes #3151
Fixes #4066
Fixes #4216

Please check if the PR fulfills these requirements

Cheatsheets are up-to-date (run go generate ./...)
Code has been formatted (see here)
Tests have been added/updated (see here for the integration test guide)
Text is internationalised (see here)
If a new UserConfig entry was added, make sure it can be hot-reloaded (see here)
Docs have been updated if necessary
You've read through your own file changes for silly mistakes etc

codacy-production · 2025-02-05T11:26:43Z

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
Report missing for `0b52429`¹	✅ 66.10%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (`0b52429`)	Report Missing	Report Missing	Report Missing
Head commit (`76924d8`)	52693	45630	86.60%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#4234)	59	39	66.10%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings Change summary preferences

_{Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more}

Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct. ↩

jesseduffield

Nice work, see comments

jesseduffield · 2025-02-06T22:16:46Z

pkg/gui/controllers/commit_description_controller.go

@@ -75,6 +75,22 @@ func (self *CommitDescriptionController) switchToCommitMessage() error {
 	return nil
 }

+func (self *CommitDescriptionController) handleTogglePanel() error {


Something that is not clear here is that opts.Config.Universal.TogglePanel defaults to tab. So a reader could get confused as to why we're conditionally inserting spaces. A comment should clear up that confusion.

Things probably get awkward when a user remaps these keys, and I'm not sure of a good way of dealing with that (doesn't need to be solved in this PR)

Ah interesting, I hadn't considered remapping keys at all. I added a condition and a comment in 8256ed2, please have a look.

jesseduffield · 2025-02-06T22:22:30Z

vendor/github.com/jesseduffield/gocui/gui.go

@@ -1311,6 +1311,10 @@ func (g *Gui) onKey(ev *GocuiEvent) error {
 	switch ev.Type {
 	case eventKey:

+		if g.IsPasting && ev.Key == KeyCtrlJ {


This deserves a code comment explaining what we're doing. Even reading the commit message, it's not actually clear to me how KeyCtrlJ and KeyCtrlM relate to \n and \r.

Added a comment in 0c0287c, does this help? It's still slightly mysterious to me, I tried to be honest about this in the comment.

jesseduffield · 2025-02-06T22:24:05Z

vendor/github.com/jesseduffield/gocui/tcell_driver.go

+	case *tcell.EventPaste:
+		return GocuiEvent{
+			Type:  eventPaste,
+			Start: tev.Start(),


Is the idea that there's two paste events: one for starting the paste and one for finishing the paste, and if it's the first kind then Start will be true? If so it would be good to add a comment to that affect against the GocuiEvent struct

Yes, that's true. I added a comment in 76924d8, is that enough? Feel free to push fixups if you want to further improve it.

jesseduffield · 2025-02-06T22:25:24Z

pkg/gui/controllers/commit_message_controller.go

@@ -140,6 +156,10 @@ func (self *CommitMessageController) setCommitMessageAtIndex(index int) (bool, e
 }

 func (self *CommitMessageController) confirm() error {
+	if self.c.GocuiGui().IsPasting {


I'd spell this out in a comment too i.e. the default keybinding for confirm is enter, and newlines are interpreted as enter presses when pasting.

Added a condition and a comment in 8256ed2, like for the other handlers.

Recognize bracketed paste events, and set a flag on Gui if a paste is active. Application code can use this to handle carriage returns or tabs differently when they come from a paste operation.

When pasting, Ghostty seems to convert all \n to \r for some reason. Convert them back to \n so that we can handle them normally. I have to admit that I'm confused as to why this is necessary. From what I understand from Ghostty's source code (e.g. ghostty-org/ghostty@010338354a0) it does this conversion only for non-bracketed paste mode, but I'm seeing it in bracketed paste mode, and it is only there that we convert it back.

My assumption is that pressing command-V on a non-editable view is always a mistake; it would execute arbitrary commands depending on what's in the clipboard, so prevent it.

When pasting a multi-line commit message into the subject field of the commit editor, we would interpret the first newline as the confirmation for closing the editor, and then all remaining characters as whatever command they are bound to, resulting in executing all sorts of arbitrary commands. Now we recognize this being a paste, and interpret the first newline as moving to the description. Also, prevent tabs in the pasted content from switching to the respective other panel; simply insert four spaces instead, which should be good enough for the leading indentation in pasted code snippets, for example.

stefanhaller added the bug Something isn't working label Feb 5, 2025

stefanhaller requested a review from jesseduffield February 5, 2025 11:22

This was referenced Feb 5, 2025

Pasting multiple lines into commit summary field executes commands #3151

Open

Freezes when pasting a string that includes a line breaks #4216

Open

jesseduffield requested changes Feb 6, 2025

View reviewed changes

stefanhaller added 7 commits February 7, 2025 09:39

[gocui] Add Gui.IsPasting

a2ed699

Recognize bracketed paste events, and set a flag on Gui if a paste is active. Application code can use this to handle carriage returns or tabs differently when they come from a paste operation.

[gocui] Don't allow pasting into non-editable views

bb576f6

My assumption is that pressing command-V on a non-editable view is always a mistake; it would execute arbitrary commands depending on what's in the clipboard, so prevent it.

fixup! Handle pasting multi-line commit messages

8256ed2

fixup! [gocui] Work around Ghostty quirk with pasted newlines

0c0287c

fixup! [gocui] Add Gui.IsPasting

76924d8

stefanhaller force-pushed the bracketed-paste branch from 943d78f to 76924d8 Compare February 7, 2025 09:18

stefanhaller mentioned this pull request Feb 7, 2025

Bracketed paste support on Windows gdamore/tcell#777

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix pasting multi-line text into commit message panel #4234

Fix pasting multi-line text into commit message panel #4234

stefanhaller commented Feb 5, 2025

codacy-production bot commented Feb 5, 2025 •

edited

Loading

jesseduffield left a comment

jesseduffield Feb 6, 2025

stefanhaller Feb 7, 2025

jesseduffield Feb 6, 2025

stefanhaller Feb 7, 2025

jesseduffield Feb 6, 2025

stefanhaller Feb 7, 2025

jesseduffield Feb 6, 2025

stefanhaller Feb 7, 2025

Fix pasting multi-line text into commit message panel #4234

Are you sure you want to change the base?

Fix pasting multi-line text into commit message panel #4234

Conversation

stefanhaller commented Feb 5, 2025

codacy-production bot commented Feb 5, 2025 • edited Loading

Coverage summary from Codacy

Footnotes

jesseduffield left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codacy-production bot commented Feb 5, 2025 •

edited

Loading