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 vendored patch for preexec histcontrol #1966

Closed

Conversation

cornfeedhobo
Copy link
Member

Description

This is my approach to pulling in the patch from rcaloras/bash-preexec#119.

I'm open to other ideas.

Motivation and Context

rcaloras/bash-preexec#115

How Has This Been Tested?

locally

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • If my change requires a change to the documentation, I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • If I have added a new file, I also added it to clean_files.txt and formatted it using lint_clean_files.sh.
  • I have added tests to cover my changes, and all the new and existing tests pass.

@cornfeedhobo
Copy link
Member Author

cc @NoahGorny @gaelicWizard @davidpfarrell Please take a look.

The idea here is that I ran the following commands to apply the patch:

$ cd $BASH_IT/vendor/github.com/rcaloras/bash-preexec
$ curl -LO https://github.com/rcaloras/bash-preexec/pull/119.patch
$ patch -p1 < 119.patch

I'm also not really sure why the test is failing in macos, in case one of you feels like taking a look before I get more time.

@NoahGorny
Copy link
Member

This approach is interesting. I wonder what will be the interaction with git-vendor, will the patch file be ignored? how will we handle conflicts when updating?

@cornfeedhobo
Copy link
Member Author

@NoahGorny what are your normal git vendor commands? I can run through some workflows to see what fits, but yes, these are the larger lifecycle questions I want us to answer before proceeding.

@NoahGorny
Copy link
Member

@NoahGorny what are your normal git vendor commands? I can run through some workflows to see what fits, but yes, these are the larger lifecycle questions I want us to answer before proceeding.

I usually just use git vendor update and deal with the aftermath. If the patch file is left alone, and we can then reapply it, I see no reason not to go with your approach. Its better than simply commiting and forgetting about the changes. We can:

  1. Revert our patch.
  2. Update. Hopefully no conflicts should arise.
  3. Apply our patch again and deal with any conflicts.

Let me know what you think @cornfeedhobo 😄

@gaelicWizard
Copy link
Contributor

gaelicWizard commented Oct 10, 2021

I think that git vendor can change branch for an already-vendor'd lib, so maybe just updating to point to the PR branch would be least-trouble for this?

I think there's a bigger discussion about using git vendor as it has so many missing capabilities, but that's a whole other story. For now, using git vendor, I don't see a way to carry a patch other than just keeping a copy of the patch and applying it sometimes.

@gaelicWizard
Copy link
Contributor

@cornfeedhobo, I opened a PR against this PR (cornfeedhobo#1) with some additions to vendor/init.d/preexec.bash

@gaelicWizard
Copy link
Contributor

@cornfeedhobo, I alsö added a fix to not require extglob in my PR against your PR (cornfeedhobo#1)

@cornfeedhobo
Copy link
Member Author

cornfeedhobo commented Oct 20, 2021

@gaelicWizard Thanks for pinging me. Check out my comment. TL;DR: your workaround makes my environment JustWork(tm). Let's get that out ASAP. I look forward to your PR. I'm closing this one.

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

Successfully merging this pull request may close these issues.

3 participants