search

evilmartians / lefthook

Fast and powerful Git hooks manager for any type of projects.
MIT License
4.66k stars 211 forks source link

Option to redirect standard input to `/dev/null` #733

Closed sanmai-NL closed 2 months ago

sanmai-NL commented 2 months ago

:zap: Summary

Similar to a subset of the options in https://www.freedesktop.org/software/systemd/man/latest/systemd.exec.html#Logging%20and%20Standard%20Input/Output

Value

There are situations in complex CI environments where Lefthook will stall, when not using use_stdin: true. How can a user make sure that commands can never use stdin without causing crashes (by closing the file descriptor)? By redirecting stdin to /dev/null or a similar concept in Windows. Currently, the user has to configure use_stdin: true with every command that could possibly stall Lefthook, even if they don't want those commands to use standard input at all.

Behavior and configuration changes

Nothing, although that options remains open in my view. But this suggestion is to extend behavior by offering a default false redirection option.

mrexox commented 2 months ago

Hey @sanmai-NL, I am currently solving an issue related to STDIN right now :)

In this PR I explicitly pass null reader (a reader that returns EOF on every read) to commands that don't explicitly specify use_stdin: true or interactive: true.

I think this is the behavior that prevents commands from being stuck on waiting for input. Do you agree?

sanmai-NL commented 2 months ago

I think that should work, but see my reference. Managing which process controls the tty could also solve this.

sanmai-NL commented 2 months ago

@mrexox Was this also resolved with https://github.com/evilmartians/lefthook/pull/732?

mrexox commented 2 months ago

This now works by default (not via an option). All commands get just EOF when they try to read from STDIN if they don't specify use_stdin: true or interactive: true.

I have concerns of whether a global option is useful. Do you think we should close the feature request or specify it?

sanmai-NL commented 2 months ago

Can be closed IMO.