Add signoff flag to revert action

[ChrisMcD1]

PR Description

This PR adds the --signoff flag to git revert commands using the standard helper in on CommitCommands.

Fixes #4911

There could also be some consideration of adding the signoff flag to other things that still do not have it, like:

Commands that create new commits

Merge commit

A bit annoying because it is not in commit.go that has the helper we use here

lazygit/pkg/commands/git_commands/branch.go

Line 280 in 17d03ec

Arg(strings.Fields(self.UserConfig().Git.Merging.Args)...).

Creating fixup commit

Maybe not relevant since these are not often merged at all, so not needing signoff

lazygit/pkg/commands/git_commands/commit.go

Line 290 in 17d03ec

cmdArgs := NewGitCmd("commit").Arg("--fixup=" + hash).ToArgv()

Commands that edit commands

Could be totally irrelevant because the signoff would only matter if someone just turned on the setting, and then amended something.

Amending head:

lazygit/pkg/commands/git_commands/commit.go

Line 235 in 17d03ec

cmdArgs := NewGitCmd("commit").

Rewording commit:

lazygit/pkg/commands/git_commands/commit.go

Line 123 in 17d03ec

Arg("--allow-empty", "--amend", "--only").

Author related commands:

lazygit/pkg/commands/git_commands/commit.go

Lines 24 to 55 in 17d03ec

	func (self *CommitCommands) ResetAuthor() error {
	cmdArgs := NewGitCmd("commit").
	Arg("--allow-empty", "--allow-empty-message", "--only", "--no-edit", "--amend", "--reset-author").
	ToArgv()
	return self.cmd.New(cmdArgs).Run()
	}
	// Sets the commit's author to the supplied value. Value is expected to be of the form 'Name <Email>'
	func (self *CommitCommands) SetAuthor(value string) error {
	cmdArgs := NewGitCmd("commit").
	Arg("--allow-empty", "--allow-empty-message", "--only", "--no-edit", "--amend", "--author="+value).
	ToArgv()
	return self.cmd.New(cmdArgs).Run()
	}
	// Add a commit's coauthor using Github/Gitlab Co-authored-by metadata. Value is expected to be of the form 'Name <Email>'
	func (self *CommitCommands) AddCoAuthor(hash string, author string) error {
	message, err := self.GetCommitMessage(hash)
	if err != nil {
	return err
	}
	message = AddCoAuthorToMessage(message, author)
	cmdArgs := NewGitCmd("commit").
	Arg("--allow-empty", "--amend", "--only", "-m", message).
	ToArgv()
	return self.cmd.New(cmdArgs).Run()
	}

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]

Looks good, thanks.

A bit annoying because it is not in commit.go that has the helper we use here

That shouldn't be a big problem; actually, I don't find the helper very useful, we might just as well get rid of it and write

		ArgIf(self.UserConfig().Git.Commit.SignOff, "--signoff").

everywhere. Then you can do the same for merge commits.

I agree that merge is probably the only other command that we should add the flag to. (What about cherry-pick though? I'm unsure about the semantics here; what if you are cherry-picking someone else's commit and it already has a signed-off-by header, should it still add yours as well?)

[ChrisMcD1]

Agreed on the lack of utility from the helper, I have inlined it.

I figure that since git cherry-pick has a --signoff option, that it would be appropriate to add. (If it doesn't make sense with the meaning of it, why have it at all?) Additionally, re-reading the DCO https://developercertificate.org/, section (c), feels like the exact scenario of a cherry-pick:

(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.

which I read to say the final Signed-Off-By line to say "I am cherrypicking this from a place where the previous person's signoff means the same thing that I mean it to be now."

[stefanhaller]

Agreed on the lack of utility from the helper, I have inlined it.

👍 (However, please don't make refactorings and behavior changes in the same commit. Also, the commit subject is no longer accurate.)

I figure that since git cherry-pick has a --signoff option, that it would be appropriate to add. (If it doesn't make sense with the meaning of it, why have it at all?)

You have a point; on the other hand, git's --signoff flag is something that you have to add to every invocation, so you can decide case-by-case whether it is appropriate. There is no global git config cherry-pick.signoff true option in git, and maybe there's a reason for that? I feel slightly uneasy about adding this as a global option in lazygit. Not a blocking objection, I'm still ok with merging this if everybody thinks it's a good idea.

(Side note: I find it interesting that of all git commands that take a --signoff flag, git format-patch is the only one that has a corresponding config option to turn it on globally. And the documentation of that option says "Note: Adding the Signed-off-by trailer to a patch should be a conscious act".)

[ChrisMcD1]

However, please don't make refactorings and behavior changes in the same commit. Also, the commit subject is no longer accurate.)

Rewrote git history to have the refactoring separate from the change.

I feel slightly uneasy about adding this as a global option in lazygit.

I'd say that most users thought that it already was added as a global option, hence the initial bug report surprised that it wasn't. (I even thought it was under the config key git.signOff when I was first drafting this reply, but double checked and it is technically nested under commit as well). It's possible that we are acting against the git developers design, but that happened the day that our config option git.commit.signOff was added, with no equivalent on git commit.