[darcs-users] [patch178] Resolve issue 1397: distinguish between "no arguments"...

[Alexey Levan]

Alexey Levan <exlevan at gmail.com> added the comment:

Thanks for the review! So, I looked what's going on with fixSubPaths...

Actually, there's no problem with the fixSubPaths function - it just
filters out incorrect absolute paths, and it does it just fine. However,
the real problem is how this function is (mis)used.

There's no consistent policy of using fixSubPaths, and if you use darcs
command with invalid absolute paths, the following may happen:
  * everything is OK, because invalid paths are handled explicitly
(Record.lhs, Rollback.lhs)
  * everything is OK, because command with no args isn't special case
(Diff.lhs)
  * you get message like "Nothing specified, nothing to do". Message is
misleading, but nothing bad happens (Remove.lhs, Add.lhs)
  * the worst: you get error (Move.lhs, try 'darcs move /foo /bar /baz'!).

So, how do we fix all this mess? I propose following:
  1. In Darcs/Arguments.lhs, create function 'maybeFixSubPaths ::
[DarcsFlag] -> [FilePath] -> IO [Maybe SubPaths]', which maps invalid
paths to Nothing.
  2. fixSubPaths gets implemented in terms of maybeFixSubPaths:
fixSubPaths flags fs = catMaybes <$> maybeFixSubPaths flags fs
  3. In each command that takes filenames as arguments, implement
commands with different semantics (such as 'darcs changes' and 'darcs
changes [args]') as different functions, and pass them already checked
arguments, doing all error handling before that. Here's example draft
for darcs move:

moveCmd opts args = case () of
  _ | length args < 2 -> fail "The `darcs move' command requires at
least two arguments."
    | length args == 2 -> do
        case maybeFixSubPaths args of
          [Just from, Just to] -> if {- to is dir -}
            then moveFileToDir opts from to
            else moveFile opts from to
          _ -> fail {- both args must be valid -}
    | length args > 2 -> let (froms, to) = (init args, last args) in
        case maybeFixSubPath to of
          Nothing -> fail "invalid destination directory"
          Just to' -> case fixSubPaths froms of
            [] -> fail "Nothing to move"
            froms' -> moveFilesToDir opts froms' to'
-- functions doing all the work
moveFile :: [DarcsFlag] -> SubPath -> SubPath -> IO ()
moveFileToDir :: [DarcsFlag] -> SubPath -> SubPath -> IO ()
moveFilesToDir :: [DarcsFlag] -> [SubPath] -> SubPath -> IO ()

That way all error handling and args mangling would became separate from
the code that actually does some work. That's plenty of work, but I
think only that way will resolve all those 'invalid absolute path' bugs.
As a bonus, code in Commands/ will become much more clear and maintainable.

So, if nobody's mind, I'll start working on it. Or maybe there are
better ideas around?

__________________________________
Darcs bug tracker <bugs at darcs.net>
<http://bugs.darcs.net/patch178>
__________________________________