Allow -exec to work as --exec #226

stevepentland · 2018-01-27T23:38:07Z

This will check for the presence of -exec and change it to --exec prior to passing the args to clap to support the request of #221

There may be some merit to updating the help docs for -e option to ensure it is known that an .xec file extension will require the --extension option instead. But that may not really be needed as .xec files don't really seem to be a common type.

If we encounter -exec, it is simply parsed to be --exec, this ensures that exec works in this fashion, but sadly removes short arg support for all those xec files. Rename function, shorten names of types. Also, do not bother running the map over if no entries are specifically exec.

sharkdp

Cool idea! Thank you very much!

Could you please also add a small test?

With this approach, there is a small danger when someone wants to pass -exec to a command that is called via --exec 😄

sharkdp · 2018-01-28T11:52:06Z

src/main.rs

@@ -170,3 +172,25 @@ fn main() {
        Err(err) => error(err.description()),
    }
 }
+
+fn transform_args_with_exec() -> Vec<OsString> {
+    let target = {


You can use OsString::from("-exec").

Thanks! I had missed that

sharkdp · 2018-01-28T11:53:01Z

src/main.rs

+        ex
+    };
+
+    if !std::env::args_os().any(|v| v == target) {


If we are going to traverse the arguments anyway, I think we can leave out this check. Just try to replace -exec with --exec below.

I've removed this, but please see my comment below

sharkdp · 2018-01-28T11:53:38Z

src/main.rs

+    };
+
+    env::args_os()
+        .map(|v| if v == target { replacement.clone() } else { v })


Instead of replacement.clone(), you can use OsString::from("--exec") here.

sharkdp · 2018-01-28T11:56:04Z

src/main.rs

@@ -170,3 +172,25 @@ fn main() {
        Err(err) => error(err.description()),
    }
 }
+
+fn transform_args_with_exec() -> Vec<OsString> {


Could we add a doc-string here to describe what the function does?

For sure, no problem

stevepentland · 2018-01-28T14:50:39Z

It is true that -exec passed as an argument to a previous --exec option of fd would result in an unexpected change of the given parameter. My initial thought was to only change the first instance of -exec, but as exec is a parameter that can be repeated this would not work.

I suppose it comes down to how much weight we want to give to -exec appearing as an arg to another command? Should it be similar to .xec where we simply don't worry about the possibility? Or would limiting the number of --exec occurrences to one be a potential solution for better consistency (causing it's own problems when coming before positional arguments however)? If multiple is desired, and we are worried about changing -exec given to the --exec target, then a more complex solution such as a full-on pre-parser seem to be the only option left.

sharkdp · 2018-01-28T14:56:27Z

My initial thought was to only change the first instance of -exec, but as exec is a parameter that can be repeated this would not work.

Oh, actually --exec should only be allowed once. I think you discovered a bug here. It seems like currently fd -x cmd1 -x cmd2 is actually equal to fd -x cmd1 cmd2. We might restrict it to just one usage of --exec/-x.

I suppose it comes down to how much weight we want to give to -exec appearing as an arg to another command? Should it be similar to .xec where we simply don't worry about the possibility?

I'd be okay with that.

Or would limiting the number of --exec arguments to one be a potential solution for better consistency?

I think that would be even better.

Also, limit usage of --exec option to single instance.

stevepentland · 2018-01-28T18:10:38Z

Ok, so I've tried to reach a middle ground with the handling. I have limited --exec to a single occurrence (as opt for fd). This means I am not able to only transform the first instance of -exec as it will no longer cause an error in clap. I've added some tests, both to the handling of the incoming args, and the overall operation. An option of -exec passed to a secondary command will also be transformed, but at least this allows the downstream to provide it's own error explanation.

sharkdp · 2018-01-28T18:50:58Z

This means I am not able to only transform the first instance of -exec

Why not? Clap will not throw an error because it knows that the other -exec is just a value given to the exec-argument.

A possible implementation could look like this:

/// Replace the first occurence of `-exec` with `--exec`.
fn transform_args_with_exec<I>(original: I) -> Vec<OsString>
where
    I: Iterator<Item = OsString>,
{
    let mut new_args: Vec<OsString> = original.collect();

    let target = OsString::from("-exec");
    let result = new_args.iter().position(|arg| *arg == target);

    if let Some(pos) = result {
        new_args[pos] = OsString::from("--exec");
    }

    return new_args;
}

(you can use this in your tests via transform_args_with_exec(original.into_iter()))

It works as expected:

▶ fd -exec echo -exec --exec    
-exec --exec README.md
-exec --exec src
-exec --exec LICENSE-MIT
...

This will actually still leave us with a problem if someone passes the actual --exec/-x to fd but -exec to the command (as it will now replace the wrong -exec):

▶ fd --exec echo -exec --exec    
--exec --exec README.md
--exec --exec src
--exec --exec LICENSE-MIT
...

sharkdp · 2018-01-28T18:54:39Z

To be 99.9% foolproof, we could actually only replace the first -exec by --exec if neither -x nor --exec is in the arguments vector. This would only fail if someone (wrongly) uses -exec for fd but -x or --exec for another program.

stevepentland · 2018-01-28T19:00:34Z

When only the first is changed, clap will later on see the second -exec passed to fd as --extension instead. As I removed the .multiple from the exec arg, it is now relying on the validation from clap. Currently this means the only error that will occur is when someone tries to pass -exec to the given executable and not fd

sharkdp · 2018-01-28T19:02:44Z

Oh, you mean if someone passes -exec to fd twice? Like to execute two commands for each search result?

stevepentland · 2018-01-28T19:03:48Z

Oh, you mean if someone passes -exec to fd twice? Like to execute two commands for each search result?

Yes, this causes a strange hybrid behavior in which we have new action for the first -exec but then the same error from the original bug for any further -exec

stevepentland · 2018-01-28T19:04:50Z

I figured that leaving any errors to the downstream makes at least our behavior consistent, and since -exec isn't the most common option (I only know of find) then this is probably the least impactful

stevepentland · 2018-01-28T20:54:19Z

@sharkdp I've created a small state machine that will keep track of it's current position (in exec option or out) and properly convert -exec as expected. It will need some cleanup and some code should be moved around, but it could do the trick.

sharkdp · 2018-01-28T21:08:25Z

Oh wow, nice! 👍

(we could move that code to the internal module)

stevepentland · 2018-01-28T21:15:55Z

Oh wow, nice! 👍

Thanks! I've actually gone over it again and reduced it to a single fold that does the same thing without all the extra junk. It's pretty concise and works as expected, we change any -exec to --exec, but not when inside of a previous --exec, -exec, or -x block, delimited with the terminator ;

Doxterpepper · 2018-01-28T21:50:06Z

Have you considered using the validator method in clap to solve this problem?

https://docs.rs/clap/2.29.2/clap/struct.Arg.html#method.validator

stevepentland · 2018-01-28T21:52:17Z

Have you considered using the validator method in clap to solve this problem?

I had looked at it, but it does not seem to provide a way to change one argument into another. It's not really validation that is being looked for, rather to keep some level of similarity to the standard find utility.

stevepentland · 2018-01-29T18:16:30Z

@sharkdp This is pretty much done for what we wanted to do. Any more changes you'd like me to make?

sharkdp · 2018-01-29T19:33:09Z

No, it looks great.

Thank you very much!

stevepentland changed the title ~~Enable -exec to work as --exec~~ Allow -exec to work as --exec Jan 27, 2018

sharkdp self-requested a review January 28, 2018 11:51

sharkdp requested changes Jan 28, 2018

View reviewed changes

Changes related to code review, add tests.

86705d7

Also, limit usage of --exec option to single instance.

stevepentland added 2 commits January 28, 2018 14:12

Make transform generic over the iterator type

3932b63

Test simple little state machine to convert -exec in proper context

885bfbd

Cleanup code, remove unneeded struct and simply fold original iterator

c52a995

stevepentland added 3 commits January 28, 2018 16:17

Remove commented test, this version is superior to original anyway

f9d7a43

Remove else block that didn't do anything anyway

a2351ff

Move transform fn to internal module

09f73a7

sharkdp mentioned this pull request Jan 29, 2018

Misleading error when typing -exec #221

Closed

sharkdp approved these changes Jan 29, 2018

View reviewed changes

sharkdp merged commit b4d3927 into sharkdp:master Jan 29, 2018

stevepentland deleted the exec_arg branch January 29, 2018 22:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow -exec to work as --exec #226

Allow -exec to work as --exec #226

stevepentland commented Jan 27, 2018 •

edited

Loading

sharkdp left a comment

sharkdp Jan 28, 2018

stevepentland Jan 28, 2018

sharkdp Jan 28, 2018

stevepentland Jan 28, 2018

sharkdp Jan 28, 2018

sharkdp Jan 28, 2018

stevepentland Jan 28, 2018

stevepentland commented Jan 28, 2018 •

edited

Loading

sharkdp commented Jan 28, 2018

stevepentland commented Jan 28, 2018

sharkdp commented Jan 28, 2018 •

edited

Loading

sharkdp commented Jan 28, 2018

stevepentland commented Jan 28, 2018

sharkdp commented Jan 28, 2018

stevepentland commented Jan 28, 2018

stevepentland commented Jan 28, 2018

stevepentland commented Jan 28, 2018

sharkdp commented Jan 28, 2018 •

edited

Loading

stevepentland commented Jan 28, 2018 •

edited

Loading

Doxterpepper commented Jan 28, 2018 •

edited

Loading

stevepentland commented Jan 28, 2018

stevepentland commented Jan 29, 2018

sharkdp commented Jan 29, 2018

Allow -exec to work as --exec #226

Allow -exec to work as --exec #226

Conversation

stevepentland commented Jan 27, 2018 • edited Loading

sharkdp left a comment

Choose a reason for hiding this comment

sharkdp Jan 28, 2018

Choose a reason for hiding this comment

stevepentland Jan 28, 2018

Choose a reason for hiding this comment

sharkdp Jan 28, 2018

Choose a reason for hiding this comment

stevepentland Jan 28, 2018

Choose a reason for hiding this comment

sharkdp Jan 28, 2018

Choose a reason for hiding this comment

sharkdp Jan 28, 2018

Choose a reason for hiding this comment

stevepentland Jan 28, 2018

Choose a reason for hiding this comment

stevepentland commented Jan 28, 2018 • edited Loading

sharkdp commented Jan 28, 2018

stevepentland commented Jan 28, 2018

sharkdp commented Jan 28, 2018 • edited Loading

sharkdp commented Jan 28, 2018

stevepentland commented Jan 28, 2018

sharkdp commented Jan 28, 2018

stevepentland commented Jan 28, 2018

stevepentland commented Jan 28, 2018

stevepentland commented Jan 28, 2018

sharkdp commented Jan 28, 2018 • edited Loading

stevepentland commented Jan 28, 2018 • edited Loading

Doxterpepper commented Jan 28, 2018 • edited Loading

stevepentland commented Jan 28, 2018

stevepentland commented Jan 29, 2018

sharkdp commented Jan 29, 2018

stevepentland commented Jan 27, 2018 •

edited

Loading

stevepentland commented Jan 28, 2018 •

edited

Loading

sharkdp commented Jan 28, 2018 •

edited

Loading

sharkdp commented Jan 28, 2018 •

edited

Loading

stevepentland commented Jan 28, 2018 •

edited

Loading

Doxterpepper commented Jan 28, 2018 •

edited

Loading