New version of patch, with tests and some commands added. (I didn't check for
conflicts with path156, because it seems to conflict with HEAD).

7 patches for repository http://darcs.net:

Tue Mar 16 02:38:17 EET 2010  Alexey Levan <exlevan@gmail.com>
  * Add Maybe variant of fixSubPaths

Wed Mar 17 00:02:11 EET 2010  Alexey Levan <exlevan@gmail.com>
  * Separate argument handling from repository work in Move.lhs

Sat Mar 20 23:28:53 EET 2010  Alexey Levan <exlevan@gmail.com>
  * Add test for absolute paths

Sun Mar 21 00:09:44 EET 2010  Alexey Levan <exlevan@gmail.com>
  * Separate arguments/repository code in AmendRecord.lhs

Sun Mar 21 00:10:01 EET 2010  Alexey Levan <exlevan@gmail.com>
  * Separate arguments/repository code in Annotate.lhs

Sun Mar 21 00:10:08 EET 2010  Alexey Levan <exlevan@gmail.com>
  * Separate arguments/repository code in Add.lhs

Sun Mar 21 00:10:14 EET 2010  Alexey Levan <exlevan@gmail.com>
  * Resolve issue1397: separate arguments/repository code in Changes.lhs

Attachments

• add-maybe-variant-of-fixsubpaths.dpatch
• unnamed

This is amendment of patch178, forgot to set subject:(

On Sat, 20 Mar 2010, Alexey Levan wrote:

> Alexey Levan <exlevan@gmail.com> added the comment:
>
> This is amendment of patch178, forgot to set subject:(

You can sort this out in the patch tracker by setting patch178 to 
obsoleted and putting 191 as the superseder.

Cheers,

Ganesh

3 patches for repository http://darcs.net:

Thu Mar 25 22:25:56 EET 2010  Alexey Levan <exlevan@gmail.com>
  * Separate arguments/repository code in Annotate.lhs

Thu Mar 25 22:26:01 EET 2010  Alexey Levan <exlevan@gmail.com>
  * Separate arguments/repository code in Diff.lhs

Fri Mar 26 16:26:06 EET 2010  Alexey Levan <exlevan@gmail.com>
  * darcs dist doesn't take any arguments

Attachments

• separate-arguments_repository-code-in-annotate_lhs.dpatch
• unnamed

Hi Alexey,

I'm very sorry about the delay reviewing this.

I like the general idea behind the patch, but I'm not going to apply it
yet, primarily because it conflicts with patch156.  Do you think you
could have a second look at that?

I think the most important patches are
  http://bugs.darcs.net/file1101/remove-implementation-of-__store_in_memory_-simplifying-matcher-code_.dpatch
  http://bugs.darcs.net/file1103/re_implement-setscriptsexecutable-using-trees-instead-of-slurpies_.dpatch

(One handy trick thanks to Trent is to save the patch as an mbox file
and open in your mail client.)

Meanwhile, I'll comment on your patches below.

One general point is that I get the feeling that despite this change, we
still don't have a sense of clarity on how all the commands should deal
with their fileargs.  I'd like to see us moving towards something a
little more homogeneous if this is possible (and it may not be because
the commands may have fundamentally different needs).

> Tue Mar 16 02:38:17 EET 2010  Alexey Levan <exlevan@gmail.com>
>   * Add Maybe variant of fixSubPaths
> 
> Wed Mar 17 00:02:11 EET 2010  Alexey Levan <exlevan@gmail.com>
>   * Separate argument handling from repository work in Move.lhs
> 
> Sat Mar 20 23:28:53 EET 2010  Alexey Levan <exlevan@gmail.com>
>   * Add test for absolute paths
> 
> Sun Mar 21 00:09:44 EET 2010  Alexey Levan <exlevan@gmail.com>
>   * Separate arguments/repository code in AmendRecord.lhs

Reviewed

> Sun Mar 21 00:10:01 EET 2010  Alexey Levan <exlevan@gmail.com>
>   * Separate arguments/repository code in Annotate.lhs

Skipped because you later amended this with one that fixes the
outstanding conflict

> Sun Mar 21 00:10:08 EET 2010  Alexey Levan <exlevan@gmail.com>
>   * Separate arguments/repository code in Add.lhs
> 
> Sun Mar 21 00:10:14 EET 2010  Alexey Levan <exlevan@gmail.com>
>   * Resolve issue1397: separate arguments/repository code in Changes.lhs

Reviewed

> Thu Mar 25 22:25:56 EET 2010  Alexey Levan <exlevan@gmail.com>                                                                                                      
>   * Separate arguments/repository code in Annotate.lhs                                                                                                              
>
> Fri Mar 26 16:26:06 EET 2010  Alexey Levan <exlevan@gmail.com>                                                                                                      
>   * darcs dist doesn't take any arguments     

From your other email.  Reviewed
                                                                                                                                                                     
> Thu Mar 25 22:26:01 EET 2010  Alexey Levan <exlevan@gmail.com>                                                                                                      
>   * Separate arguments/repository code in Diff.lhs                                                                                                                  

I'll give most of this one a miss as I have to get to bed...
 
Add Maybe variant of fixSubPaths
--------------------------------
> -fixSubPaths :: [DarcsFlag] -> [FilePath] -> IO [SubPath]
> -fixSubPaths flags fs =
> -    withCurrentDirectory o $
> -    do fixedfs <- mapM fixit $ filter (not.null) fs
> -       let (good, bad) = partitionEither fixedfs
> -       unless (null bad) $
> -              putStrLn $ "Ignoring non-repository paths: " ++ unwords bad
> -       return $ nub good

> +maybeFixSubPaths :: [DarcsFlag] -> [FilePath] -> IO [Maybe SubPath]
> +maybeFixSubPaths flags fs = withCurrentDirectory o $ do
> +  fixedFs <- mapM fixit fs
> +  let bads = snd . unzip . filter (isNothing . fst) $ zip fixedFs fs
> +  unless (null bads) . putStrLn $ "Ignoring non-repository paths: " ++
> +    intercalate ", " bads
> +  return fixedFs

I wonder if the bads would be clearer as a list comprehension

   bads = [ p | (Nothing,p) <- zip fixedFs fs ]

> +fixSubPaths :: [DarcsFlag] -> [FilePath] -> IO [SubPath]
> +fixSubPaths flags fs = nub . catMaybes <$> (maybeFixSubPaths flags $
> +  filter (not . null) fs)

I was curious about why you didn't just do this stuff (the nub and
the filter) in maybeFixSubPaths, but this becomes clearer in future
patches.  Basically, it's to avoid throwing out information about
what the user passed in.

> hunk ./src/Darcs/Arguments.lhs 449
> -    (r,o) = case extractFixPath flags of
> -            Just xxx -> xxx
> -            Nothing -> bug "Can't fix path in fixSubPaths"
> -    fixit p = do ap <- ioAbsolute p
> -                 case makeSubPathOf r ap of
> -                   Just sp -> return $ Right sp
> -                   Nothing -> return $ maybe (Left p) Right $ simpleSubPath p
> +  (r, o) = case extractFixPath flags of
> +    Just x -> x
> +    Nothing -> bug "Can't fix path in maybeFixSubPaths"
> +  fixit p = do
> +    ap <- ioAbsolute p
> +    return $ makeSubPathOf r ap `mplus` simpleSubPath p

This appears to be just refactoring which I tend to prefer as a separate
patch.  Thanks for the MonadPlus reminder...

Separate argument handling from repository work in Move.lhs
-----------------------------------------------------------
> +  | length args < 2 = fail $ "The `darcs move' command requires at least" ++
> +      "two arguments."
> +  | length args == 2 = do

I was going to suggest you keep the pattern matching, but I guess we
don't *really* need it here because we were really just doing a length
verification and not really grabbing the values out of the match.

> +      xs <- maybeFixSubPaths opts args
> +      case xs of
> +        [Just from, Just to]
> +          | from == to -> fail "Cannot rename a file or directory onto itself!"
> +          | otherwise -> moveFile opts from to
> +        _ -> fail "Both source and destination must be valid."
> +  | otherwise = let (froms, to) = (init args, last args) in do
> +      x <- head <$> maybeFixSubPaths opts [to]

Hmm, it seems like there's a lot of packing-unpacking things here.
Perhaps fixIt above should be refactored to maybeFixSubPath?  (Then what do
we do about warning messages?)

Also, thinking about this, do you catch http://bugs.darcs.net/issue1800
?  (darcs HEAD does not).

> +      case x of
> +        Nothing -> fail "Invalid destination directory."
> +        Just to' -> do
> +          xs <- nub . sort <$> fixSubPaths opts froms
> +          case xs of
> +            [] -> fail "Nothing to move."
> +            froms' -> moveFilesToDir opts froms' to'

You shouldn't need to nub here if fixSubPaths does it (but should
fixSubPaths do it?)

Thanks for catching the case where path checking fails for all the from
paths.

It may be nice if moveFilesToDir would enforce a length >= 1 list
(which is doable at the expense of code-readability perhaps)

The rest seems fine.

Add test for absolute paths
---------------------------
> +if darcs move /non_existent_path/a /non_existent_path/b 2>&1 | grep 'bug'; then
> +  echo 'Not OK 1: darcs move causes a bug'
> +  exit 1
> +else
> +  echo 'OK 1'
> +fi

You don't need to print this stuff explicitly; you could just rely on
the shell harness and the fact that we're using bash -e

For example, darcs move /non_existent_path/a /non_existent_path/b 2>&1 | not grep 'bug'

> +if darcs move /non_existent_path/a /non_existent_path/b /non_existent_path/c 2>&1 | grep 'Prelude.init: empty list'; then

That seems like an odd thing to test for.  Shouldn't the code just avoid
getting into a situation where this sort of thing can happen?

In general, I do think having one script that gathers all this path
handling stuff into one place is a good idea.  I might probably add
tests for the original changes bug, for example

Separate arguments/repository code in AmendRecord.lhs
-----------------------------------------------------
>  amendrecordCmd :: [DarcsFlag] -> [String] -> IO ()
> -amendrecordCmd opts args =
> +amendrecordCmd opts args = if null args
> +  then doAmendRecord opts Nothing
> +  else do
> +    files <- fixSubPaths opts args
> +    if null files
> +      then fail "No valid arguments were given, nothing to do."
> +      else doAmendRecord opts $ Just files

This is just switching to use Alexey's convention that Nothing means no
command line arguments (which is different from some command line
arguments of which some could be bad).

> +doAmendRecord :: [DarcsFlag] -> Maybe [SubPath] -> IO ()
> +doAmendRecord opts fs = do

One mildly Hungarian coding convention might be to use mfs here

>      withRepoLock (testByDefault opts) $- \(repository :: Repository p C(r u r)) -> do
> hunk ./src/Darcs/Commands/AmendRecord.lhs 137
> -    files  <- sort `fmap` fixSubPaths opts args
> -    when (areFileArgs files) $
> -         putStrLn $ "Amending changes in "++unwords (map show files)++":\n"

I'd be quite interested to see areFileArgs go away.

Meanwhile, what happens when you do darcs amend-record .?
What should happen?

>      with_selected_patch_from_repo "amend" repository opts $ \ (_ :> oldp) -> do
> hunk ./src/Darcs/Commands/AmendRecord.lhs 138
> -        ch <- unrecordedChanges opts repository files
> -
> -        let handleChanges :: FL Prim C(r y) -> IO ()
> -            handleChanges NilFL | not (hasEditMetadata opts) = putStrLn "No changes!"
> -            handleChanges ch =
> +        ch <- case fs of
> +          Nothing -> unrecordedChanges opts repository []
> +          Just files -> do
> +            putStrLn $ "Amending changes in "++unwords (map show files)++":\n"

Is it a mistake to move this putStrLn inside the CPS job?

> +            unrecordedChanges opts repository files
> +        case ch of
> +          NilFL | not (hasEditMetadata opts) -> putStrLn "No changes!"
> +          _ ->
>                 with_selected_changes_to_files' "add" (filter (==All) opts) (Just primSplitter)
> hunk ./src/Darcs/Commands/AmendRecord.lhs 147
> -                (map toFilePath files) ch $ addChangesToPatch opts repository oldp
> -        handleChanges ch
> +                (fromMaybe [] $ map toFilePath <$> fs) ch $ addChangesToPatch opts repository oldp

Sounds like this last bit could be more simply expressed as (map toFilePath $ fromMaybe [] fs)

I may be mistaken, but I think this patch would also have been clearer
if it a bit was more conservative in its approach.

Separate arguments/repository code in Add.lhs
---------------------------------------------
>  addCmd :: [DarcsFlag] -> [String] -> IO ()
> -addCmd opts args = withRepoLock opts $- \repository ->
> +addCmd opts args
> +  | null args = putStrLn $ "Nothing specified, nothing added." ++
> +      "Maybe you wanted to say `darcs add --recursive .'?"
> +  | otherwise = do
> +      fs <- fixSubPaths opts args
> +      case fs of
> +        [] -> putStrLn "No valid arguments were given, nothing to do."
> +        _ -> addFiles opts fs
> +
> +addFiles :: [DarcsFlag] -> [SubPath] -> IO ()
> +addFiles opts origfiles = withRepoLock opts $- \repository ->
>   do cur <- slurp_pending repository
> hunk ./src/Darcs/Commands/Add.lhs 113
> -    origfiles <- fixSubPaths opts args
> -    when (null origfiles) $
> -       putStrLn "Nothing specified, nothing added." >>
> -       putStrLn "Maybe you wanted to say `darcs add --recursive .'?"

Seems fine to me.

> -    when (nullFL ps && not (null args)) $
> +    when (nullFL ps && not (null origfiles)) $

Any particular reason why 'orig'files?  Just curious.

Resolve issue1397: separate arguments/repository code in Changes.lhs
--------------------------------------------------------------------
>  changesCmd :: [DarcsFlag] -> [String] -> IO ()
> -changesCmd [Context _] [] = return ()

You do not appear to be catching this case anymore?
What exactly was this supposed to catch?

> -changesCmd opts args | Context rootDirectory `elem` opts =
> -  let repodir = fromMaybe "." (getRepourl opts) in
> -  withRepositoryDirectory opts repodir $- \repository -> do
> -  when (args /= []) $ fail "changes --context cannot accept other arguments"
> -  changesContext repository opts

> +changesCmd opts args
> +  | Context rootDirectory `elem` opts = if not . null $ args
> +      then fail "changes --context cannot accept other arguments"
> +      else changesContext opts

I personally like to avoid negating things, eg

  | Context rootDirectory `elem` opts = if null args
      then changesContext opts
      else fail "changes --context cannot accept other arguments"

But maybe your style of catching the failure cases first makes
more sense.

> +  | null args = showChanges opts Nothing
> +  | otherwise = do
> +      fs <- fixSubPaths opts args
> +      case fs of
> +        [] -> putStrLn "No valid arguments were given, nothing to do."
> +        _ -> showChanges opts . Just . nub $ sort fs

Should that that string be going out on stderr?

> +showChanges :: [DarcsFlag] -> Maybe [SubPath] -> IO ()
> +showChanges opts files =

This is again following the convention from above...

> -  files <- sort `fmap` fixSubPaths opts args
> -  Sealed unrec <- if null files then return (Sealed identity)
> -                  else Sealed `fmap` unrecordedChanges opts repository files
> +  Sealed unrec <- case files of
> +    Nothing -> return $ Sealed identity
> +    Just files' -> Sealed `fmap` unrecordedChanges opts repository files'
>                    `catch` \_ -> return (Sealed identity) -- this is triggered when repository is remote

This looks like it's just taking advantage of the fact that we don't
have to check for explicit null anymore

> -    else do when (not (null files) && not (XMLOutput `elem` opts)) $
> -                 putStrLn $ "Changes to "++unwords filez++":\n"
> +    else do when (isJust files && not (XMLOutput `elem` opts)) $
> +                 putStrLn $ "Changes to "++unwords (fromJust filez)++":\n"

Hmm... using things like fromJust sometimes indicates that there may be
a better way still to write this.  I guess something is just bugging me
about this convention

Nothing     -- here I'm expecting some sort of failure, but it's actually
            -- just one of the success cases
Just []     -- here's the actual failure
Just (p:ps) -- another success condition

I guess what's not clear is whose responsibility it is to deal with the
failure condition.  So far, it seems like we do it in the wrapper layer
above and then expect the inner layers to just ignore it implicitly.
Is there a better way?

One way might be something like

   data Args a = ExplicitNoArgs | NoGoodArgs | GoodArgs a [a]

(the names are just tentative).  I don't know if that's any clearer
though.  In general I like the idea of making it impossible to pass
"bad" arguments to low-level functions by just enforcing things in
types.  I just haven't worked how to do it nicely yet :-)

> -getChangesInfo :: RepoPatch p => [DarcsFlag] -> [FilePath]
> +getChangesInfo :: RepoPatch p => [DarcsFlag] -> Maybe [FilePath]
>                 -> RL (RL (PatchInfoAnd p)) C(x y)
>                 -> ([(Sealed2 (PatchInfoAnd p), [FilePath])], [FilePath], Doc)

One note is that a goal of the RepoPath work has been to push the use of
SubPath as far down as possible (make as much use of typed paths over
filepaths).

>  getChangesInfo opts plain_fs ps =
> hunk ./src/Darcs/Commands/Changes.lhs 178
>        (Sealed p1s, Sealed p2s) ->
>            case get_common_and_uncommon (p2s,p1s) of
>              (_,us:\/:_) ->
> -                filterPatchesByNames (maxCount opts) fs $ filterRL pf us
> -  where fs = map (\x -> "./" ++ x) $ plain_fs
> -        sp1s = if firstMatch opts
> +              let ps' = filterRL pf us in
> +                case plain_fs of
> +                  Nothing -> foldr (\x xs -> (x, []) -:- xs) ([], [], empty) $
> +                    maybe id take (maxCount opts) ps'

Oh, so this is breaking up some of the max-count logic up in the
interests of consolidating the file path handling.  Hmm, wouldn't
it be easier to maintain if the max-count stuff was kept together?
I suppose it may make sense to apply your convention to
filterPatchesByNames below.

> +                  Just fs -> let fs' = map (\x -> "./" ++ x) fs in
> +                    filterPatchesByNames (maxCount opts) fs' ps'
> +  where sp1s = if firstMatch opts
>                 then matchFirstPatchset opts ps
>                 else Sealed $ NilRL:<:NilRL
>          sp2s = if secondMatch opts
> hunk ./src/Darcs/Commands/Changes.lhs 208
>                          -> [Sealed2 (PatchInfoAnd p)] -- ^ patchlist
>                          -> ([(Sealed2 (PatchInfoAnd p),[FilePath])], [FilePath], Doc)
>  filterPatchesByNames (Just 0) _ _ = ([], [], empty)
>  filterPatchesByNames _ _ [] = ([], [], empty)
> -filterPatchesByNames maxcount [] (hp:ps) =
> -    (hp, []) -:- filterPatchesByNames (subtract 1 `fmap` maxcount) [] ps
> +filterPatchesByNames _ [] _ = ([], [], empty)

The reason this does not break darcs changes --max-count with no arguments
is that

> -changesContext :: RepoPatch p => Repository p C(r u t) -> [DarcsFlag] -> IO ()
> -changesContext repository opts = do
> +changesContext :: [DarcsFlag] -> IO ()
> +changesContext opts = do
> +  let repodir = fromMaybe "." $ getRepourl opts
> +  withRepositoryDirectory opts repodir $- \repository -> do

What exactly motivates this change?  What was wrong with just passing
a repository down?

darcs dist doesn't take any arguments
-------------------------------------
> -distCmd opts args = withRepoReadLock opts $- \repository -> do
> +distCmd opts _ = withRepoReadLock opts $- \repository -> do

> -  path_list <- if null args
> -               then return [""]
> -               else map toFilePath `fmap` fixSubPaths opts args

Hmm, I notice that darcs dist indeed complains when you pass it args,
but I'm a bit confused by where this complaining is currently taking
place.  Have we lost the ability to complain if the user passes in
some args? Should we check for it explicitly?

> -        else createPartialsPristineDirectoryTree repository path_list (toFilePath ddir)
> +        else createPartialsPristineDirectoryTree repository [""] (toFilePath ddir)

Separate arguments/repository code in Annotate.lhs
--------------------------------------------------
> -annotateCmd opts [] = withRepository opts $- \repository -> do
> -  when (not $ haveNonrangeMatch opts) $
> -      fail $ "Annotate requires either a patch pattern or a " ++
> -               "file or directory argument."
> +annotateCmd opts args = case args of
> +  [] -> if haveNonrangeMatch opts
> +    then annotatePattern opts
> +    else fail $ "Annotate requires either a patch pattern or a " ++
> +      "file or directory argument."
> +  [_] -> do
> +    f <- head <$> maybeFixSubPaths opts args
> +    case f of
> +      Nothing -> fail "invalid argument"
> +      Just f' -> annotatePath opts f'
> +  _ -> fail "annotate accepts at most one argument"

Your change certainly seems to make it a good deal clearer, separating the path
validation from the core logic.

> -annotateCmd opts args@[_] = withRepository opts $- \repository -> do
> +annotatePath :: [DarcsFlag] -> SubPath -> IO ()
> +annotatePath opts file = withRepository opts $- \repository -> do
>    r <- read_repo repository
> hunk ./src/Darcs/Commands/Annotate.lhs 161
> -  (rel_file_or_directory:_) <- fixSubPaths opts args
> -  let file_or_directory = rel_file_or_directory

One of the benefits of being able to move all the validation up and
just use types downwards.

> -  if toFilePath file_or_directory == "." || toFilePath file_or_directory == ""
> +  let file' = toFilePath file
> +  if file' == "." || file' == ""

Seems like a nice refactor here (saves a lot of toFilePath file), again
with my usual grumbling about refactors sometimes making patches hard to
read when they're mixed in :-)

Separate arguments/repository code in Diff.lhs
----------------------------------------------
>  diffCmd :: [DarcsFlag] -> [String] -> IO ()
> -diffCmd opts args = withRepository opts $- \repository -> do
> -  when (not (null [i | LastN i <- opts])
> -       && not (null [p | AfterPatch p <- opts])
> -       ) $
> -    fail ("using --patch and --last at the same time with the 'diff' command"
> -         ++ " doesn't make sense. Use --from-patch to create a diff from this"
> -         ++ " patch to the present, or use just '--patch' to view this specific"
> -         ++ " patch.")
> +diffCmd opts args
> +  | not (null [i | LastN i <- opts]) &&
> +      not (null [p | AfterPatch p <- opts]) =
> +        fail $ "using --patch and --last at the same time with the 'diff'" ++
> +          " command doesn't make sense. Use --from-patch to create a diff" ++
> +          " from this patch to the present, or use just '--patch' to view" ++
> +          " this specific patch."
> +  | null args = doDiff opts Nothing
> +  | otherwise = doDiff opts . Just =<< fixSubPaths opts args

Same application of the convention above.

> +doDiff :: [DarcsFlag] -> Maybe [SubPath] ->  IO ()
> +doDiff opts sps = withRepository opts $- \repository -> do
> +  let pathList = map sp2fn `fmap` sps
>    formerdir <- getCurrentDirectory
> hunk ./src/Darcs/Commands/Diff.lhs 198
> -  subpaths <- if null args then return []
> -                           else fixSubPaths opts args
> -  let path_list = map sp2fn subpaths
> -  thename <- return $ takeFileName formerdir
> -  withTempDir ("old-"++thename) $ \odir -> do
> -    setCurrentDirectory formerdir
> -    withTempDir ("new-"++thename) $ \ndir -> do
> +  withTempDirs (takeFileName formerdir) $ \odir ndir -> do
>      if firstMatch opts
> hunk ./src/Darcs/Commands/Diff.lhs 200
> -       then withCurrentDirectory odir $
> -            getPartialFirstMatch repository opts path_list
> -       else if null path_list
> -            then createPristineDirectoryTree repository (toFilePath odir)
> -            else createPartialsPristineDirectoryTree repository path_list (toFilePath odir)
> +      then withCurrentDirectory odir . getPartialFirstMatch repository opts $
> +        fromMaybe [] pathList
> +      else case pathList of
> +        Nothing -> createPristineDirectoryTree repository $ toFilePath odir
> +        Just pl -> createPartialsPristineDirectoryTree repository pl $ toFilePath odir
>      if secondMatch opts
> hunk ./src/Darcs/Commands/Diff.lhs 206
> -       then withCurrentDirectory ndir $
> -            getPartialSecondMatch repository opts path_list
> -       else withCurrentDirectory formerdir $ do
> -               restrict <- restrictSubpaths repository subpaths
> -               restrict `fmap` readUnrecorded repository >>= (flip writePlainTree (toFilePath ndir))
> +      then withCurrentDirectory ndir . getPartialSecondMatch repository opts $
> +        fromMaybe [] pathList
> +      else withCurrentDirectory formerdir $ do
> +        restrict <- restrictSubpaths repository (fromMaybe [] sps)
> +        unrec <- readUnrecorded repository
> +        writePlainTree (restrict unrec) $ toFilePath ndir
>      thediff <- withCurrentDirectory (toFilePath odir ++ "/..") $
> hunk ./src/Darcs/Commands/Diff.lhs 213
> -                   case path_list of
> -                   [] -> rundiff (takeFileName $ toFilePath odir) (takeFileName $ toFilePath ndir)
> -                   fs -> vcat `fmap`
> +                   case pathList of
> +                   Nothing -> rundiff (takeFileName $ toFilePath odir) (takeFileName $ toFilePath ndir)
> +                   Just fs -> vcat `fmap`
>                           mapM (\f -> rundiff
>                                 (takeFileName (toFilePath odir) ++ "/" ++ toFilePath f)
>                                 (takeFileName (toFilePath ndir) ++ "/" ++ toFilePath f)) fs
> hunk ./src/Darcs/Commands/Diff.lhs 237
>                      return ()
>                   return output
>  
> +          withTempDirs :: String -> (AbsolutePath -> AbsolutePath -> IO a) -> IO a
> +          withTempDirs x f = withTempDir ("old-" ++ x) $ \odir ->
> +            withTempDir ("new" ++ x) $ \ndir -> f odir ndir
> +
>  getDiffInfo :: RepoPatch p => [DarcsFlag] -> PatchSet p C(start x) -> [PatchInfo]
>  getDiffInfo opts ps =
>      let infos = mapRL info . concatRL


-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9

Eric Kow <kowey@darcs.net> writes:

> I like the general idea behind the patch, but I'm not going to apply it
> yet, primarily because it conflicts with patch156.  Do you think you
> could have a second look at that?

Now that patch156 is out of the way, could this be amended and (the
review finished and the patch) pushed?

Thanks,
   Petr.

Here are the 9 final patches by Alex with 7 patches by me resolving conflicts
with current HEAD.

16 patches for repository http://darcs.net/screened:

Tue Mar 16 01:38:17 CET 2010  Alexey Levan <exlevan@gmail.com>
  * Add Maybe variant of fixSubPaths

Tue Mar 16 23:02:11 CET 2010  Alexey Levan <exlevan@gmail.com>
  * Separate argument handling from repository work in Move.lhs

Sat Mar 20 22:28:53 CET 2010  Alexey Levan <exlevan@gmail.com>
  * Add test for absolute paths

Sat Mar 20 23:09:44 CET 2010  Alexey Levan <exlevan@gmail.com>
  * Separate arguments/repository code in AmendRecord.lhs

Sat Mar 20 23:10:08 CET 2010  Alexey Levan <exlevan@gmail.com>
  * Separate arguments/repository code in Add.lhs

Sat Mar 20 23:10:14 CET 2010  Alexey Levan <exlevan@gmail.com>
  * Resolve issue1397: separate arguments/repository code in Changes.lhs

Sun Oct 17 10:18:03 CEST 2010  Reinier Lamers <tux_rocker@reinier.de>
  * Resolve conflicts with Alexey's introduction of maybeFixSubPaths
  
  This resolves conflicts with:
  Tue Mar 16 02:38:17 EET 2010  Alexey Levan <exlevan@gmail.com>
    * Add Maybe variant of fixSubPaths

Sun Oct 17 10:24:17 CEST 2010  Reinier Lamers <tux_rocker@reinier.de>
  * Resolve conflicts with Alexey's refactoring of argument handling in amend-record
  
  This resolves conflicts with:
  Sun Mar 21 00:09:44 EET 2010  Alexey Levan <exlevan@gmail.com>
    * Separate arguments/repository code in AmendRecord.lhs

Sun Oct 17 10:27:17 CEST 2010  Reinier Lamers <tux_rocker@reinier.de>
  * Resolve conflicts with Alexey's refactoring of argument handling in add
  
  This resolves conflicts with:
  Sun Mar 21 00:10:08 EET 2010  Alexey Levan <exlevan@gmail.com>
    * Separate arguments/repository code in Add.lhs

Sun Oct 17 10:27:44 CEST 2010  Reinier Lamers <tux_rocker@reinier.de>
  * Resolve conflicts with Alexey's refactoring of argument handling in changes
  
  This resolves conflicts with:
  Sun Mar 21 00:10:14 EET 2010  Alexey Levan <exlevan@gmail.com>
    * Resolve issue1397: separate arguments/repository code in Changes.lhs

Thu Mar 25 21:25:56 CET 2010  Alexey Levan <exlevan@gmail.com>
  * Separate arguments/repository code in Annotate.lhs

Sun Oct 17 10:51:17 CEST 2010  Reinier Lamers <tux_rocker@reinier.de>
  * Resolve conflicts with Alexey's refactoring of argument handling in annotate
  
  This resolves conflicts with:
  Thu Mar 25 22:25:56 EET 2010  Alexey Levan <exlevan@gmail.com>
    * Separate arguments/repository code in Annotate.lhs

Sun Oct 17 10:52:38 CEST 2010  Reinier Lamers <tux_rocker@reinier.de>
  * resolve issue1965: Resolve conflicts with argument handling work in move
  
  This resolves conflicts with:
  Wed Mar 17 00:02:11 EET 2010  Alexey Levan <exlevan@gmail.com>
    * Separate argument handling from repository work in Move.lhs

Thu Mar 25 21:26:01 CET 2010  Alexey Levan <exlevan@gmail.com>
  * Separate arguments/repository code in Diff.lhs

Sun Oct 17 11:18:57 CEST 2010  Reinier Lamers <tux_rocker@reinier.de>
  * Resolve conflicts with Alexey's refactoring of argument handling in diff
  
  This resolves conflicts with:
  Thu Mar 25 22:26:01 EET 2010  Alexey Levan <exlevan@gmail.com>
    * Separate arguments/repository code in Diff.lhs

Fri Mar 26 15:26:06 CET 2010  Alexey Levan <exlevan@gmail.com>
  * darcs dist doesn't take any arguments

Attachments

• add-maybe-variant-of-fixsubpaths.dpatch
• unnamed

This is reworked version of patches, based on my previous work with conflicts
resolved (thanks Reinier!).  To make review simpler, I rewrote these patches
from scratch, instead of adding a new layer of amendments.

7 patches for repository http://darcs.net/screened:

Sat Nov  6 00:44:43 EET 2010  Alexey Levan <exlevan@gmail.com>
  * Add a Maybe variant of fixSubPaths

The same as before, plus haddock by Renier.


Sat Nov  6 02:09:14 EET 2010  Alexey Levan <exlevan@gmail.com>
  * A bit of type tuning in Darcs.Commands.AmendRecord

In this small refactoring I removed a type annotation where it's not needed,
and made a type of 'addChangesToPatch' to be less general.


Sat Nov  6 03:06:29 EET 2010  Alexey Levan <exlevan@gmail.com>
  * Don't treat empty filelists as special in Darcs.Repository.State

New versions of 'unrecordedChanges' and 'readUnrecorded', where empty
filelists have no special meaning.


Sat Nov  6 03:10:33 EET 2010  Alexey Levan <exlevan@gmail.com>
  * Refactor argument handling in Darcs.Commands.AmendRecord

Sat Nov  6 19:08:23 EET 2010  Alexey Levan <exlevan@gmail.com>
  * Separate no args/no valid args cases in Darcs.Commands.Diff

Sat Nov  6 20:27:25 EET 2010  Alexey Levan <exlevan@gmail.com>
  * Resolve issue1397: handle "no valid args" case in Darcs.Commands.Changes

Sat Nov  6 20:46:29 EET 2010  Alexey Levan <exlevan@gmail.com>
  * Resolve issue1965: fix argument handling in Darcs.Commands.Move

Attachments

• add-a-maybe-variant-of-fixsubpaths.dpatch
• unnamed

Hi Alexey,

your previous patches have already been applied in the screened branch
(http://darcs.net/screened).
As far as I understand the process, correcting patches that are in
screened is not longer done by amending them but by sending followup
patches.
So I think you should make patches against the screneed branch in its
current state.

As of now only the following patch from your bundles applies to
screened without conflicts:

Sat Nov  6 03:06:29 EET 2010  Alexey Levan <exlevan@gmail.com>
 * Don't treat empty filelists as special in Darcs.Repository.State

Guillaume

2010/11/6 Alexey Levan <bugs@darcs.net>:
>
> Alexey Levan <exlevan@gmail.com> added the comment:
>
> This is reworked version of patches, based on my previous work with conflicts
> resolved (thanks Reinier!).  To make review simpler, I rewrote these patches
> from scratch, instead of adding a new layer of amendments.
>
> 7 patches for repository http://darcs.net/screened:
>
> Sat Nov  6 00:44:43 EET 2010  Alexey Levan <exlevan@gmail.com>
>  * Add a Maybe variant of fixSubPaths
>
> The same as before, plus haddock by Renier.
>
>
> Sat Nov  6 02:09:14 EET 2010  Alexey Levan <exlevan@gmail.com>
>  * A bit of type tuning in Darcs.Commands.AmendRecord
>
> In this small refactoring I removed a type annotation where it's not needed,
> and made a type of 'addChangesToPatch' to be less general.
>
>
> Sat Nov  6 03:06:29 EET 2010  Alexey Levan <exlevan@gmail.com>
>  * Don't treat empty filelists as special in Darcs.Repository.State
>
> New versions of 'unrecordedChanges' and 'readUnrecorded', where empty
> filelists have no special meaning.
>
>
> Sat Nov  6 03:10:33 EET 2010  Alexey Levan <exlevan@gmail.com>
>  * Refactor argument handling in Darcs.Commands.AmendRecord
>
> Sat Nov  6 19:08:23 EET 2010  Alexey Levan <exlevan@gmail.com>
>  * Separate no args/no valid args cases in Darcs.Commands.Diff
>
> Sat Nov  6 20:27:25 EET 2010  Alexey Levan <exlevan@gmail.com>
>  * Resolve issue1397: handle "no valid args" case in Darcs.Commands.Changes
>
> Sat Nov  6 20:46:29 EET 2010  Alexey Levan <exlevan@gmail.com>
>  * Resolve issue1965: fix argument handling in Darcs.Commands.Move
>
> __________________________________
> Darcs bug tracker <bugs@darcs.net>
> <http://bugs.darcs.net/patch191>
> __________________________________
> _______________________________________________
> darcs-users mailing list
> darcs-users@darcs.net
> http://lists.osuosl.org/mailman/listinfo/darcs-users
>
>

Hi Reinier and Alexey,

Here's another review pass.  Thanks for your effort in cleaning up the
patch!  Unfortunately you were bitten by recent experimental changes to
our process that Guillaume pointed out.

I'm reviewing Reinier's version of the bundle (due to the screened
branch).  To make life easier, I'm reviewing the whole bundle in one
go as if it were a single coalesced patch.

Note that I'm a bit dissatisfied with the test case (see below).  Part
of me is tempted not to apply the patch in a Zooko-like spirit of being
very demanding about testing in the interests of faster progress (*),
but I feel a bit like I've played a big part in this patch stalling
unnecessarily and making work for everybody, so I'm applying it a little
bit out of guilt.

Well, guilt and the fact that it really is an improvement :-)
Applied, thanks!

GENERAL COMMENTS
----------------------------------------------------------------------
There are two more commands that take subpaths, I think: revert and
darcs show contents.  I think we may need a follow-up patch to cover
these and other subpath commands I may not have thought of.

I've prefixed the more important comments with NOTE

Please send your recent addendum as a new patch to screened
branch.

./src/Darcs/Arguments.lhs
----------------------------------------------------------------------
> --- | @fixSubPaths files@ returns the @SubPath@s for the paths in @files@ that
> --- are inside the repository, preserving their order. Paths in @files@ that are
> --- outside the repository directory are not in the result.
> +-- | @maybeFixSubPaths files@ tries to turn the file paths in its argument into
> +-- @SubPath@s.
>  --
>  -- When converting a relative path to an absolute one, this function first tries
>  -- to interpret the relative path with respect to the current working directory.
> hunk ./src/Darcs/Arguments.lhs 496
>  -- If that fails, it tries to interpret it with respect to the repository
> --- directory. Only when that fails does it omit the path from the result.
> +-- directory. Only when that fails does it put a @Nothing@ in the result at the
> +-- position of the path that cannot be converted.
>  --
>  -- It is intended for validating file arguments to darcs commands.

OK so the core of fixSubPaths has been tweaked and broken out as a new
function maybeFixSubPaths which preserves the number/order of the input
paths and puts Nothing in place of all the bad paths.

> hunk ./src/Darcs/Arguments.lhs 500
> -fixSubPaths :: [DarcsFlag] -> [FilePath] -> IO [SubPath]
> -fixSubPaths flags fs =
> -    withCurrentDirectory o $
> -    do fixedfs <- mapM fixit $ filter (not.null) fs
> -       let (good, bad) = partitionEither fixedfs
> -       unless (null bad) $
> -              putStrLn $ "Ignoring non-repository paths: " ++ unwords bad
> -       return $ nub good
> +maybeFixSubPaths :: [DarcsFlag] -> [FilePath] -> IO [Maybe SubPath]
> +maybeFixSubPaths flags fs = withCurrentDirectory o $ do
> +  fixedFs <- mapM fixit fs
> +  let bads = snd . unzip . filter (isNothing . fst) $ zip fixedFs fs
> +  unless (null bads) . putStrLn $ "Ignoring non-repository paths: " ++
> +    intercalate ", " bads
> +  return fixedFs

This maybeFixSubPaths function is slightly more than just a map.
It also does report any non-repository paths, if there are any:

  Ignoring non-repository paths: ../baz, /quux/yoda, /blah

> +-- | @fixSubPaths files@ returns the @SubPath@s for the paths in @files@ that
> +-- are inside the repository, preserving their order. Paths in @files@ that are
> +-- outside the repository directory are not in the result.
> +--
> +-- When converting a relative path to an absolute one, this function first tries
> +-- to interpret the relative path with respect to the current working directory.
> +-- If that fails, it tries to interpret it with respect to the repository
> +-- directory. Only when that fails does it omit the path from the result.
> +--
> +-- It is intended for validating file arguments to darcs commands.
> +fixSubPaths :: [DarcsFlag] -> [FilePath] -> IO [SubPath]
> +fixSubPaths flags fs = nub . catMaybes <$> (maybeFixSubPaths flags $
> +  filter (not . null) fs)

The old fixSubPaths is still around. Do we really need it?

./src/Darcs/Commands/Add.lhs
----------------------------------------------------------------------
>  addCmd :: [DarcsFlag] -> [String] -> IO ()
> -addCmd opts args = withRepoLock opts $- \repository ->
> +addCmd opts args
> +  | null args = putStrLn $ "Nothing specified, nothing added." ++
> +      "Maybe you wanted to say `darcs add --recursive .'?"
> +  | otherwise = do
> +      fs <- fixSubPaths opts args
> +      case fs of
> +        [] -> fail "No valid arguments were given"
> +        _ -> addFiles opts fs

This is part of a general pattern of separating the inner command logic
from an outer traffic-control layer which basically figures out what
variant of the command to run depending on the kind of arguments you
gave it.

Alexey added an extra distinction between
 darcs add
and
 darcs add /only/non-repository /paths/here

which previously were indistinguishable from each other because the
non-repository paths were thrown out.

NOTE: I notice we're using fail to complain here. Is that really
the right thing to do UI-wise?  You're just continuing current
practice, which is fine, but maybe what we need is a fail-like
function for dying on user error?

> hunk ./src/Darcs/Commands/Add.lhs 117
> -    when (null args) $
> -       putStrLn "Nothing specified, nothing added." >>
> -       putStrLn "Maybe you wanted to say `darcs add --recursive .'?"
> -    origfiles <- fixSubPaths opts args
> hunk ./src/Darcs/Commands/Add.lhs 131
> -    when (nullFL ps && not (null args)) $
> +    when (nullFL ps && not (null origfiles)) $

./src/Darcs/Commands/AmendRecord.lhs
----------------------------------------------------------------------
>  amendrecordCmd :: [DarcsFlag] -> [String] -> IO ()
> -amendrecordCmd opts args =
> +amendrecordCmd opts args = if null args
> +  then doAmendRecord opts []
> +  else do
> +    files <- fixSubPaths opts args
> +    if null files
> +      then fail "No valid arguments were given, nothing to do."
> +      else doAmendRecord opts files

Same pattern and same distinction.

NOTE: Is it worth trying to unify our user-error message?

> +doAmendRecord :: [DarcsFlag] -> [SubPath] -> IO ()
> +doAmendRecord opts files =
>      withRepoLock opts $- \(repository :: Repository p C(r u r)) -> do
> hunk ./src/Darcs/Commands/AmendRecord.lhs 139
> -    files  <- sort `fmap` fixSubPaths opts args
> -    when (areFileArgs files) $
> -         putStrLn $ "Amending changes in "++unwords (map show files)++":\n"
>      withSelectedPatchFromRepo "amend" repository opts $ \ (_ :> oldp) -> do
> hunk ./src/Darcs/Commands/AmendRecord.lhs 140
> +        when ((not.null) files) (putStrLn $ "Amending changes in "++unwords (map show files)++":\n")

./src/Darcs/Commands/Annotate.lhs
----------------------------------------------------------------------
>  annotateCmd :: [DarcsFlag] -> [String] -> IO ()
> -annotateCmd opts [] = withRepository opts $- \repository -> do
> -  when (not $ haveNonrangeMatch opts) $
> -      fail $ "Annotate requires either a patch pattern or a " ++
> -               "file or directory argument."
[and much later]
> -annotateCmd opts [""] = annotateCmd opts []

> +annotateCmd opts args = case args of
> +  [] -> if haveNonrangeMatch opts
> +    then annotatePattern opts
> +    else fail $ "Annotate requires either a patch pattern or a " ++
> +      "file or directory argument."
> +  [""] -> annotateCmd opts []
> +  [_] -> do
> +    f <- head <$> maybeFixSubPaths opts args
> +    case f of
> +      Nothing -> fail "invalid argument"
> +      Just f' -> annotatePath opts f'
> +  _ -> fail "annotate accepts at most one argument"

It does seem indeed easier to read with the arg processing separated
from the actual business end code.

I'm not terribly excited about the xs@[_] -> blah (head xs) above
but I guess there's not too much we can do about it unless we wanted
to go introduce a
  maybeFixSubPath :: [DarcsFlag] -> FilePath -> IO (Maybe SubPath)
which brings up questions about where the invalid-path UI code should
go.

Maybe this is better?

     f <- maybeFixSubPaths opts args
     case f of
       [Nothing] -> fail "invalid argument"
       [Just f'] -> annotatePath opts f'
       _         -> error "blah" -- using Darcs.Bug so we can find it

> +annotatePattern :: [DarcsFlag] -> IO ()
> +annotatePattern opts =
> +  withRepository opts $- \repository -> do
>    Sealed2 p <- matchPatch opts `fmap` readRepo repository

> -annotateCmd opts [file] = withRepository opts $- \repository -> do
> +annotatePath :: [DarcsFlag] -> SubPath -> IO ()
> +annotatePath opts file = withRepository opts $- \repository -> do
>    r <- readRepo repository
> hunk ./src/Darcs/Commands/Annotate.lhs 163
> -  fixed_args <- fixSubPaths opts [file]
> -  (rel_file_or_directory:_) <- case fixed_args of
> -                                 [] -> fail ("The supplied path " ++ file ++ " is not usable")
> -                                 fs -> return fs
> -  let file_or_directory = rel_file_or_directory

OK

>                       Just cp -> lookupCreationPop cp
> -  if toFilePath file_or_directory == "." || toFilePath file_or_directory == ""
> +  let file' = toFilePath file
> +  if null file'

NOTE: Is this really OK? I'm assuming that we do this because we know
how subpaths get converted to filepaths, and that the one for the
current directory is ""

Chatty aside: For some reason I like comparing empty string against ""
instead of checking if its null.  I guess I just like to treat strings
as being conceptually self-contained things, not necessary lists of
characters.  I guess 'null' could just mean 'is the empty string' so
that's just me being silly.

> hunk ./src/Darcs/Commands/Annotate.lhs 178
> -    else case lookup_thing (toFilePath file_or_directory) pop of
> -      Nothing -> fail $ "There is no file or directory named '"++
> -                 toFilePath file_or_directory++"'"
> +    else case lookup_thing file' pop of
> +      Nothing -> fail $ "There is no file or directory named '"++file'++"'"
> hunk ./src/Darcs/Commands/Annotate.lhs 182
> -              errorDoc $ text ("The directory '" ++ toFilePath rel_file_or_directory ++
> +              errorDoc $ text ("The directory '" ++ file' ++
> hunk ./src/Darcs/Commands/Annotate.lhs 188
> -              errorDoc $ text ("The file '" ++ toFilePath rel_file_or_directory ++
> +              errorDoc $ text ("The file '" ++ file' ++

Basic code-duplication refactor.

> -          | otherwise -> annotateFile repository opts pinfo file_or_directory pt
> -
> -annotateCmd _ _ = fail "annotate accepts at most one argument"
> +          | otherwise -> annotateFile repository opts pinfo file pt

Tidied away in the 2-layer pattern by Alexey.

./src/Darcs/Commands/Changes.lhs
----------------------------------------------------------------------
>  changesCmd :: [DarcsFlag] -> [String] -> IO ()
> -changesCmd opts args | GenContext `elem` opts =
> -  let repodir = fromMaybe "." (getRepourl opts) in
> -  withRepositoryDirectory opts repodir $- \repository -> do
> -  when (args /= []) $ fail "changes --context cannot accept other arguments"
> -  changesContext repository opts
> -changesCmd opts args =
> +changesCmd opts args
> +  | GenContext `elem` opts = if not . null $ args
> +      then fail "changes --context cannot accept other arguments"
> +      else changesContext opts
> +  | null args = showChanges opts Nothing
> +  | otherwise = do
> +      fs <- fixSubPaths opts args
> +      case fs of
> +        [] -> putStrLn "No valid arguments were given, nothing to do."
> +        _ -> showChanges opts . Just . nub $ sort fs

Yeah, this is an improvement.

NOTE: The nub is actually not because fixSubPaths does it. That said,
I wonder if fixSubPaths doing nub is really wise? I wonder what was
the initial motivation.

NOTE: Well, now we're not being very consistent here about this user
error complaint. Do we fail? Do we putStrLn? How do we exit?

> +    else do when (isJust files && not (XMLOutput `elem` opts)) $
> +                 putStrLn $ "Changes to "++unwords (fromJust filez)++":\n"

As above, I'm not thrilled about this sort of
pack-and-immediately-unsafely-unpack thing, particularly since the thing
we're unpacking filez is not the same as files (we just know it has the
same shape).

Is this an improvement? Or is it just longer?

     else do unless (XMLOutput `elem` opts) $
             case filez of
              Just fs -> putStrLn "Changes to " ++ unwords fs ++ ":\n"
              Nothing -> return ()

> -getChangesInfo :: RepoPatch p => [DarcsFlag] -> [FilePath]
> +getChangesInfo :: RepoPatch p => [DarcsFlag] -> Maybe [FilePath]
>                 -> PatchSet p C(x y)
>                 -> ([(Sealed2 (PatchInfoAnd p), [FilePath])], [FilePath], Doc)
>  getChangesInfo opts plain_fs ps =
> hunk ./src/Darcs/Commands/Changes.lhs 178
>      case (sp1s, sp2s) of
>        (Sealed p1s, Sealed p2s) ->
>            case findCommonWithThem p2s p1s of
> -            _ :>> us -> filterPatchesByNames (maxCount opts) fs $ filterRL pf $ reverseFL us
> -  where fs = map (\x -> "./" ++ x) $ plain_fs
> -        sp1s = if firstMatch opts
> +            _ :>> us ->
> +              let ps' = filterRL pf (reverseFL us) in
> +                case plain_fs of
> +                  Nothing -> foldr (\x xs -> (x, []) -:- xs) ([], [], empty) $
> +                    maybe id take (maxCount opts) ps'
> +                  Just fs -> let fs' = map (\x -> "./" ++ x) fs in
> +                    filterPatchesByNames (maxCount opts) fs' ps'
> +  where sp1s = if firstMatch opts

Looks like just unpacking/rearranging the code above (see
filterPatchesByNames). Seems very slightly gratuitous to me.

> hunk ./src/Darcs/Commands/Changes.lhs 209
>  filterPatchesByNames (Just 0) _ _ = ([], [], empty)
> +filterPatchesByNames _ [] _ = ([], [], empty)
>  filterPatchesByNames _ _ [] = ([], [], empty)
> hunk ./src/Darcs/Commands/Changes.lhs 211
> -filterPatchesByNames maxcount [] (hp:ps) =
> -    (hp, []) -:- filterPatchesByNames (subtract 1 `fmap` maxcount) [] ps
>  filterPatchesByNames maxcount fs ((Sealed2 hp):ps)
>      | Just p <- hopefullyM hp =
>      case lookTouch fs (invert p) of

This changes the meaning of filterPatchesByNames to remove the special
handling of "no files", which does seem a bit cleaner.  The cost is
duplicating the handling of maxcount.

Maybe it would be better to encode the distinction into
filterPatchesByNames by having it take Maybe [FilePath] instead?

> -changesContext :: RepoPatch p => Repository p C(r u t) -> [DarcsFlag] -> IO ()
> -changesContext repository opts = do
> +changesContext :: [DarcsFlag] -> IO ()
> +changesContext opts = do
> +  let repodir = fromMaybe "." $ getRepourl opts
> +  withRepositoryDirectory opts repodir $- \repository -> do
>    FlippedSeal ps' <- contextPatches `fmap` readRepo repository
>    let ps = mapRL (\p -> (seal2 p, [])) ps'
>    unless fancy $ putStrLn "\nContext:\n"

Re-arranged logic; the new code used to be part of changesCmd.

I dimly recall that we recently did a refactor of this stuff repodir
stuff though, but I don't remember where/when.

./src/Darcs/Commands/Diff.lhs
----------------------------------------------------------------------
>  diffCmd :: [DarcsFlag] -> [String] -> IO ()
> -diffCmd opts args = withRepository opts $- \repository -> do
> -  when (not (null [i | LastN i <- opts])
> -       && not (null [p | AfterPatch p <- opts])
> -       ) $
> -    fail ("using --patch and --last at the same time with the 'diff' command"
> -         ++ " doesn't make sense. Use --from-patch to create a diff from this"
> -         ++ " patch to the present, or use just '--patch' to view this specific"
> -         ++ " patch.")
> +diffCmd opts args
> +  | not (null [i | LastN i <- opts]) &&
> +      not (null [p | AfterPatch p <- opts]) =
> +        fail $ "using --patch and --last at the same time with the 'diff'" ++
> +          " command doesn't make sense. Use --from-patch to create a diff" ++
> +          " from this patch to the present, or use just '--patch' to view" ++
> +          " this specific patch."
> +  | null args = doDiff opts Nothing
> +  | otherwise = doDiff opts . Just =<< fixSubPaths opts args

Sounds like a standard Alexey refactor.

NOTE: don't we also want to introduce the no-args vs no-valid-args
distinction? We seem to be missing it here

> +doDiff :: [DarcsFlag] -> Maybe [SubPath] ->  IO ()
> +doDiff opts sps = withRepository opts $- \repository -> do
> +  let pathList = map sp2fn `fmap` sps
>    formerdir <- getCurrentDirectory
> hunk ./src/Darcs/Commands/Diff.lhs 198
> -  subpaths <- if null args then return []
> -                           else fixSubPaths opts args
> -  let path_list = map sp2fn subpaths
> -  thename <- return $ takeFileName formerdir
> -  withTempDir ("old-"++thename) $ \odir -> do
> -    setCurrentDirectory formerdir
> -    withTempDir ("new-"++thename) $ \ndir -> do

NOTE: (question) Was the setCurrentDirectory formerdir removed
from withTempDirs below because it was really superfluous?  Or
was it just an oversight?

> +  withTempDirs (takeFileName formerdir) $ \odir ndir -> do
>      if firstMatch opts
> hunk ./src/Darcs/Commands/Diff.lhs 200
> -       then withCurrentDirectory odir $
> -            getPartialFirstMatch repository opts path_list
> -       else if null path_list
> -            then createPristineDirectoryTree repository (toFilePath odir)
> -            else createPartialsPristineDirectoryTree repository path_list (toFilePath odir)
> +      then withCurrentDirectory odir . getPartialFirstMatch repository opts $
> +        fromMaybe [] pathList
> +      else case pathList of
> +        Nothing -> createPristineDirectoryTree repository $ toFilePath odir
> +        Just pl -> createPartialsPristineDirectoryTree repository pl $ toFilePath odir
>      if secondMatch opts
> hunk ./src/Darcs/Commands/Diff.lhs 206
> -       then withCurrentDirectory ndir $
> -            getPartialSecondMatch repository opts path_list
> -       else withCurrentDirectory formerdir $ do
> -               readUnrecorded repository subpaths >>= (flip writePlainTree (toFilePath ndir))
> +       then withCurrentDirectory ndir . getPartialSecondMatch repository opts $
> +               fromMaybe [] pathList
> +       else withCurrentDirectory formerdir $
> +               readUnrecorded repository (fromMaybe [] sps) >>= (flip writePlainTree (toFilePath ndir))

Changes here are actually fairly superficial: taking the change from [SubPath]
to Maybe [SubPath] into account, replacing parens with "$".

In the interests of minimising the amount of time reviewers spend
squinting at patches, I'd avoid making minor syntactic changes like the
parens/$ thing unless it was (a) done separately and (b) really does
improve clarity (and isn't just a matter of taste).  Maybe I shouldn't
be one to talk. I did something a bit similar in my HINT patches.

> hunk ./src/Darcs/Commands/Diff.lhs 211
> -                   case path_list of
> -                   [] -> rundiff (takeFileName $ toFilePath odir) (takeFileName $ toFilePath ndir)
> -                   fs -> vcat `fmap`
> +                   case pathList of
> +                   Nothing -> rundiff (takeFileName $ toFilePath odir) (takeFileName $ toFilePath ndir)
> +                   Just fs -> vcat `fmap`

Yep.

General thought. I'm not really sure it's that helpful to introduce the
distinction between [] (x:xs) and Nothing and (Just xs) in this inner
layer.  Is it really an improvement?  I get that the new one lets you
distinguish between implicit and explicit empty.  I guess it's just that
there isn't really an explicit empty here so we're effectively dealing
with a third case that we'd previously made impossible.

> +          withTempDirs :: String -> (AbsolutePath -> AbsolutePath -> IO a) -> IO a
> +          withTempDirs x f = withTempDir ("old-" ++ x) $ \odir ->
> +            withTempDir ("new" ++ x) $ \ndir -> f odir ndir
> +

Note the setCurrentDirectory going away.  I hope that doesn't change
something; it doesn't appear to.

./src/Darcs/Commands/Dist.lhs
----------------------------------------------------------------------
>  distCmd :: [DarcsFlag] -> [String] -> IO ()
> -distCmd opts args = withRepoReadLock opts $- \repository -> do
> +distCmd opts _ = withRepoReadLock opts $- \repository -> do
>    distname <- getDistName opts
>    verb <- return $ Verbose `elem` opts
>    predist <- getPrefval "predist"
> hunk ./src/Darcs/Commands/Dist.lhs 107
>    formerdir <- getCurrentDirectory
> -  path_list <- if null args
> -               then return [""]
> -               else map toFilePath `fmap` fixSubPaths opts args
>    resultfile <- return (formerdir</>distname++".tar.gz")
>    withTempDir "darcsdist" $ \tempdir -> do
>      setCurrentDirectory (formerdir)
> hunk ./src/Darcs/Commands/Dist.lhs 113
>      withTempDir (toFilePath tempdir </> takeFileName distname) $ \ddir -> do
>        if haveNonrangeMatch opts
>          then withCurrentDirectory ddir $ getNonrangeMatch repository opts
> -        else createPartialsPristineDirectoryTree repository path_list (toFilePath ddir)
> +        else createPartialsPristineDirectoryTree repository [""] (toFilePath ddir)

The new code is closer to what dist advertises itself as doing,
but it looks like the old code has more functionality (tarball with just
some of files in the repo?).  Anyway the old code was blocked off by
commandExtraArgs = 0 in the definition of the dist command, so the new
code is better.  I guess if it turns out that dist taking args is
actually a useful feature, somebody could always request it.

./src/Darcs/Commands/Move.lhs
----------------------------------------------------------------------
> hunk ./src/Darcs/Commands/Move.lhs 91
> -moveCmd _ [] = fail "The `darcs move' command requires at least two arguments."
> -moveCmd _ [_] = fail "The `darcs move' command requires at least two arguments."
> +moveCmd opts args
> +  | length args < 2 = fail $ "The `darcs move' command requires at least" ++
> +      "two arguments."
> +  | length args == 2 = do
> +      xs <- maybeFixSubPaths opts args
> +      case xs of
> +        [Just from, Just to]
> +          | from == to -> fail "Cannot rename a file or directory onto itself!"
> +          | otherwise -> moveFile opts from to
> +        _ -> fail "Both source and destination must be valid."
> +  | otherwise = let (froms, to) = (init args, last args) in do
> +      x <- head <$> maybeFixSubPaths opts [to]
> +      case x of
> +        Nothing -> fail "Invalid destination directory."
> +        Just to' -> do
> +          xs <- nub . sort <$> fixSubPaths opts froms
> +          case xs of
> +            [] -> fail "Nothing to move."
> +            froms' -> moveFilesToDir opts froms' to'

Seems better indeed.  Same traffic-control/business end separation with
some subtle checking on the command line args.

I really like the fact that
  darcs mv foo bar /invalid
no longer thinks that you're trying to move foo to bar.

> addfile ./tests/invalid_absolute_paths.sh
> hunk ./tests/invalid_absolute_paths.sh 1
> +## Regression test for patch178

I think the description should be about the functionality, not the patch
number... but maybe you've found a new useful practice.

> +. lib                           # Load some portability helpers.
> +rm -rf R                        # Another script may have left a mess.
> +darcs init      --repo R        # Create our test repos.
> +cd R
> +
> +if darcs move /non_existent_path/a /non_existent_path/b 2>&1 | grep 'bug'; then
> +  echo 'Not OK 1: darcs move causes a bug'
> +  exit 1
> +else
> +  echo 'OK 1'
> +fi
> +
> +if darcs move /non_existent_path/a /non_existent_path/b /non_existent_path/c 2>&1 | grep 'Prelude.init: empty list'; then
> +  echo 'Not OK 2: darcs move causes an error'
> +  exit 1
> +else
> +  echo 'OK 2'
> +fi
> +
> +if darcs annotate /non_existent_path/a 2>&1 | grep 'Pattern match failure'; then
> +  echo 'Not OK 3: darcs annotate causes an error'
> +  exit 1
> +else
> +  echo 'OK 3'
> +fi

The test could be made a lot simpler and maybe easier to read
if you just rely on the test harness exiting on error.

For example

 darcs move /non_existent_path/a /non_existent_path/b > log
 not grep bug log

NOTE: The test doesn't seem sufficiently aggressive to me.  Shouldn't it
be checking that the moves are interpreted correctly?  Shouldn't it be
checking that eg darcs move a b /invalid fails in the right way?

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
For a faster response, try +44 (0)1273 64 2905 or
xmpp:kowey@jabber.fr (Jabber or Google Talk only)

> GENERAL COMMENTS
> ----------------------------------------------------------------------
> There are two more commands that take subpaths, I think: revert and
> darcs show contents.  I think we may need a follow-up patch to cover
> these and other subpath commands I may not have thought of.
> 
> I've prefixed the more important comments with NOTE
> 
> Please send your recent addendum as a new patch to screened
> branch.

Actually, maybe one way to improve things (perhaps in the same bundle as
the follow-up patch) is a test for each of the issues that you resolve.

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
For a faster response, try +44 (0)1273 64 2905 or
xmpp:kowey@jabber.fr (Jabber or Google Talk only)

This patch bundle (with 16 patches) was just applied to the repository http://darcs.net/.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-5b51194896275147bdea9152a3fa56808f7e5785

This patch bundle (with 3 patches) was just applied to the repository http://darcs.net/.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-8f4f9bfe469a36991b6b05d4bccc11413053c94e

Eric Kow <bugs@darcs.net> writes:

> Note that I'm a bit dissatisfied with the test case (see below).  Part
> of me is tempted not to apply the patch in a Zooko-like spirit of being
> very demanding about testing in the interests of faster progress (*),
> but I feel a bit like I've played a big part in this patch stalling
> unnecessarily and making work for everybody, so I'm applying it a little
> bit out of guilt.
>
> Well, guilt and the fact that it really is an improvement :-)
> Applied, thanks!

Turns out this broke a large number of tests on Windows. The pattern is
this:

"ssh": Could not resolve hostname C: no address associated with name

when passing absolute drive-qualified paths to darcs. Anyone tackling
this can use buildbot-try if they don't have access to a windows box
(although with a bit of creativity it should be possible to test this
without windows). If you are not a reviewer, you may need to contact me
to get access to the functionality.

I could also, in theory, hand out interactive (rdesktop) access to
windows machine(s) with HP, MSYS and darcs installed, against a promise
to not break anything. If you need this, contact me. I can't give out
permanent access though -- I will create throwaway accounts on demand,
for a couple of days at a time, or something.

Yours,
   Petr.

This patch bundle (with 3 patches) was just applied to the repository http://darcs.net/reviewed.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_reviewed.html#bundle-8f4f9bfe469a36991b6b05d4bccc11413053c94e

This patch bundle (with 16 patches) was just applied to the repository http://darcs.net/reviewed.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_reviewed.html#bundle-5b51194896275147bdea9152a3fa56808f7e5785