X.P.RunOrRaise: Disambiguate directories and executables

[slotThe]

X.P.RunOrRaise: Disambiguate directories and executables

When there is a directory and an executable of the same name foo, we display foo, as well as foo/ in the prompt, but selecting either one of them will invariably run or raise the directory, as this test is done beforehand.

Since directories already carry a trailing forward slash from the shell prompt's getShellCompl, this is easily disambiguated by incorporating an additional check.

It is duly noted that the same problem also exists for files and executables. This may be fixed in a similar way (adding a unique suffix to files and checking for that), but since this would involve changing getShellCompl and no-one has complained about this so far, it was deemed "not worth it" for now.

Fixes: https://github.com/xmonad/xmonad-contrib/issues/616

Checklist

• [x] I've read CONTRIBUTING.md

• [x] I've considered how to best test these changes (property, unit, manually, ...) and concluded: I tested it manually, waiting for confirmation of @yusi1 as well.

• [ ] I updated the CHANGES.md file TODO

• [n/a] I updated the XMonad.Doc.Extending file (if appropriate)

[liskin]

Also, can you elaborate on the idea of changing getShellCompl? Did you mean to somehow disamiguate $PATH completions from absolute/relative filename/dirname completions?

[slotThe]

It's not files vs. directories vs. executables, it's about executables in $PATH conflicting with files/directories in the current directory.

Isn't that files vs. directories vs. executables (of the same name)? I'm a bit confused, but do feel free to suggest ways to make this clearer

Also, can you elaborate on the idea of changing getShellCompl? Did you mean to somehow disamiguate $PATH completions from absolute/relative filename/dirname completions?

Yes, the easiest option would probably be to stick a * at the end of "normal" files:

diff --git a/XMonad/Prompt/Shell.hs b/XMonad/Prompt/Shell.hs
index 39ce2d47..f1b6cae1 100644
--- a/XMonad/Prompt/Shell.hs
+++ b/XMonad/Prompt/Shell.hs
@@ -155,7 +155,7 @@ shellComplImpl csn filterFiles cmds cmpgenStr input
         choices <- filterFiles . lines <$> compgenFiles csn cmpgenStr
         files   <- case choices of
             [x] -> do fs <- getFileStatus (encodeString x)
-                      pure $ if isDirectory fs then [x ++ "/"] else [x]
+                      pure $ if isDirectory fs then [x ++ "/"] else [x ++ "*"]
             _   -> pure choices
         pure . sortBy typedFirst . uniqSort $ files ++ cmds
   where

[liskin]

Isn't that files vs. directories vs. executables (of the same name)? I'm a bit confused, but do feel free to suggest ways to make this clearer

Not really, no. The problem is in mixing executable names in $PATH and relative file/dir names in the same list of completions.

In bash, there's no such confusion: https://www.gnu.org/software/bash/manual/html_node/Command-Search-and-Execution.html When you type "xterm↵", it always looks for "xterm" in $PATH (after trying aliases, functions and builtins), even if there's an "xterm" executable in the current directory. When you type "x/xterm↵", there's a slash, so it doesn't search for it and executes it directly.

But in here, we don't have that slash distinction, so we need to disambiguate. But we're not disambiguating between "directories and files and executables", because "executables" are just files, we're disambiguating between "relative paths (files/dirs) and executables/commands in $PATH". It's probably just a wording nitpick, as all you need to do is change "executables" to "executables in $PATH" or "commands in $PATH" (or perhaps just "commands", although that's almost as unclear as "executables").

Anyway, as to whether this is worth dealing with, I'd think that preferring commands in $PATH to both files/dirs in the current directory would be the most natural thing as it's closer in behaviour to a shell. Adding a suffix like the attached patch would probably work, but it's a bit ugly. Can we perhaps refactor the check in RunOrRaise to try https://hackage.haskell.org/package/directory-1.3.6.2/docs/System-Directory.html#v:findExecutable first (you'll need to also check for a slash not being present as findExecutable doesn't do that)? That seems more like what the shell does and what I think might be the expected behaviour. Haven't really thought about what might break, though.

(That being said, absolutely do feel free to merge the fix you have so far, with improved commit message wording, and leave the rest for later.)

[slotThe]

Not really, no. The problem is in mixing executable names in $PATH and relative file/dir names in the same list of completions.

Ah I see; thanks for the explanation.

Anyway, as to whether this is worth dealing with, I'd think that preferring commands in $PATH to both files/dirs in the current directory would be the most natural thing as it's closer in behaviour to a shell. Adding a suffix like the attached patch would probably work, but it's a bit ugly. Can we perhaps refactor the check in RunOrRaise to try https://hackage.haskell.org/package/directory-1.3.6.2/docs/System-Directory.html#v:findExecutable first (you'll need to also check for a slash not being present as findExecutable doesn't do that)? That seems more like what the shell does and what I think might be the expected behaviour. Haven't really thought about what might break, though.

Since the old code definitely executes the file when it's an executable, I think this check can actually be done up-front without breaking anything; I did that in c83a46a92fe8e77567a97ba87016f71862ccd08f.

The problem with files in the current directory having the same name as something in $PATH is that right now they won't even show up in the prompt since we don't show the user paths, but only names of files/directories. We would need to at least mark these "duplicates" somehow, which would involve messing with X.P.Shell internals and I'm really not up for that right now :)

EDIT: As for breaking current functionality I'm hoping that @yusi1 will confirm that at least their use-case still works

[liskin]

Since the old code definitely executes the file when it's an executable, I think this check can actually be done up-front without breaking anything; I did that in c83a46a.

The old code does, but the new code doesn't always. If you try to run e.g. bin/something, the old code would decide it's not a normal file and execute it (but not raise it! pidof bin/something won't find the pid). The new code doesn't do the executable check, so it'll invoke xdg-open bin/something instead. I have absolutely no idea if this breaks any meaningful workflows, though: stuff in ~/bin will likely be on the $PATH, and also raising won't work, so…

[slotThe]

I'm willing to take that risk :)

[liskin]

There was a comment here from @yusi1 saying that this breaks inputs like /usr/bin/firefox, which it definitely does, and since it's a full path, pidof accepts that and raising would work before as well. That comment is now lost, or at least I don't see it now. Reintroducing the getPermissions check would fix this.

Anyway, I'm happy to merge this as it is, but I do need to say that the commit message is still the same as it was before we fixed the files vs. commands ambiguity, so it now claims that a problem still exists which it no longer does. :-)

[slotThe]

I do need to say that the commit message is still the same as it was before we fixed the files vs. commands ambiguity, so it now claims that a problem still exists which it no longer does. :-)

The problem still exists insofar as what I said above about displaying files in path and files in the current directory:

The problem with files in the current directory having the same name as something in $PATH is that right now they won't even show up in the prompt since we don't show the user paths, but only names of files/directories. We would need to at least mark these "duplicates" somehow

so I figured the commit message still applied