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

Fixes and improvements #382

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

piegamesde
Copy link
Contributor

@piegamesde piegamesde commented Apr 17, 2023

Closes #378

@piegamesde
Copy link
Contributor Author

The CI failures don't look like they were introduced by this PR

@andrewchambers
Copy link
Owner

Looks good, thanks for tracking this down, must have been a nightmare.

@piegamesde
Copy link
Contributor Author

Yeah, it was tedious and unpleasant in a lot of ways. Maybe the worst part was learning about how Linux IO works on a low level and how fundamentally broken it actually is.

About the second commit with the error messages, it is far from complete (it mainly contains a few sprinkled annotations that were required for me to track down the bug). I suggest merging it as-is, and strongly encouraging adding more context statements as we go over time.

@piegamesde piegamesde marked this pull request as ready for review April 17, 2023 11:11
@piegamesde

This comment was marked as resolved.

@piegamesde
Copy link
Contributor Author

friendly ping

@jtrees
Copy link

jtrees commented Dec 25, 2023

@andrewchambers Any chance you could take a look another look at this?

Although the examples already disambiguate this, this hopefully makes it more clear that the command is not a single argument which may need to be escaped, but a list of arguments which will be executred
The letter conflicts with --exclude, which would be much better suited for a shorthand.
Unlike --exclude, --exec is only used up to once, less often, and is shorter.
I didn't directly add -e back as a shorthand for --exclude, as I'm not 100% sure yet if we'd want that.
Closes andrewchambers#389. Also implements a duplicates check, therefore fixes andrewchambers#396.
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.

EAGAIN / EWOULDBLOCK when writing logs to stderr, causing failure
3 participants