denisidoro / navi

An interactive cheatsheet tool for the command-line
Apache License 2.0
15.12k stars 505 forks source link

repo browser works poorly on paths with spaces #675

Open dhduvall opened 2 years ago

dhduvall commented 2 years ago

Describe the bug On MacOS, the repo browser can't show the cheat files.

To Reproduce Steps to reproduce the behavior:

  1. Run navi repo browse
  2. Select a repo
  3. Select No (you don't want to import all files)
  4. In the right-hand panel, see most of an error:
    cat: /Users/duvall/Library/Application Support/navi/cheats/tmp//User1/2u
    cat: Support/navi/cheats/tmp/misc/crontab.cheat: No such file or directo
  5. If I make the font small enough, I can see the whole thing:
    cat: /Users/duvall/Library/Application Support/navi/cheats/tmp//Users/duvall/Library/Application:1/2
    cat: Support/navi/cheats/tmp/misc/crontab.cheat: No such file or directory

Expected behavior To see the contents of the selected cheat file in the right-hand panel

Screenshots If applicable, add screenshots to help explain your problem.

Versions:

Additional context This is reproducible with

find ~/Library/Application\ Support/navi/cheats/tmp/pages -type f | sk --preview "cat '{}'"

(also with fzf). That repro is fixable by adding an extra set of single quotes:

find ~/Library/Application\ Support/navi/cheats/tmp/pages -type f | sk --preview "cat ''{}''"

but simply applying that change to the source doesn't fix the issue:

diff --git a/src/handler/repo_add.rs b/src/handler/repo_add.rs
index 8125da2..b9b4df0 100644
--- a/src/handler/repo_add.rs
+++ b/src/handler/repo_add.rs
@@ -55,7 +55,7 @@ pub fn main(uri: String) -> Result<()> {

     let opts = FinderOpts {
         suggestion_type: SuggestionType::MultipleSelections,
-        preview: Some(format!("cat '{}/{{}}'", tmp_path_str)),
+        preview: Some(format!("cat ''{}/{{}}''", tmp_path_str)),
         header: Some("Select the cheatsheets you want to import with <TAB> then hit <Enter>\nUse Ctrl-R for (de)selecting all".to_string()),
         preview_window: Some("right:30%".to_string()),
         ..Default::default()

What does work is to do that and get rid of the path prefix:

diff --git a/src/handler/repo_add.rs b/src/handler/repo_add.rs
index 8125da2..ad2a6f7 100644
--- a/src/handler/repo_add.rs
+++ b/src/handler/repo_add.rs
@@ -55,7 +55,7 @@ pub fn main(uri: String) -> Result<()> {

     let opts = FinderOpts {
         suggestion_type: SuggestionType::MultipleSelections,
-        preview: Some(format!("cat '{}/{{}}'", tmp_path_str)),
+        preview: Some(format!("cat ''{{}}''")),
         header: Some("Select the cheatsheets you want to import with <TAB> then hit <Enter>\nUse Ctrl-R for (de)selecting all".to_string()),
         preview_window: Some("right:30%".to_string()),
         ..Default::default()

but I don't know what unintended consequences that would have.

denisidoro commented 2 years ago

I'm surprised it's working as-is (aside the bug you mentioned). A few lines below this one I'm prepending the tmp path for the second time, which is unintended.

I'll probably accept your suggestion. Hopefully one of the integration tests will be able to capture bugs, if any.