[darcs-devel] Re: darcs patch: automatically add parent directories for darcs add

[David Roundy]

On Sat, Feb 05, 2005 at 03:40:22AM +0100, Benedikt Schmidt wrote:
> I implemented feature request #20 on the BTS. It passes some simple tests
> and 'make test' but i didn't try anything special.

Thanks! It looks good.  I just have a few comments.  I'm sending them to
the list, since I know there are people there in the process of learning
haskell who may be interested.
 
> -    flist <- if Recursive `elem` opts
> -             then expand_dirs $ map (fix_filepath opts) args
> -             else return $ map (fix_filepath opts) args
> +    flist' <- if Recursive `elem` opts
> +              then expand_dirs $ map (fix_filepath opts) args
> +              else return $ map (fix_filepath opts) args
> +    -- add needed parent directories, don't expand these directories
> +    pdirlist <- nub `liftM` get_parents cur flist'
> +    flist <- return $ pdirlist ++ flist'


Wouldn't it be simpler (and potentially much more efficient) to expand the
original list of files rather than the recursively expanded list?

I'm thinking something like

let origfiles = map (fix_filepath opts) args
parlist <- get_parents cur origfiles
flist' <- if Recursive `elem` opts then expand_dirs origfiles
                                   else return origfiles
let flist = nub (parlist ++ origfiles)

> +get_parents :: Slurpy -> [FilePath] -> IO [FilePath]
> +get_parents cur fs =
> +  concat `liftM` mapM (removeLast . get_parent cur) fs
> +    where removeLast = liftM $ reverse . tail . reverse 

>From the haskell prelude, init = reverse . tail . reverse

But I think we could improve get_parents by making get_parent not return
the original file, see below.

> +get_parent :: Slurpy -> FilePath -> IO [FilePath]
> +get_parent cur f =
> +  if have_parentdir then return [f]
> +     else do parents <- get_parent cur parentdir
> +             return $ parents ++ [f]
> +       where parentdir = get_parentdir f
> +             have_parentdir = slurp_hasdir (fp2fn parentdir) cur

If we rewrote this as

if slurp_hasdir (fp2fn parentdir) cur
  then []
  else do grandparents <- get_parent cur parentdir
          return (grandparents ++ [parentdir])
     where ...

Then we wouldn't need the reverse .  tail . reverse in get_parents.

Also, I prefer to not bind a variable in a where statement if it's only
used once, unless it's really needed as a sort of commenting mechanism.  In
this case "slurp_hasdir (fp2fn parentdir) cur" pretty clearly (at least to
me...) says what it does.

Thanks again.  If you could submit an improved patch, that would be great.
I've already applied your patch, but in particular, the efficiency thing of
no looking for parents of children on recursive adds is important.  The
code clarity issues are less so.
-- 
David Roundy
http://www.darcs.net