Clean up the configuration of where a custom command's output goes #4525

stefanhaller · 2025-04-30T16:17:27Z

PR Description

Previously, custom commands had a stream field that was overloaded with two meanings: 1) it made the command's output appear in the log view, and 2) it used a pty for running the command. It makes sense to be able to configure these independently, so add a separate pty field (although that's probably rarely needed in practice).

Also, the stream and showOutput fields were conflicting; they could be used together, but when setting them both to true, the popup would always show "empty output", so this doesn't make sense. Combine them both into a single output property with the possible values "none", "log", or "popup".

We still have some more redundancy here, for example pty is only used when output is set to "log", and neither output nor pty are used when subprocess is true. But I stopped there, because I think this is already an improvement over the previous state.

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

Looking at these again, I needed a moment to remember what they do, so make this more obvious to help future readers.

…menus We only validate the commands at top level right now.

codacy-production · 2025-04-30T16:19:31Z

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
Report missing for `66caa25`¹	✅ 82.84%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (`66caa25`)	Report Missing	Report Missing	Report Missing
Head commit (`e8d365f`)	55978	48580	86.78%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#4525)	204	169	82.84%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings Change summary preferences

Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct. ↩

pkg/gui/controllers/helpers/gpg_helper.go

stefanhaller · 2025-04-30T16:32:30Z

Some thorough manual testing of this would be good to make sure that all the credential handling stuff still works, and GPG signing. I don't use either, so I don't even know how to test that...

ChrisMcD1 · 2025-04-30T22:18:45Z

So the removal of ICmdObj is possible/desired because it is trivial for test code to create real CmdObj, that we then just feed into the FakeCmdObjRunner?

ChrisMcD1 · 2025-04-30T22:28:17Z

pkg/gui/services/custom_commands/handler_creator.go

 		self.c.LogAction(self.c.Tr.Actions.CustomCommand)

-		if customCommand.Stream != nil && *customCommand.Stream {
+		if customCommand.Output != nil && *customCommand.Output == "log" {


Is it worth defining these literals log, popup, none as a constant somewhere?

Not sure; we aren't doing this for any of the other existing enum configs either.

At one point I looked into replacing those string fields with enums, but couldn't find a good way to do that. Maybe it's still useful to use constants instead of string literals even though we can't enforce their use, but if we do that, I think we should do it consistently for all our configs, and that's out of scope for this PR.

ChrisMcD1 · 2025-04-30T22:31:43Z

pkg/config/user_config_validation.go

+
+		if err := validateCustomCommands(customCommand.CommandMenu); err != nil {
+			return err
+		}


Should we add validation here that the user has not done pty: true with subprocess: true, or output not equal to log?

I don't see any protections against:

Only used when output is set to 'log', and subprocess is false.
from the docs

It's true that this is missing, but we also don't validate that stream: true (or now output: log) can't be used together with subprocess: true. And I was reluctant to do that because it could render existing config files invalid which used to load successfully before, and that could be annoying.

But it only just occurred to me that we can avoid these questions by taking it one step further and removing the subprocess and pty configs too, including them in the output enum too. It would then become output: none | terminal | log | logWithPty | popup. I like this idea a lot, it would simplify the custom command type further. @jesseduffield Thoughts?

I pushed a new version that does this. No fixups, sorry, that wasn't feasible.

ChrisMcD1 · 2025-04-30T22:34:14Z

pkg/config/app_config.go

+				if streamValue.Kind == yaml.ScalarNode && streamValue.Value == "true" && output == "" {
+					output = "popup"
+				}
+			}


I don't see a migration path to output: none from when the existing config had streamValue explicitly set to false. Do we need that?

I didn't think this was needed.

Can you give me some details of why it isn't needed? Given the intent of previous streamValue having, false, true, nil, it seems we would cause issues for some people by removing the distinction between nil and false.

would cause issues for some people

What kind of issues do you have in mind? Omitting 'stream' and setting stream to false has exactly the same effect, as does omitting 'output' and setting 'output' to none. I don't see the point of preserving the fact that they explicitly provided the default value.

In general we recommend against putting default values in your config file, see #4401 (comment). (Yes, that person responding to that comment disagrees, but I'd wager that's a small minority.)

Honestly, in this case I don't care too much either way, because I think it's extremely unlikely that someone put stream: false in their config file anyway. However, it's a little bit more complex to maintain the precedence of the three options in the migrator if we do it this way, and I just don't think it's worth it.

jesseduffield · 2025-04-30T23:26:27Z

pkg/config/user_config.go

-	// [dev] Pointer to bool so that we can distinguish unset (nil) from false.
-	ShowOutput *bool `yaml:"showOutput"`
-	// The title to display in the popup panel if showOutput is true. If left unset, the command will be used as the title.
+	// Where the output of the command should go. 'none' discards it, 'log' shows it in the command log, and 'popup' shows it in a popup.


I think it's worth mentioning that 'log' will stream the output whereas 'popup' will only show the output after the command has finished

Why? This feels like a matter of course to me. With 'log', we stream because it's easy to do and it would be kind of silly not to, but it isn't essential. And for 'popup' it makes little sense to stream the output to the popup window (although technically we might be able to). Why do you think it needs mentioning?

(Happy to include this if it doesn't make the text a lot longer. Would you say it's enough to replace "'log' shows it in the command log" with "'log' streams it to the command log"?)

I just don't expect the user to have that level of understanding that we do about what makes sense. But yes I'm happy to say 'log streams it to the command log'

Changed in the new version

jesseduffield · 2025-04-30T23:32:47Z

pkg/config/app_config.go

+			output := ""
+			if streamKey, streamValue := yaml_utils.RemoveKey(node, "stream"); streamKey != nil {
+				if streamValue.Kind == yaml.ScalarNode && streamValue.Value == "true" {
+					output = "log"


In the interest of backwards compatibility, what are your thoughts on migrating everybody who's currently on 'stream: true' to both 'output: log' and 'pty: true'?

My assumption is that people using stream: true did that because they wanted to see the output, not because they wanted to use a PTY (like me, when I discovered the issue). I think it makes sense to err on the side of what's probably best for most people, rather than what faithfully replicates the previous behavior.

But how do we actually know what's best for most people? The fact that we've come this far without anybody complaining suggests that the current way actually is the best way.

The fact that we've come this far without anybody complaining

Huh? I am complaining, that's what started this whole change. I ran into a real problem (and a pretty annoying one at that, took me several days to sort it out), just because "stream: true" used a pty without me realizing it. This was caused by an updated tool (gh in this case) which suddenly started to do fancy things when running in a terminal, things that I don't want or need. This could happen to other people, and with other tools, and I want to spare them the frustration that I had.

Defaulting to no pty is the much more conservative default; the worst that can happen is that people lose the color output that they had before, which is not a big deal compared to the potential of misbehavior caused by running in a pty.

I added a breaking changes notice (e8d365f) to make people aware of this.

I did not mean to imply that you were not yourself complaining, my point was that we've come this far without any complaints prior, meaning lots of people may be happy with the current default.

Nevertheless you've persuaded me: it's much more painful to realise that you don't need a pty than vice versa, and it's possible for an issue to arise out of nowhere like it did with your gh.

jesseduffield

Looking good, left a couple questions

stefanhaller · 2025-05-01T08:13:22Z

So the removal of ICmdObj is possible/desired because it is trivial for test code to create real CmdObj, that we then just feed into the FakeCmdObjRunner?

I'm following the rule of thumb that if there is only a single type that implements an interface, the interface is unnecessary and you can use the type directly. See https://100go.co/5-interface-pollution/

It is only implemented by *CmdObj, so use that directly in client code.

getCmdHandlerNonPty is defined for all platforms.

This decouples StreamOutput from whether a PTY is used. In most cases we just want to see the output in the log window, but don't have to use a PTY, e.g. for the bisect commands. This has the implication that custom commands that are using "stream: true" no longer use a PTY. In most cases that's probably a good thing, but we're going to add a separate pty config for those who really wanted this.

Co-authored-by: Chris McDonnell <[email protected]>

…o a single output enum

stefanhaller · 2025-05-01T14:08:10Z

pkg/i18n/english.go

  autoForwardBranches: none

 If, on the other hand, you want this even for feature branches, you can set it to 'allBranches' instead.`,
+			"0.51.0": `- The 'subprocess', 'stream', and 'showOutput' fields of custom commands have been replaced by a single 'output' field. This should be transparent, if you used these in your config file it should have been automatically updated for you. There's one noteable change though: the 'stream' field used to mean both that the command's output would be streamed to the command log, and that the command would be run in a pseudo terminal (pty). We converted this to 'output: log', which means that the command's output will be streamed to the command log, but not use a pty, on the assumption that this is what most people wanted. If you do actually want to run a command in a pty, you can change this to 'output: logWithPty' instead.`,


Setting this to 0.51 assuming that I won't merge it before Saturday's scheduled release.

…t use a pty any more

jesseduffield

LGTM

stefanhaller · 2025-05-03T08:07:05Z

Some thorough manual testing of this would be good to make sure that all the credential handling stuff still works, and GPG signing. I don't use either, so I don't even know how to test that...

@jesseduffield @ChrisMcD1 Do we think this has gotten enough manual testing to be comfortable merging it? Has either of you tried the above features?

jesseduffield · 2025-05-03T14:29:13Z

I have not manually tested it: I too do not use GPG or credentials. I'm not sure if we've explored adding an integration test for GPG in the past but that would be great because it is a hole in our tests at the moment (it would also cover the credentials side of things). Unfortunately it would require adding a dependency (gnupg) to CI as well as to anybody who wants to run the tests locally.

We could set the GPG thing back to using a PTY for the sake of getting this PR across the line and have a separate issue for removing PTY from GPG and adding an integration test.

Trying to test out credentials on push now, I get a warning about not being able to use password authentication with github both on master and on this branch:

The fact I get prompted at all and can enter the values is reassuring though.

I can do some more manual testing tomorrow

stefanhaller · 2025-05-03T15:05:57Z

For pushing with credentials we do have an integration test, and it does indeed fail if you revert this hunk, so I think we're fine there. It is implemented using this fake pre-push hook that pretends to be the password prompt, which I think was a pretty smart idea.

I don't see how we can write an integration test for GPG signing though, at least not for the case where it prompts you for a passphrase. It suspends to the terminal in that case, and we don't have any way to type the necessary characters in response to the prompt from within an integration test. So the only test we could write here is one where no passhrase is requested, and the config file contains overrideGpg: true, but this test is the less interesting one.

jesseduffield · 2025-05-03T23:00:20Z

Just tested GPG signing manually:

brew install gnupg

# Make a throw‑away key (expiry 1 day)
gpg --quick-gen-key "Test User <[email protected]>" default default 1d

# Tell Git to use it
KEYID=$(gpg --list-secret-keys --with-colons | awk -F: '$1=="sec"{print $5;exit}')
git config --global user.signingkey "$KEYID"
git config --global commit.gpgsign true
git config --global gpg.program "$(which gpg)"

# Make a commit in lazygit, verify it asks for passphrase in a subprocess

# Verify the commit
git log -1 --show-signature

# Make another commit in lazygit, verify gpg caches the passphrase so you're not prompted again

# Set `git.overrideGpg: true` in lazygit config
# Make yet another commit, verify there's no issue creating the commit

# Disable caching of passphrase
gpgconf --kill gpg-agent

# Make another commit in lazygit, verify it completely breaks (as expected)

# Cleanup
TEST_EMAIL="[email protected]"
KEY_FP=$(gpg --list-secret-keys --with-colons "$TEST_EMAIL" | awk -F: '/^fpr:/ { print $10; exit }')

if [ -z "$KEY_FP" ]; then
  echo "No secret key found for: $TEST_EMAIL"
  exit 1
fi

echo "Deleting GPG key: $KEY_FP ($TEST_EMAIL)"

gpg --batch --yes --delete-secret-and-public-key "$KEY_FP"

gpgconf --kill gpg-agent

stefanhaller · 2025-05-04T06:11:23Z

Awesome, thanks. I guess we're good to merge then?

ChrisMcD1 · 2025-05-05T00:58:54Z

Do we think this has gotten enough manual testing?

I'm not a user of custom commands at all right now, so I'm not in a place to put this through its paces. But the code LGTM

jesseduffield · 2025-05-05T02:01:11Z

Awesome, thanks. I guess we're good to merge then?

Yep

…or a passphrase (#4586) Fix a regression introduced with #4525. This fixes the problem that background fetching makes lazygit hang when the fetch request needs to prompt for a passphrase. For Mac users who use the keychain to store their ssh passphrases, this can happen when lazygit is running while the machine goes to sleep, because macOS looks the keychain in that case.

This MR contains the following updates: | Package | Update | Change | |---|---|---| | [jesseduffield/lazygit](https://github.com/jesseduffield/lazygit) | minor | `v0.50.0` -> `v0.51.0` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>jesseduffield/lazygit (jesseduffield/lazygit)</summary> ### [`v0.51.0`](https://github.com/jesseduffield/lazygit/releases/tag/v0.51.0) [Compare Source](jesseduffield/lazygit@v0.50.0...v0.51.0)  #### What's Changed ##### Enhancements 🔥 - Clean up the configuration of where a custom command's output goes by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4525 - Add custom patch command "Move patch into new commit before the original commit" by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4552 - Make '>' first jump to the beginning of the branch, and only then to the first commit by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4544 - Add an alternate keybinding (default <c-s>) for ConfirmInEditor by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4532 - Print migration changes to the console when migrating config file by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4548 ##### Fixes 🔧 - Migrate deprecated AllBranchesLogCmd to AllBranchesLogCmds by [@ChrisMcD1](https://github.com/ChrisMcD1) in jesseduffield/lazygit#4345 - Clear preserved commit message when entering CommitEditorPanel by [@ChrisMcD1](https://github.com/ChrisMcD1) in jesseduffield/lazygit#4558 - Split behavior of rendering allBranchesLogCmd and switching to next cmd by [@ChrisMcD1](https://github.com/ChrisMcD1) in jesseduffield/lazygit#4574 - Fix possible crash with auto-forwarding branches by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4565 - Fix main view occasionally scrolling to the top on its own when focused by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4573 - Fix home and end keys in prompts by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4554 - Fix crash when clicking in the status view by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4567 ##### Maintenance ⚙️ - Clean up utils package by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4538 ##### Docs 📖 - reword documentation for git.autoForwardBranches by [@sean-xyz](https://github.com/sean-xyz) in jesseduffield/lazygit#4545 #### New Contributors - [@sean-xyz](https://github.com/sean-xyz) made their first contribution in jesseduffield/lazygit#4545 **Full Changelog**: jesseduffield/lazygit@v0.50.0...v0.51.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

stefanhaller added 2 commits April 30, 2025 17:31

Add comments in tests to explain what they test

7137196

Looking at these again, I needed a moment to remember what they do, so make this more obvious to help future readers.

Add test demonstrating missing validation for custom commands in sub …

e295477

…menus We only validate the commands at top level right now.

stefanhaller added the enhancement New feature or request label Apr 30, 2025

stefanhaller mentioned this pull request Apr 30, 2025

Migrate deprecated AllBranchesLogCmd to AllBranchesLogCmds #4345

Merged

7 tasks

stefanhaller force-pushed the custom-command-output branch from deb1cbe to 63f755a Compare April 30, 2025 16:23

stefanhaller requested a review from jesseduffield April 30, 2025 16:26

stefanhaller commented Apr 30, 2025

View reviewed changes

pkg/gui/controllers/helpers/gpg_helper.go Outdated Show resolved Hide resolved

ChrisMcD1 reviewed Apr 30, 2025

View reviewed changes

jesseduffield reviewed Apr 30, 2025

View reviewed changes

stefanhaller and others added 7 commits May 1, 2025 15:21

Validate custom commands in sub menus

4e3d09e

Remove ICmdObj interface

a400ef0

It is only implemented by *CmdObj, so use that directly in client code.

Rename getCmdHandler to getCmdHandlerPty, and add getCmdHandlerNonPty

8cf617b

getCmdHandlerNonPty is defined for all platforms.

Remove unused function UpdateYamlValue

02611da

Add yaml_utils.RemoveKey

5f4be3b

Co-authored-by: Chris McDonnell <[email protected]>

Combine customCommand's subprocess, stream, and showOutput fields int…

a9f9dee

…o a single output enum

stefanhaller force-pushed the custom-command-output branch from da8a9e7 to 3159e24 Compare May 1, 2025 13:59

stefanhaller commented May 1, 2025

View reviewed changes

Add breaking change notice about 'stream: true' being converted to no…

e8d365f

…t use a pty any more

stefanhaller force-pushed the custom-command-output branch from 3159e24 to e8d365f Compare May 1, 2025 14:09

jesseduffield approved these changes May 2, 2025

View reviewed changes

stefanhaller merged commit 223978e into master May 5, 2025
14 checks passed

stefanhaller deleted the custom-command-output branch May 5, 2025 15:49

stefanhaller mentioned this pull request May 23, 2025

Fix lazygit's UI becoming unresponsive when a background fetch asks for a passphrase #4586

Merged

Uh oh!

Clean up the configuration of where a custom command's output goes #4525

Clean up the configuration of where a custom command's output goes #4525

Uh oh!

Conversation

stefanhaller commented Apr 30, 2025

Uh oh!

codacy-production bot commented Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage summary from Codacy

Footnotes

Uh oh!

Uh oh!

stefanhaller commented Apr 30, 2025

Uh oh!

ChrisMcD1 commented Apr 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stefanhaller May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jesseduffield left a comment

Choose a reason for hiding this comment

Uh oh!

stefanhaller commented May 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jesseduffield left a comment

Choose a reason for hiding this comment

Uh oh!

stefanhaller commented May 3, 2025

Uh oh!

jesseduffield commented May 3, 2025

Uh oh!

stefanhaller commented May 3, 2025

Uh oh!

jesseduffield commented May 3, 2025

Uh oh!

stefanhaller commented May 4, 2025

Uh oh!

ChrisMcD1 commented May 5, 2025

Uh oh!

jesseduffield commented May 5, 2025

codacy-production bot commented Apr 30, 2025 •

edited

Loading

stefanhaller May 1, 2025 •

edited

Loading