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

Odd error message when attempting to pass full path to --editor #77

Open
assarbad opened this issue Nov 14, 2023 · 5 comments
Open

Odd error message when attempting to pass full path to --editor #77

assarbad opened this issue Nov 14, 2023 · 5 comments

Comments

@assarbad
Copy link
Contributor

assarbad commented Nov 14, 2023

I get the following (found while looking into #76), running from cmd.exe.

>renamer -p -e "%LOCALAPPDATA%\Programs\Microsoft VS Code\code.exe" *
Error: Failed to execute editor command: 'C:UsersOliverAppDataLocalProgramsMicrosoft VS Codecode.exe'

Caused by:
    program not found

The same can be got via pwsh.exe:

$ & renamer -p -e "$env:LOCALAPPDATA\Programs\Microsoft VS Code\code.exe" *
Error: Failed to execute editor command: 'C:UsersOliverAppDataLocalProgramsMicrosoft VS Codecode.exe'

Caused by:
    program not found

It appears that either the output of the path interpolates the backslashes somehow or the path gets interpolated by renamer (1.6.5) removing all backslashes.

@mtimkovich My current working theory is that shell_words::split() and/or shell_words::join() are coming back to haunt us in this case. Mind you, I introduced those 😑. It may be sufficient to make sure that all backslashes are doubled on Windows. I'll have a look.

@assarbad
Copy link
Contributor Author

assarbad commented Nov 14, 2023

@mtimkovich my hunch seems to have been right. It can be addressed using this:

diff --git a/src/main.rs b/src/main.rs
index 583ee64..b0c98f0 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -253,7 +253,11 @@ fn open_editor(
         write!(tmpfile, "{}", input_files.join("\n"))?;
     }

-    let editor_parsed = shell_words::split(editor_string)
+    let editor_path = Path::new(editor_string);
+    let editor_quoted_path = shell_words::quote(&editor_string).to_string();
+    let editor_string = if editor_path.exists() { editor_quoted_path } else { editor_string.to_string() };
+
+    let editor_parsed = shell_words::split(&editor_string)
         .expect("failed to parse command line flags in EDITOR command");
     tmpfile.seek(SeekFrom::Start(0))?;
     let child = Command::new(&editor_parsed[0])

What do you think? I'm sure you have a much deeper understanding of Rust than I have, so changes are improvements can be made.

This is still far from ideal, because it will also give the same weird output for the EDITOR command with the backslashes "eaten up". But using shell_word::quote() unconditionally doesn't seem like the right thing to do. Does it?

PS: tested with .\renamer -p -e "%LOCALAPPDATA%\Programs\Microsoft VS Code\code.exe" * (debug config).

@assarbad
Copy link
Contributor Author

Thinking about this some further, I'm more and more convinced that renamer should either do this only on Unix-like systems or, provided we don't want to exclude Cygwin, MSYS2 et. al., on Windows needs to check for the SHELL variable to detect if it is non-empty and contains some known value pointing to a Unix-y shell ...

Any input, @mtimkovich?

@mtimkovich
Copy link
Collaborator

I'm doing some research into how other tools handle this issue and I'll report back

@mtimkovich
Copy link
Collaborator

This person ran into the same problem as us. What do you think about using their package if the user is on Windows?

https://github.com/chipsenkbeil/winsplit-rs

@assarbad
Copy link
Contributor Author

I also found that crate, would that be a valid solution to you? Personally I am fairly conservative when it comes to adding new dependencies. That's why I ask.

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

No branches or pull requests

2 participants