Skip to content

Conversation

@ChrisMcD1
Copy link
Contributor

@ChrisMcD1 ChrisMcD1 commented Feb 16, 2025

  • PR Description
    I based this off of the existing CommitMessage option in the copy commit attributes menu. I think the only interesting thing I did was choose the definition of commit body as:
func (self *CommitCommands) GetCommitBody(commitHash string) (string, error) {
	message, err := self.GetCommitMessage(commitHash)
	if err != nil {
		return "", err
	}

	splitMessage := strings.SplitAfterN(message, "\n", 2)
	if len(splitMessage) != 2 {
		return "", errors.Errorf("Requested commit %s does not have a body", commitHash)
	}

	return strings.TrimSpace(splitMessage[1]), nil
}

But that feels pretty intuitive. The definition of GetCommitMessage that I delegate to already has a strings.ReplaceAll(strings.TrimSpace(message), "\r\n", "\n"), so dealing with newline returns shouldn't be particularly relevant.

I have an integration test that validates the happy path, and that the error we return is gracefully presented to the user.

Fixes #4035

  • 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

OnPress: func() error {
return self.copyCommitBodyToClipboard(commit)
},
Key: 'b',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of the other keys in this menu allow customization, so I felt it was fine to leave this here. I can't think of a different commit attribute that might want to claim the b shortcut one day.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds fine to me.

@ChrisMcD1 ChrisMcD1 force-pushed the 4035-commit-body branch 3 times, most recently from b514421 to 79774b8 Compare February 16, 2025 01:56
@ChrisMcD1 ChrisMcD1 marked this pull request as ready for review February 16, 2025 01:57
@stefanhaller stefanhaller added the enhancement New feature or request label Feb 16, 2025
Copy link
Collaborator

@stefanhaller stefanhaller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work. A few things inline below, but also a few general thoughts here:

  • The term "commit body" sounds strange to me. I'd call it "commit message body" everywhere.
  • I wonder if the menu with the three entries "Commit subject", "Commit message", and "Commit message body" is clear enough. Maybe we should say something like "Commit message (subject and body)" for the 2nd entry to make that clearer?
  • For tags, the entry is crossed out if the commit doesn't have tags, but for the body the entry is enabled and shows an error when you invoke it; this is inconsistent. I'd prefer the entry to be crossed out in that case, too. Now it's true that it's a little more work to find out if the commit message has a body, and we said that DisabledReasons must be very efficient to compute so that there's no delay when opening a menu; however, I think this mainly applies to menu entries in the global keybindings menu, I doubt it would be noticeable here.

OnPress: func() error {
return self.copyCommitBodyToClipboard(commit)
},
Key: 'b',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds fine to me.

}

func (self *BasicCommitsController) copyCommitBodyToClipboard(commit *models.Commit) error {
message, err := self.c.Git().Commit.GetCommitBody(commit.Hash)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of adding a new function that does this, you could call GetCommitSubject and then split it here using self.c.Helpers().Commits.SplitCommitMessageAndDescription(message). Fewer different ways of doing the same thing. (Could be a local helper method here if you prefer.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I see your point here. Does how I restructured the code in order to eagerly provide a DisabledReason now make a compelling case to have a GetCommitMessageBody on CommitCommands?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point is simply that I don't want to have two functions in the code that do the same thing (splitting a message into subject and body, in this case), but in different ways. I pushed a fixup (9d938df) to address this.

@ChrisMcD1
Copy link
Contributor Author

I don't see the same disabled behavior I imagine you do. This is on a commit with both tag and message body disabled.

Am I missing some setting to make these visually distinct from the disabled options?
image

@stefanhaller
Copy link
Collaborator

I don't see the same disabled behavior I imagine you do.

Someone else had this problem too, where strikethrough doesn't render. This is annoying, would be good to find out what's responsible for this: #4117 (comment)

@stefanhaller
Copy link
Collaborator

I also found one more use of "commit body" and pushed a fixup for that, too (d21d51b).

This would be ready to merge for me if you are fine with the fixups.

@ChrisMcD1
Copy link
Contributor Author

I'm happy with the fixups! Autosquashed them. Is that what you wanted, or did you have a way your normally squash them prior to merge?

@ChrisMcD1
Copy link
Contributor Author

Although somehow that autosquash didn't mark it as joint authorship.... Odd.....

@stefanhaller
Copy link
Collaborator

I'm happy with the fixups! Autosquashed them. Is that what you wanted, or did you have a way your normally squash them prior to merge?

Either way is fine with me. I don't mind having to squash fixups myself before merging either.

Although somehow that autosquash didn't mark it as joint authorship.... Odd.....

Not sure what you mean by "joint authorship", really. Did you expect some sort of "Co-authored-by" trailer to get added to the commit message? I'm not aware of any tools that do that when squashing, and for the record I don't care about getting added as a co-author when I push fixups.

I am, however, very surprised about the Committer field of your squashed commit, it looks like this:
Screenshot 2025-02-17 at 18 45 04

Where does this come from? How exactly did you perform the autosquash? The only reference to the address CI@example.com that I can find is here:

email = CI@example.com

This is just curiosity at this point. I took the liberty of fixing the Committer field, and am going to merge.

@stefanhaller stefanhaller merged commit a7bfeca into jesseduffield:master Feb 17, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add ability to copy commit message body to clipboard

2 participants