Multi exec

[tmccombs]

No description provided.

[sharkdp]

Oh - very cool! Thank you for working on this. Let us know when it's ready for review.

[tmccombs]

One issue here is if you run something like:

fd --exec echo directory  '{//}' \; --exec  echo basename '{/}' \;

then you can end up printing baches of directories then batches of basenames instead of writing the directory and base name together, because the commands are run in parallel.

There are a few things we can do about this:

1. Do nothing, and warn users that if they care about the order of output they need to use --threads=1
2. Hold the lock for stdout while running all the --exec commands. Which is not great, since it means commands can't be run in parallel.
3. Concatenate the output from all commands into a string before writing to stdout. Would require some refactoring.
4. Create a Vec of the outputs from the commands, then write ouch each entry with the stdout lock held. Would require some refactoring.

I'm leaning towards 3 or 4, what are your thoughts?

[tmccombs]

My latest commit implements 4. All that remains is writing some tests.

[tmccombs]

Biggest downsides to using 3 or 4 are that it can result in higher memory usage, and potentially delay output from the first exec command(s). There will also be slightly higher overhead for allocating the buffers. That could probably be mitigated somewhat by using a smallvec instead of a Vec. Or possibly using a thread-local arena.

[tavianator]

I think 4 makes sense since we already buffer anyway, might as well buffer everything for each file.

Maybe we should implement a limit to the buffering? Right now fd -x yes will just eat all your memory. But no need to fix that in this PR, it's preexisting.

[sharkdp]

Thank you, I'm planning to take a look soon.

[sharkdp]

I found the following problematic case:

> touch foo
> fd -x echo \; -x
[fd error]: Problem while executing command: Permission denied (os error 13)

(maybe we could improve the error messages here)

Using strace -f -e execve we can see that it's trying to execute:

[pid 107001] execve("./foo", ["./foo"], 0x7ffc494d0718 /* 66 vars */) = -1 EACCES (Permission denied)

If foo is executable, it's actually executed.

[sharkdp]

Benchmarks look good (no change when running a single command) 👍

Command execution

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
./fd-8.3.2 'ab' '/home/shark/Informatik/' --exec echo	357.7 ± 4.7	353.5	370.4	1.00 ± 0.01
./fd-960 'ab' '/home/shark/Informatik/' --exec echo	356.0 ± 1.7	354.3	359.3	1.00

Command execution (large output)

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
./fd-8.3.2 -tf 'ab' '/home/shark/Informatik/' --exec cat	325.7 ± 1.7	323.1	327.9	1.00
./fd-960 -tf 'ab' '/home/shark/Informatik/' --exec cat	326.8 ± 2.6	324.2	333.8	1.00 ± 0.01

[sharkdp]

What is the problem here? Only the path separator (/ vs \)? I think we have code somewhere to replace them automatically?

echo should be available on Windows as well.

[tavianator]

I think echo is only a shell builtin on Windows

[sharkdp]

These tests seem to indicate otherwise, right?

fd/tests/tests.rs

Lines 1480 to 1501 in 0fd7ec5

	/// Shell script execution (--exec) with a custom --path-separator
	#[test]
	fn test_exec_with_separator() {
	let (te, abs_path) = get_test_env_with_abs_path(DEFAULT_DIRS, DEFAULT_FILES);
	te.assert_output(
	&[
	"--path-separator=#",
	"--absolute-path",
	"foo",
	"--exec",
	"echo",
	],
	&format!(
	"{abs_path}#a.foo
	{abs_path}#one#b.foo
	{abs_path}#one#two#C.Foo2
	{abs_path}#one#two#c.foo
	{abs_path}#one#two#three#d.foo
	{abs_path}#one#two#three#directory_foo",
	abs_path = abs_path.replace(std::path::MAIN_SEPARATOR, "#"),
	),
	);

[tavianator]

Well, that test fails on my Windows VM. I bet GIt Bash or MSYS or something installs an echo.exe binary that gets picked up on CI.

[tmccombs]

I was just following what we did in the test_exec and test_exec_batch, and (specificaly) test_exec_batch_with_limit.

[sharkdp]

Alright, I'm fine with skipping Windows for now.

In this case, the only open point I see right now is the problem mentioned above (#960 (comment)).

[sharkdp]

Oh, you already reported it upstream. Thanks!

[sharkdp]

Thank you!

One thing that I would like to discuss (independent from this PR) is the syntax for passing multiple commands. This might be a personal thing, but I really dislike the ; syntax for "ending" the list of arguments. It is ugly, needs escaping, and is hard to type (on German keyboards). It's also potentially confusing since ; has special meaning in a shell so users really need to understand the escaping part here.

fd … -x echo \; -x do_something

Is there any way we can provide something easier here? Using ; for separation has precedent in find, of course, but it's not like that's something universal. It's just a convention.

One option would be to come up with a different value terminator character or sequence (https://docs.rs/clap/latest/clap/struct.Arg.html#method.value_terminator). -- would be one common option, but that has special meaning already. So this would likely not work(?). But let's say we use something that's easier to type and doesn't need escaping:

fd … -x echo :: -x do_something

This would only cause problems if someone actually wants to pass :: to a command. But the same is true for the semicolon.

Another idea might be to provide a way to get rid of both: the value separator and the additional -x/--exec option. For example, we could choose a special character sequence (let's say + or ++) to directly combine two commands:

fd … -x echo + do_something

What do you think?

[tmccombs]

personally I prefer the ;, because it is the same as what find uses, and seems pretty self-explanatory. to be fair, I use an american keyboard, where \; and ';' are pretty easy to type (the semicolon is on the home row).

I do like the idea of having a delimiter so you don't have to specify -x multiple times like fd ... -x echo ++ do_something. ++ is probably rare enough that needing to escape it would be rare.

What do you think of keeping \; to end the options to -x but also allowing you to specify multiple commands delimited by ++ or similar?

[sharkdp]

What do you think of keeping \; to end the options to -x but also allowing you to specify multiple commands delimited by ++ or similar?

I agree. Let's keep \; for now. The other (++) syntax would be interesting to explore. But that can be done in a separate PR. @tavianator Any opinion on this?