Respect exit codes with --exec-batch
#1137
Conversation
| @@ -196,14 +198,20 @@ impl CommandBuilder { | |||
| fn finish(&mut self) -> io::Result<()> { | |||
| if self.count > 0 { | |||
| self.cmd.try_args(&self.post_args)?; | |||
| self.cmd.status()?; | |||
| if !self.cmd.status()?.success() { | |||
| self.exit_code = ExitCode::GeneralError; | |||
There was a problem hiding this comment.
Rather than storing an exit code here, I think we should return an Err.
There was a problem hiding this comment.
Hmm, my idea went like this: We do not want to short-circuit on non-success exit codes (compare -x semantics), so we were to return an Err, we would have to catch it (but just this one, not other Errs) in execute_batch and still maintain sth like this exit_code there. But this seemed just more convoluted, so I took this approach.
Happy to change though if you have a different idea 👍
There was a problem hiding this comment.
The way -x works is if a command fails, we short circuit for that particular path.
So for example if you have fd foo -x c1 \; -x c2 and c1 fails for a specific path, then c2 won't be run for that path. But it will continue trying to run c1 for other paths
I think we would probably want similar behavior for exec-batch. If one command fails, we probably don't want to run the next one in the series, because it might depend on the first one succeeding.
However, maybe we do want to run the commandset with the next batch of paths if we have more paths than can fit in a single command invocation?
I created #1138 which will short circuit.
There was a problem hiding this comment.
I was actually only thinking about batching in my comment above, thanks for mentioning multiple commands.
However, maybe we do want to run the commandset with the next batch of paths if we have more paths than can fit in a single command invocation?
Yes, I think that we should run either all or no batches of any given command, to be consistent with -x (where one can be sure that a command is either executed for all files or for none).
So in short, it seems reasonable to have short circuiting for mulitple commands, but no short circuiting for multiple batches of the same command.
I can update this PR if you agree, but if short circuiting batches is prefered, then #1138 seems nicer 👍
There was a problem hiding this comment.
I don't have a very strong preference on whether or not we short circuit for batches for a single command. @sharkdp do you have any thoughts on it?
There was a problem hiding this comment.
Ah.... I didn't think about that. Now I think 3 might be the right way to go, at least for batches.
Or possibly we change the way batches work, and we run all batches for each command before moving on to the next one, but that could hurt performance.
There was a problem hiding this comment.
Or possibly we change the way batches work, and we run all batches for each command before moving on to the next one, but that could hurt performance.
That's not a bad idea, but it's probably material for another PR :). I don't think the perf impact would be too bad
There was a problem hiding this comment.
Okay. So how do we proceed with this PR? I think it would be great to add some tests for the batch-behavior as well. We can use --batch-size=N for that.
There was a problem hiding this comment.
So I understand it correctly that the semantics of this PR (no short circuiting at all, i.e. @sharkdp option 3.) are good for now? In that case, I will add a few tests ensuring that all commands are executed even if only some commands/batches fail 👍
| self.cmd.status()?; | ||
| if !self.cmd.status()?.success() { | ||
| self.exit_code = ExitCode::GeneralError; | ||
| } | ||
|
|
||
| self.cmd = Self::new_command(&self.pre_args)?; | ||
| self.count = 0; |
There was a problem hiding this comment.
If we do go with this approach, then I think we might need to reset the exit code here
|
I'm going to merge this as-is, because it's definitely better than what we have on master. If you feel like adding those tests later @amesgen, that would be very much appreciated. But please don't feel obliged. |
Closes #1136