From daf9b8cfa9213a791d10d41e8f00c61d21ac7aa4 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Tue, 19 Dec 2023 15:03:35 +0100 Subject: [PATCH 1/4] Simplify GetCommitMessage Use git log instead of git rev-list, this way we don't get a line "commit " at the beginning that we then have to discard again. The test TestGetCommitMsg is becoming a bit pointless now, since it just compares that input and output are identical. --- pkg/commands/git_commands/commit.go | 5 ++--- pkg/commands/git_commands/commit_test.go | 15 ++++++--------- 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/pkg/commands/git_commands/commit.go b/pkg/commands/git_commands/commit.go index 812d2f6b5..443289f6e 100644 --- a/pkg/commands/git_commands/commit.go +++ b/pkg/commands/git_commands/commit.go @@ -137,12 +137,11 @@ func (self *CommitCommands) signoffFlag() string { } func (self *CommitCommands) GetCommitMessage(commitSha string) (string, error) { - cmdArgs := NewGitCmd("rev-list"). + cmdArgs := NewGitCmd("log"). Arg("--format=%B", "--max-count=1", commitSha). ToArgv() - messageWithHeader, err := self.cmd.New(cmdArgs).DontLog().RunWithOutput() - message := strings.Join(strings.SplitAfter(messageWithHeader, "\n")[1:], "") + message, err := self.cmd.New(cmdArgs).DontLog().RunWithOutput() return strings.TrimSpace(message), err } diff --git a/pkg/commands/git_commands/commit_test.go b/pkg/commands/git_commands/commit_test.go index a6f887a12..0ade25a8d 100644 --- a/pkg/commands/git_commands/commit_test.go +++ b/pkg/commands/git_commands/commit_test.go @@ -260,19 +260,17 @@ func TestGetCommitMsg(t *testing.T) { scenarios := []scenario{ { "empty", - ` commit deadbeef`, + ``, ``, }, { "no line breaks (single line)", - `commit deadbeef -use generics to DRY up context code`, + `use generics to DRY up context code`, `use generics to DRY up context code`, }, { "with line breaks", - `commit deadbeef -Merge pull request #1750 from mark2185/fix-issue-template + `Merge pull request #1750 from mark2185/fix-issue-template 'git-rev parse' should be 'git rev-parse'`, `Merge pull request #1750 from mark2185/fix-issue-template @@ -285,7 +283,7 @@ Merge pull request #1750 from mark2185/fix-issue-template s := s t.Run(s.testName, func(t *testing.T) { instance := buildCommitCommands(commonDeps{ - runner: oscommands.NewFakeRunner(t).ExpectGitArgs([]string{"rev-list", "--format=%B", "--max-count=1", "deadbeef"}, s.input, nil), + runner: oscommands.NewFakeRunner(t).ExpectGitArgs([]string{"log", "--format=%B", "--max-count=1", "deadbeef"}, s.input, nil), }) output, err := instance.GetCommitMessage("deadbeef") @@ -306,15 +304,14 @@ func TestGetCommitMessageFromHistory(t *testing.T) { scenarios := []scenario{ { "Empty message", - oscommands.NewFakeRunner(t).ExpectGitArgs([]string{"log", "-1", "--skip=2", "--pretty=%H"}, "", nil).ExpectGitArgs([]string{"rev-list", "--format=%B", "--max-count=1"}, "", nil), + oscommands.NewFakeRunner(t).ExpectGitArgs([]string{"log", "-1", "--skip=2", "--pretty=%H"}, "", nil).ExpectGitArgs([]string{"log", "--format=%B", "--max-count=1"}, "", nil), func(output string, err error) { assert.Error(t, err) }, }, { "Default case to retrieve a commit in history", - oscommands.NewFakeRunner(t).ExpectGitArgs([]string{"log", "-1", "--skip=2", "--pretty=%H"}, "sha3 \n", nil).ExpectGitArgs([]string{"rev-list", "--format=%B", "--max-count=1", "sha3"}, `commit sha3 - use generics to DRY up context code`, nil), + oscommands.NewFakeRunner(t).ExpectGitArgs([]string{"log", "-1", "--skip=2", "--pretty=%H"}, "sha3 \n", nil).ExpectGitArgs([]string{"log", "--format=%B", "--max-count=1", "sha3"}, `use generics to DRY up context code`, nil), func(output string, err error) { assert.NoError(t, err) assert.Equal(t, "use generics to DRY up context code", output) From 9a423c388ddf7185a22d8515a37a6f9a16318d82 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Tue, 19 Dec 2023 16:40:06 +0100 Subject: [PATCH 2/4] Remove unused function I think this is a left-over from before we had the new commit message panel. It no longer makes sense to add a newline to the commit subject. --- pkg/integration/components/commit_message_panel_driver.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/pkg/integration/components/commit_message_panel_driver.go b/pkg/integration/components/commit_message_panel_driver.go index 52ad60815..b3dda6a04 100644 --- a/pkg/integration/components/commit_message_panel_driver.go +++ b/pkg/integration/components/commit_message_panel_driver.go @@ -38,12 +38,6 @@ func (self *CommitMessagePanelDriver) SwitchToDescription() *CommitDescriptionPa return &CommitDescriptionPanelDriver{t: self.t} } -func (self *CommitMessagePanelDriver) AddNewline() *CommitMessagePanelDriver { - self.t.press(self.t.keys.Universal.Confirm) - - return self -} - func (self *CommitMessagePanelDriver) Clear() *CommitMessagePanelDriver { // clearing multiple times in case there's multiple lines // (the clear button only clears a single line at a time) From 3ebba5f32cdd1549f032a68592928b23bbddf3f8 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Tue, 19 Dec 2023 16:44:16 +0100 Subject: [PATCH 3/4] Add test demonstrating a bug with preserving the commit message SplitCommitMessageAndDescription splits at the first '\n\n' that it finds (if there is one), which in this case is between the two paragraphs of the description. This is wrong. --- .../commit_description_panel_driver.go | 11 +++++ .../tests/commit/preserve_commit_message.go | 48 +++++++++++++++++++ pkg/integration/tests/test_list.go | 1 + 3 files changed, 60 insertions(+) create mode 100644 pkg/integration/tests/commit/preserve_commit_message.go diff --git a/pkg/integration/components/commit_description_panel_driver.go b/pkg/integration/components/commit_description_panel_driver.go index 993b1316f..0c4b2cfbb 100644 --- a/pkg/integration/components/commit_description_panel_driver.go +++ b/pkg/integration/components/commit_description_panel_driver.go @@ -8,6 +8,13 @@ func (self *CommitDescriptionPanelDriver) getViewDriver() *ViewDriver { return self.t.Views().CommitDescription() } +// asserts on the current context of the description +func (self *CommitDescriptionPanelDriver) Content(expected *TextMatcher) *CommitDescriptionPanelDriver { + self.getViewDriver().Content(expected) + + return self +} + func (self *CommitDescriptionPanelDriver) Type(value string) *CommitDescriptionPanelDriver { self.t.typeContent(value) @@ -29,3 +36,7 @@ func (self *CommitDescriptionPanelDriver) Title(expected *TextMatcher) *CommitDe return self } + +func (self *CommitDescriptionPanelDriver) Cancel() { + self.getViewDriver().PressEscape() +} diff --git a/pkg/integration/tests/commit/preserve_commit_message.go b/pkg/integration/tests/commit/preserve_commit_message.go new file mode 100644 index 000000000..5a580e854 --- /dev/null +++ b/pkg/integration/tests/commit/preserve_commit_message.go @@ -0,0 +1,48 @@ +package commit + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var PreserveCommitMessage = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Test that the commit message is preserved correctly when canceling the commit message panel", + ExtraCmdArgs: []string{}, + Skip: false, + SetupConfig: func(config *config.AppConfig) {}, + SetupRepo: func(shell *Shell) { + shell.CreateFileAndAdd("myfile", "myfile content") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Files(). + IsFocused(). + Press(keys.Files.CommitChanges) + + t.ExpectPopup().CommitMessagePanel(). + InitialText(Equals("")). + Type("my commit message"). + SwitchToDescription(). + Type("first paragraph"). + AddNewline(). + AddNewline(). + Type("second paragraph"). + Cancel() + + t.Views().Files(). + IsFocused(). + Press(keys.Files.CommitChanges) + + /* EXPECTED: + t.ExpectPopup().CommitMessagePanel(). + Content(Equals("my commit message")). + SwitchToDescription(). + Content(Equals("first paragraph\n\nsecond paragraph")) + + ACTUAL: + */ + t.ExpectPopup().CommitMessagePanel(). + Content(Equals("my commit message\nfirst paragraph")). + SwitchToDescription(). + Content(Equals("second paragraph")) + }, +}) diff --git a/pkg/integration/tests/test_list.go b/pkg/integration/tests/test_list.go index c61a7c2e6..ccefd1dd2 100644 --- a/pkg/integration/tests/test_list.go +++ b/pkg/integration/tests/test_list.go @@ -74,6 +74,7 @@ var tests = []*components.IntegrationTest{ commit.History, commit.HistoryComplex, commit.NewBranch, + commit.PreserveCommitMessage, commit.ResetAuthor, commit.Revert, commit.RevertMerge, From cd50c79ae49b1fb710ce2d4f934f2132d762d3c0 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Tue, 19 Dec 2023 16:47:55 +0100 Subject: [PATCH 4/4] Preserve the commit message correctly even if the description has blank lines There are two possible fixes for this bug, and they differ in behavior when rewording a commit. The one I chose here always splits at the first line feed, which means that for an improperly formatted commit message such as this one: This is a very long multi-line subject, which you shouldn't really use in git. And this is the body (we call it "description" in lazygit). we split after the first line instead of after the first paragraph. This is arguably not what the original author meant, but splitting after the first paragraph doesn't really work well in lazygit, because we would try to put both lines into the one-line subject field of the message panel, and you'd only see the second and not even know that there are more. The other potential fix would have been to join subject and description with two line feeds instead of one in JoinCommitMessageAndDescription; this would have fixed our bug in the same way, but would result in splitting the above message after the second line instead of the first. I think that's worse, so I decided for the first fix. While we're at it, simplify the code a little bit; strings.Cut is documented to return (s, "") when the separator is not found, so there's no need to do this on our side. We do have to trim spaces on the description now, to support the regular reword case where subject and body are separated by a blank line. --- pkg/gui/controllers/helpers/commits_helper.go | 9 ++------- pkg/integration/tests/commit/preserve_commit_message.go | 8 -------- 2 files changed, 2 insertions(+), 15 deletions(-) diff --git a/pkg/gui/controllers/helpers/commits_helper.go b/pkg/gui/controllers/helpers/commits_helper.go index 5d388c319..8691518cd 100644 --- a/pkg/gui/controllers/helpers/commits_helper.go +++ b/pkg/gui/controllers/helpers/commits_helper.go @@ -41,13 +41,8 @@ func NewCommitsHelper( } func (self *CommitsHelper) SplitCommitMessageAndDescription(message string) (string, string) { - for _, separator := range []string{"\n\n", "\n\r\n\r", "\n", "\n\r"} { - msg, description, found := strings.Cut(message, separator) - if found { - return msg, description - } - } - return message, "" + msg, description, _ := strings.Cut(message, "\n") + return msg, strings.TrimSpace(description) } func (self *CommitsHelper) SetMessageAndDescriptionInView(message string) { diff --git a/pkg/integration/tests/commit/preserve_commit_message.go b/pkg/integration/tests/commit/preserve_commit_message.go index 5a580e854..e9297ab76 100644 --- a/pkg/integration/tests/commit/preserve_commit_message.go +++ b/pkg/integration/tests/commit/preserve_commit_message.go @@ -32,17 +32,9 @@ var PreserveCommitMessage = NewIntegrationTest(NewIntegrationTestArgs{ IsFocused(). Press(keys.Files.CommitChanges) - /* EXPECTED: t.ExpectPopup().CommitMessagePanel(). Content(Equals("my commit message")). SwitchToDescription(). Content(Equals("first paragraph\n\nsecond paragraph")) - - ACTUAL: - */ - t.ExpectPopup().CommitMessagePanel(). - Content(Equals("my commit message\nfirst paragraph")). - SwitchToDescription(). - Content(Equals("second paragraph")) }, })