Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allows gpg to run for merge commands #3827

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Neko-Box-Coder
Copy link
Contributor

@Neko-Box-Coder Neko-Box-Coder commented Aug 14, 2024

  • PR Description
    This PR allows GPG to run for merge commands, previously it just hangs if GPG password is required in the CLI.

  • 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)
  • Docs have been updated if necessary
  • You've read through your own file changes for silly mistakes etc

Copy link
Owner

@jesseduffield jesseduffield left a comment

Choose a reason for hiding this comment

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

Nice, see comments

runCmd := self.c.Git().Rebase.AddSkipEditorCommand(
self.c.Git().Rebase.GenericMergeOrRebaseActionCmdObj(commandType, command))

var result error = nil
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
var result error = nil
var result error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed


var result error = nil
if self.c.Git().Config.UsingGpg() && command != REBASE_OPTION_ABORT {
result = self.c.RunSubprocessAndRefresh(runCmd)
Copy link
Owner

Choose a reason for hiding this comment

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

If you're going to run this in a subprocess, you may as well not skip the editor, right? That is, we should only use AddSkipEditorCommand in the else block

Copy link
Contributor Author

@Neko-Box-Coder Neko-Box-Coder Aug 17, 2024

Choose a reason for hiding this comment

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

I have just tried your suggestions (latest commit)

It brings up an (external) editor only when using GPG, which is fine.

But this is a different behavior compared to without GPG signing (else block) where it doesn't bring up any editor at all. Which makes sense since we are skipping the editor I guess?

I am fine either way but shouldn't them be behaving the same (i.e. no editor at all) or is the SkipEditorCommand supposed to bring up the lazygit editor?

@Neko-Box-Coder
Copy link
Contributor Author

Neko-Box-Coder commented Aug 17, 2024

Okay, I fixed the obvious ones that caused it to fail the tests.

But I can't figure out the tests that are failing now. To me, the changes should behave the same as before because I am just calling the logic in rebase.go in merge_and_rebase_helper.go instead. And nobody else is using the functions I am modifying except itself (rebase.go)

I can replicate the failing tests in sandbox mode but I am not too sure why. I probably need some help/pointers here since I am new to the codebase and golang is not main language

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants