Clear preserved commit message when entering CommitEditorPanel

[ChrisMcD1]

• PR Description

Pretty basic implementation of this fix. The one thing I considered doing that I didn't was to rename OnCommitSuccess, since now 2 out of 4 call sites invoke it prior to make the commit message (and one does it here to invoke a tag, which might be a separate bug)

lazygit/pkg/gui/controllers/helpers/tags_helper.go

Lines 36 to 39 in 5d30409

	return self.gpg.WithGpgHandling(command, git_commands.TagGpgSign, self.c.Tr.CreatingTag, func() error {
	self.commitsHelper.OnCommitSuccess()
	return nil
	}, []types.RefreshableView{types.COMMITS, types.TAGS})

The implementation of OnCommitSuccess also leaves me a bit confused. I'm not sure why it doesn't clear the message every time. By checking with the CommitMessageViewModel.preserveMessage, we only open up the option that we don't clear an existing message should that be false. Is there some world where this is desired? I figure if they are opening the window without the intent to save the message, it would be even more okay to throw away any potential message stored there.

lazygit/pkg/gui/controllers/helpers/commits_helper.go

Lines 160 to 165 in 5d30409

	func (self *CommitsHelper) OnCommitSuccess() {
	// if we have a preserved message we want to clear it on success
	if self.c.Contexts().CommitMessage.GetPreserveMessage() {
	self.c.Contexts().CommitMessage.SetPreservedMessageAndLogError("")
	}
	}

Fixes #4557

• 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

[stefanhaller]

Pretty basic implementation of this fix.

Looks good to me.

The one thing I considered doing that I didn't was to rename OnCommitSuccess,

I'd be in favor of doing that. Maybe ClearPreservedCommitMessage or something like that.

and one does it here to invoke a tag, which might be a separate bug)

I don't think that's a bug, but this call could be removed, because it does nothing (since we are opening the tags panel with preserveMessage=false, see below). Could be another, separate cleanup commit.

The implementation of OnCommitSuccess also leaves me a bit confused. I'm not sure why it doesn't clear the message every time. By checking with the CommitMessageViewModel.preserveMessage, we only open up the option that we don't clear an existing message should that be false. Is there some world where this is desired?

Yes. I think the idea is that creating tags doesn't take part in the remember-my-partial-commit-message business. I'm not sure how relevant this really is in practice, but something like:

• start to make a commit, begin typing a message, cancel halfway through
• create a tag; neither uses nor clears the preserved message
• press c again to continue your commit. Message is still there.

[ChrisMcD1]

I'd be in favor of doing that. Maybe ClearPreservedCommitMessage or something like that.

Stole your name because naming is hard 😆

I don't think that's a bug, but this call could be removed

Yeah, I misused the word "bug" there, I just meant "This is weird code". I have removed that call

I'm not sure how relevant this really is in practice

I have done some research and I believe this is now never relevant in practice. So I added a commit removing the check. With previous commits like 41a7afb we have been whittling down the usages of OnCommitSuccess, and at this point there are 3 call sites:

1. working_tree_helper.handleCommit https://github.com/ChrisMcD1/lazygit/blob/2988df76aaa5fd1e0d31b57c7749984bc7ae328a/pkg/gui/controllers/helpers/working_tree_helper.go#L115
2. working_tree_helper.HandleCommitEditorPress https://github.com/ChrisMcD1/lazygit/blob/2988df76aaa5fd1e0d31b57c7749984bc7ae328a/pkg/gui/controllers/helpers/working_tree_helper.go#L141
3. working_tree.switchFromCommitMessagePanelToEditor https://github.com/ChrisMcD1/lazygit/blob/2988df76aaa5fd1e0d31b57c7749984bc7ae328a/pkg/gui/controllers/helpers/working_tree_helper.go#L127

which tracking their upstream leads to:

1. handleCommit and switchFromCommitMessgePanelToEditor share the same singular callsite has preserveMessage: true https://github.com/ChrisMcD1/lazygit/blob/2988df76aaa5fd1e0d31b57c7749984bc7ae328a/pkg/gui/controllers/helpers/working_tree_helper.go#L94-L97
2. HandleCommitEditorPress (this is the upstream)

^ This is the method we are adding this clearing to in this PR, and I'm thinking it should clear the preserved message regardless of the previous state of preserveMessage. If you begin a partial commit message with c, switch and create a tag with T, you have now made preserveMessage = false, so when you go back to committing, create a full commit message with C, you don't end up clearing the partial commit message. This feels less intuitive to me than "When I create a commit with C, I clear my partial message"

(But now that I've put the whole paragraph into words, I'm actually less convinced than when it was an idea floating around in my head. I'm open to reverting this last commit if people want)