marcusbuffett / pipe-rename

Rename your files using your favorite text editor
https://crates.io/crates/pipe-rename
MIT License
396 stars 12 forks source link

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

Open assarbad opened 11 months ago

assarbad commented 11 months ago

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 commented 11 months ago

@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 commented 11 months ago

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 commented 11 months ago

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

mtimkovich commented 11 months ago

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 commented 11 months ago

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.