[darcs-users] [patch127] Re: patches to Darcs.Commands.ShowFiles, and repository internals

[Ganesh Sittampalam]

Ganesh Sittampalam <ganesh at earth.li> added the comment:

In general I think this looks good, some detailed comments/requests for
changes/further explanation below.

As you say this is a baby step and noone should rely on the precise signature of
the resulting API functions, though we should certainly aim to always provide
this kind of functionality in some form.

> [in show files, break out manifestCmd' helper function with result type IO
[FilePath], as baby step towards a useful darcs api
> thomashartman1 at gmail.com**20100102140103
>  Ignore-this: 5290a1b50a4a23a961138920de7bc6b4
> ]

This patch has a spurious whitespace change at the end.

Also, I'm not convinced by the naming of "filesCmd'" for an exported function
intended to be in the API. I'd suggest renaming things appropriately so this can
be "filesCmd" or "filesCommand".

> [simplify output subfunction
> thomashartman1 at gmail.com**20100102161308
> Ignore-this: b32d46fdfa02ee74425966747a711226
> ]
> -  where output_null name = do { putStr name ; putChar '\0' }
> -        output = if NullFlag `elem` opts then output_null else putStrLn
> +  where output name = do putStr name
> +                         putChar $ if NullFlag `elem` opts then '\0' else '\n'

Is this change correct on Windows? I suspect so but we should check.

> [make monad pipeline explicit
> thomashartman1 at gmail.com**20100102174548
>  Ignore-this: 8f10d4403232fc96f55834833736403c
> ]

> -         do x <- do pid <- runProcess c args Nothing Nothing (Just h) Nothing
Nothing
> -                    waitForProcess pid
> +         do x <- do waitForProcess  =<< runProcess c args Nothing Nothing
(Just h) Nothing Nothing
>              when (x == ExitFailure 127) $

In general this kind of change feels rather like churn, though I think in this
particular case it might be a slight improvement. However you have left in an
unnecessary "do".

> [make slurpPristine use absolute paths (no more need to wrap in
getCurrrentDirectory/withCurrentDirectory)
> thomashartman1 at gmail.com**20100102175426
>  Ignore-this: 9e0d47590c4b92d9c6c774730136113e
> ]

I'm not convinced this patch is correct, since mmap_slurp itself looks at the
current directory. I definitely support the intention though. Could you provide
more justification of why it's ok?

----------
status: needs-review -> review-in-progress

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