Do not attempt to find an executable if a script is referenced

[adinapoli]

@andreasabel This is a (potentially naive) attempt at fixing #189 (and perhaps #107?).

This was the result of a quick hacking session so I am sure the story is more complicated than this, and perhaps we should also have @gregwebs chime in on this one.

I am not sure why the switch from system-filepath to filepath triggered it, but the crux of the issue is that unless shelly is run with escaping False, the input FilePath fed into run & co will be "escaped", which in this context means calling the runCommand function, which will attempt to find an executable at PATH.

This works most of the time, but if we have something like ./foo/bar/baz.sh, this won't work, and Shelly will complain.

The easiest fix I could think of was to simply check if we had a FilePath commencing with . and, in this case, we don't attempt to find an executable, but we run the command as-if we used escaping False.

I have also added a regression test, to catch this in the future.

Thoughts?

EDIT: For visibility, here is the postmortem copied from an inline comment:

I have finally nailed down exactly why the regression happened. We have this code, which is responsible for locating the file, in the guts of whichEith:

    whichFull fp = do
      (trace . mappend "which " . toTextIgnore) fp >> whichUntraced
      where
        whichUntraced | isAbsolute fp          = checkFile
                      | dotSlash splitOnDirs   = checkFile
                      | length splitOnDirs > 0 = lookupPath  >>= leftPathError
                      | otherwise              = lookupCache >>= leftPathError

        splitOnDirs = splitDirectories fp
        dotSlash ("./":_) = True
        dotSlash _ = False

As you can see, we are already accounting for dotSlash, so why things fail? Well, it turns out that both system-filepath and filepath have a splitDirectory function, but hey behave differently:

> import qualified Filesystem.Path.CurrentOS as SFP
> import qualified Data.Text as T
> import System.FilePath as FP
> FP.splitDirectories "./test/data/hello.sh"
[".","test","data","hello.sh"]
> SFP.splitDirectories (SFP.fromText $ T.pack "./test/data/hello.sh")
[FilePath "./",FilePath "test/",FilePath "data/",FilePath "hello.sh"]
> 

As you can see the former doesn't include the "./", so the pattern match on dotSlash fails, and here is our regression.

[andreasabel]

CI reports some broken tests under Linux:

Failures:

  test/src/FindSpec.hs:114:22: 
  1) find follow symlinks
       expected: ["dir", "dir/symlinked_dir", "dir/symlinked_dir/hoge_file", "hello.sh", "nonascii.txt", "symlinked_dir", "symlinked_dir/hoge_file", "zshrc"]
        but got: ["dir", "dir/symlinked_dir", "dir/symlinked_dir/hoge_file", "nonascii.txt", "symlinked_dir", "symlinked_dir/hoge_file", "zshrc"]

  To rerun use: --match "/find/follow symlinks/"

  test/src/FindSpec.hs:129:22: 
  2) find not follow symlinks
       expected: ["dir", "dir/symlinked_dir", "hello.sh", "nonascii.txt", "symlinked_dir", "symlinked_dir/hoge_file", "zshrc"]
        but got: ["dir", "dir/symlinked_dir", "nonascii.txt", "symlinked_dir", "symlinked_dir/hoge_file", "zshrc"]

  To rerun use: --match "/find/not follow symlinks/"

  test/src/RunSpec.hs:31:5: 
  3) run script at $PWD
       uncaught exception: ReThrownException

       Ran commands: 
       ./test/data/hello.sh

       Exception: ./test/data/hello.sh: createProcess: exec: invalid argument (Bad file descriptor)

  To rerun use: --match "/run/script at $PWD/"

Could you please have a look what causes these and how to update your PR accordingly, @adinapoli ?

[adinapoli]

In a lucky turn of events I will be working remotely from Rome and my digital nomad workstation is a Linux machine (as opposed to my main Mac mini), so I can definitely look into this one later this week.

Thanks for the heads up!

[gregwebs]

Thanks for adding a test case. With enough testing we can get make a change.

[adinapoli]

@andreasabel @gregwebs Ok, I think we should be in business now. Two failures were due to the fact I had forget to add hello.sh to the data files ( :facepalm: ) whereas the run failure was due to the fact CI didn't have the +x permission set for hello.sh to execute it.

As far as windows testing goes, obviously running that bash script doesn't make sense, so I have guarded the test using isWindows. I am not sure what the best course of action is here.

[adinapoli]

Ok, we do not have to improve on Shelly in this PR, just restore its old behavior...

@andreasabel hehe yes, my temptation would be to eschew any temptation to improve Shelly as part of this PR and instead deliver the smallest fix that restores the old behaviour, and perhaps we flag all these low hanging fruits in a ticket, as those sounds very inviting for newcomers willing to contribute to the project :wink:

[adinapoli]

@andreasabel Ok, I have addressed all the review remarks. Reshuffling that whichFull function also had the added bonus of removing some now-redundant functions.

Something I should point out though is that, at least on my Linux machine, splitDirectories can return a length of 0, which is when the empty string is passed:

> import System.FilePath as FP
> FP.splitDirectories ""
[]
> 

This is very weird, because you also ran the same test here but got something different. Having said that, the tests seem to still all pass, so perhaps that's nothing to worry about.

Furthermore, I couldn't conceivably see why somebody would try to run a command by passing the empty string as the executable (and I agree with your analysis, garbage in, garbage out). A case could be made that Shelly could get rid of these unrepresentable states if it had its own (internal) definition of something like newtype FilePath = FilePath (NonEmpty Char) but again, this might be overkill (or too simplistic) and definitely doesn't belong to this PR :wink:

[adinapoli]

@andreasabel I have lost track of this -- are you folks expecting anything else from me or is this essentially good to go? Thanks! 😉

[adinapoli]

Thanks again @adinapoli and apologies for the long break. I am planning to release this as 1.11.0 now, assuming you haven't experienced any problems with this code in the last 9 month. I rebased this on latest master but otherwise left the commits untouched.

No worries, and thanks for picking this one up 😉 In interest of full transparency, I ended up not using (just yet) this branch/commit, as I try to avoid depending on unreleased/unapproved forks/branches/patches for production code, so I just ended up downgrading to a suitable Shelly, but given how much we spent thinking about this issue, I think it should be fine. We know precisely where and why the regression happened and we have a test to prevent that from happening again, so we should be good 😉

[andreasabel]

Thanks again, @adinapoli !
I pushed the button now, so you have opportunities to evaluate the fix in production now: https://hackage.haskell.org/package/shelly-1.11.0

[adinapoli]

Thanks again, @adinapoli ! I pushed the button now, so you have opportunities to evaluate the fix in production now: https://hackage.haskell.org/package/shelly-1.11.0

Beautiful, I'll keep you posted, thanks!