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

Fix $LESSOPEN tests and behavior #2662

Merged
merged 5 commits into from
Sep 8, 2023
Merged

Fix $LESSOPEN tests and behavior #2662

merged 5 commits into from
Sep 8, 2023

Conversation

Anomalocaridid
Copy link
Contributor

@Anomalocaridid Anomalocaridid commented Sep 6, 2023

This PR has a handful of fixes for the $LESSOPEN functionality that addresses concerns brought up in #2444 and #2602:

  • Pass --no-lessopen to less to prevent it from preprocessing input a second file. This should fix the intermittently failing test, assuming we correctly figured out the cause. Unfortunately, this did not appear to fix the intermittently failing test.
  • Make $LESSOPEN support opt-in rather than on by default. I added a --lessopen flag to enable this and kept the --no-lessopen flag for people that add --lessopen to their config file, but want to override it for whatever reason. Also changed the tests, man page, and completions accordingly.
  • Require that $LESSOPEN has exactly one %s, otherwise ignoring it and with a warning message much like less does. Also changed the tests accordingly.
  • Along with updating CHANGELOG.md, I took the liberty of moving the entry for the $LESSOPEN PR from v0.22.0 to the unreleased features section since it wasn't updated before it was merged recently.
  • I re-added the lessopen feature to default since you need to enable it with an option now anyways. I made sure to have this in its own commit so it can be easily removed if anyone objects to it. Per @Enselic's request, I removed it so it has a chance to get more testing before being enabled by default.

@Anomalocaridid
Copy link
Contributor Author

Anomalocaridid commented Sep 7, 2023

In the absence of a better solution, I ignored lessopen_and_lessclose_stdin_temp() for now. I figure that if we find a better solution, we can just remove it, and if we do not find one before this gets merged, I assume we'll probably opt for ignoring the test anyways to prevent random pipeline failures.

I'll make sure to take another look at the test later when I get a chance.

Copy link
Collaborator

@Enselic Enselic left a comment

Choose a reason for hiding this comment

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

Thank you for the quick update. I'm not sure about needing --lessopen to enable. Long term we want to let the LESSOPEN env var decide if the feature is on or off, I think? Or could someone want LESSOPEN set but not want bat to process it?

CHANGELOG.md Outdated

## Bugfixes

- Fix `more` not being found on Windows when provided via `BAT_PAGER`, see #2570, #2580, and #2651 (@mataha)
- Fix `less` preprocessing with `LESSOPEN` again and causing a test to intermittently fail, and incorrect `LESSOPEN` behavior see #2444, #2602, and #2662 (@Anomalocaridid)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This was was never present in a release version of bat, right? In that case I don't think we should have this as a bugfix. But feel free to list the PRs in the "Features" item.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved the PRs to the "Features" item. How is it now?

Cargo.toml Outdated
@@ -21,6 +21,7 @@ application = [
"build-assets",
"git",
"minimal-application",
"lessopen",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I actually think we should give this feature some more testing before we enable it by default. I plan on making a release in 1-4 weeks, and let's keep it disabled. Then right after the release we can enable it again on the git master branch so it gets some testing. Once it has been enabled by default on git master without any serious bugs reported, we can enable it by default on an official release.

Note that it will still be possible to enable the features in the upcoming official release like this: cargo install bat --features lessopen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that you mention it, that seems like a much better option. I have removed the commit that re-enabled the lessopen feature by default.

@@ -32,10 +35,18 @@ enum LessOpenKind {

impl LessOpenPreprocessor {
/// Create a new instance of LessOpenPreprocessor
/// Will return Ok if and only if $LESSOPEN is set
/// Will return Ok if and only if $LESSOPEN is set and contains exactly one %s
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you mean with "exactly one %s"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's say you want to use batpipe as to preprocess less or bat's input.

batpipe %s is a valid command for $LESSOPEN.

The following are not valid and will result in less/bat ignoring it and printing a warning:

  • batpipe
  • batpipe %s %s
  • batpipe %s %s (...) %s

);

bat()
.env("LESSOPEN", "|echo File is %s")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am completely unfamiliar LESSOPEN and with %s. Can you link to a good online resource where I can learn more please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would recommend the INPUT PREPROCESSOR section of the less(1) man page. It is what I used to implement $LESSOPEN support.

@Anomalocaridid
Copy link
Contributor Author

Thank you for the quick update. I'm not sure about needing --lessopen to enable. Long term we want to let the LESSOPEN env var decide if the feature is on or off, I think? Or could someone want LESSOPEN set but not want bat to process it?

Actually, there is a use case where one would want LESSOPEN but not want bat to use it. Specifically, I added --lessopen to address this concern that using bat in LESSOPEN breaks bat with infinite recursion.

Of course, if you still have reservations, we could try alternative approaches, like this suggestion to use separate variables like BATOPEN. (Although in my opinion, the "special case syntax" for having bat use LESSOPEN is redundant and can be replaced with normal shell variable assignment.)

Or maybe something else entirely if you have any suggestions.

@Enselic
Copy link
Collaborator

Enselic commented Sep 8, 2023

Thanks! Let's merge this.

@Enselic Enselic merged commit fe73010 into sharkdp:master Sep 8, 2023
21 checks passed
@sharkdp
Copy link
Owner

sharkdp commented Sep 8, 2023

Thank you @Anomalocaridid the update and @Enselic for the review.

@Anomalocaridid
Copy link
Contributor Author

Thank both of you as well!

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.

3 participants