Fix createDirectoryIfMissing function

[jpvillaisaza]

see #4

[hvr]

What was the original problem and how can it be reproduced?

[hvr]

Oh, I guess this is about #4

[jpvillaisaza]

Yes, it's about #4.

[hvr]

Tbh, I'm not convinced this is a proper fix (yet)...

[jpvillaisaza]

@hvr What do you think or what do you mean by yet? I think the permission error could be handled differently, but it should be separate from the already exists error, and it shouldn't end in a return () so the error is not ignored.

[jpvillaisaza]

@hvr Or maybe just remove the catch that's doing the return () (so a permission error is not ignored) and take a closer look at the idea of separating both errors (taking into account that there's a comment saying something about Windows with the permission error).

[hvr]

@jpvillaisaza I've been digging through the Git history, and stumbled over

b513fe8 which was the commit where @dcoutts introduced that confusing throw (NB: it's not a throwIO, which I'm not sure if it was a deliberate hack or not), wrapped by a silencing catch. I consider this highly suspicious.

The isPermissionError case was hacked in several years later via f85cd29 to try to workaround windows-specific semantics.

In any case, I'd like to better understand the reasoning behind b513fe8 before changing anything here.

[jpvillaisaza]

@hvr f85cd29 explains why the two errors should be handled together in Windows, as you mentioned.

[jpvillaisaza]

@hvr To be more precise, https://github.com/jpvillaisaza/directory/commit/f18994e8259593b8026fe02e438a6f5b76de8c1e could be reverted. The only modification would be https://github.com/jpvillaisaza/directory/commit/6d1fc9bb0e4e935f1eaceb61ef4bbb5984aec130, which could be discussed more.

[hvr]

After a conversation with @dcoutts here's one suggestion how to fix it:

    createDir dir notExistHandler = do
      r <- E.try $ createDirectory dir
      case (r :: Either IOException ()) of
        Right ()                   -> return ()
        Left  e
          | isDoesNotExistError  e -> notExistHandler e

          | isAlreadyExistsError e || isPermissionError e -> do
#ifdef mingw32_HOST_OS
              canIgnore <- (withFileStatus "createDirectoryIfMissing" dir isDirectory)
#else
              canIgnore <- (Posix.isDirectory `fmap` Posix.getFileStatus dir)
#endif
                           `catch` ((\ _ -> return False) :: IOException -> IO Bool)
              unless canIgnore (throwIO e)
          | otherwise              -> throwIO e

[dcoutts]

Ok, it's actually more subtle.

It looks to me like we need to split the isAlreadyExistsError e case from the isPermissionError e case.

In the isAlreadyExistsError e case we need to use

canIgnore <- (...)
                      `catch` (\_ -> return True)

while apparently in the isPermissionError e case we need the other way around:

canIgnore <- (...)
                      `catch` (\_ -> return False)

which actually is the same way round as the doesDirectoryExist function.

Why is this? Well, in the isAlreadyExistsError case, if doing the stat() tells us that in fact the file does not exist (e.g. a race with other code that deleted it) then it's safe to ignore. Note that we changed it to be this way round in 2008 because of problems with racing delete vs createDirectoryIfMissing.

But for isPermissionError, the reason stat would fail would be again due to a permission error, and so in that case we still want to throw the exception.

[jpvillaisaza]

This pull request is no longer necessary. See 1f11393.