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

Add support for LESSOPEN and LESSCLOSE. #1597

Closed
wants to merge 5 commits into from
Closed

Conversation

eth-p
Copy link
Collaborator

@eth-p eth-p commented Mar 24, 2021

This pull request adds opt-in support for preprocessors using the LESSOPEN and LESSCLOSE environment variables.

To enable the preprocessor, --preprocessor=lessopen must be set either be passed as a command line argument or set in the config file. This was made opt-in to avoid breaking default installations that provide an incompatible LESSOPEN program.

image

When used with something like lesspipe.sh, this can potentially fix #237, #642, #748, and #971.

Implementation (Preprocessor Abstraction)

The preprocessor implementation is basically a map operation on Input<'a> -> OpenedInput<'a>.
It can either open the source input and return that directly (e.g. nothing to be done), or create a new Input<'a> and return that instead.

Preprocessors (or inputs like InputKind::StdIn) can attach "handles" to OpenedInput<'a> structs.
Handles can be used to perform cleanup actions (like deleting temporary files), or in the case of InputKind::StdIn, used to (unsafely) keep a reference to a Stdin object alive as long as its corresponding InputReader.

Note: handles will either be closed automatically when dropped (panicking on errors), or explicitly closed with OpenedInput::close.

Implementation (Lessopen)

Using the new preprocessor abstraction, basic LESSOPEN/LESSCLOSE support was added to bat.
Note: This only supports file inputs.

  1. If LESSOPEN is defined and invalid (as determined by shell_words::split), bat will silently do nothing.
    This doesn't follow the behavior of less, but it's more consistent with how bat handles the pager.

  2. If LESSOPEN is defined but the process could not be spawned, bat will print a warning.
    For consistency with less (which simply ignores the exit code), this does not include the exit code of an unsuccessfully-exited command.

  3. If LESSOPEN is defined and starts with "|", bat will use its standard output as the preprocessed file contents.
    If no output is provided by the LESSOPEN function, bat will use the original file.

  4. If LESSOPEN is defined and does not start with "|", bat will read the first line of output as the path to a temporary file containing the preprocessed file contents.

  5. If any preprocessor command was executed and LESSCLOSE is defined, bat will execute LESSCLOSE.
    If the process could not be spawned, bat will print a warning.

Caveats

  • Git changes will be displayed with LESSOPEN, even if it outputs something different than the original file.
    This is intentional, and it's an unfortunate side-effect of the way that lesspipe.sh will always cat the file, even if it doesn't need to preprocess it.
    Without this, git changes would not be displayed at all.

  • Due to some rewrites with the input module in order to add support for preprocessors, two instances of unsafe were added.

    • A comment with safety information was included.
  • It is not 100% compatible with less's implementation.

    • This implementation tries to avoid invoking the user's shell, which limits the support to a shell-quoted list of arguments.
    • This implementation only replaces substitutes %s if it is an entire argument. Something like LESSOPEN='| echo File:%s' will not work.
  • No tests were added. I couldn't think of a way to add tests without writing to the filesystem.

@Enselic
Copy link
Collaborator

Enselic commented Oct 2, 2021

We currently have no unsafe code in the app code, only in tests. Personally, I would consider the addition of unsafe to the "real" code to be a dramatic change for the project. Please accept my apologies in advance, if you have already discussed this and agreed that starting to use unsafe in the app code is no big deal.

I'm tentatively marking as needs-work for now. (There are also some conflicts by now that needs to be sorted out.)

@eth-p
Copy link
Collaborator Author

eth-p commented Oct 2, 2021

Oh man, it's been a while. Honestly, this was more of a proof-of-concept idea/prototype than anything. Looking back at the implementation, I agree that I shouldn't have used unsafe here and that we don't need to introduce any unsafe code unless strictly necessary.

I'm actually going to close this. If we end up with more interest in supporting LESSOPEN thought, I would be happy to look into rewriting it at a later date when I have more free time.

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

Successfully merging this pull request may close these issues.

zcat like functionality
2 participants