Skip to content

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