[darcs-users] darcs patch: Add LANGUAGE pragmas for explicit langua... (and 3 more)

[David Roundy]

On Tue, Oct 28, 2008 at 03:36:32PM -0700, Eric Kow wrote:
> Salvo 6!
> 
> Sun Oct 26 00:01:40 BST 2008  Don Stewart <dons at galois.com>
>   * Add LANGUAGE pragmas for explicit language extensions
> 
> Sun Oct 26 16:40:09 GMT 2008  tux_rocker at reinier.de
>   * add a get_unrecorded_in_files to check for unrecorded changes in a subset of working directory
> 
> Sun Oct 26 19:06:12 GMT 2008  tux_rocker at reinier.de
>   * make get_unrecorded_private work with type witnesses again
> 
> Sun Oct 26 19:46:36 GMT 2008  tux_rocker at reinier.de
>   * make whatsnew use the lstat-saving functions to scan the working copy

These look very appealing, and have some nice ideas, but are also buggy.  :(

> ] hunk ./src/Darcs/Repository/Internal.lhs 417
>      ftf <- filetype_function
>      Sealed pend <- read_pending repository
>      debugMessage "diffing dir..."
> +    -- the unsafeCoerceP below is necessary to be able to concatenate
> +    -- pend with NilFL to form dif. See http://hpaste.org/11480
>      let diffs = if null files
> hunk ./src/Darcs/Repository/Internal.lhs 420
> -                  then [smart_diff opts ftf cur work]
> -                  else catMaybes (map (diff_at_path opts ftf cur work) (map fn2fp files))
> -    let workdiff = foldl (+>+) NilFL diffs
> +                  then smart_diff opts ftf cur work
> +                  else let diffsPerFile = catMaybes (map (diff_at_path opts ftf cur work) (map fn2fp files)) 
> +                       in foldr (+>+) (unsafeCoerceP NilFL) diffsPerFile
>          dif = if AnyOrder `elem` opts

There may be other bugs, but this one is sufficient to cause trouble.  This
unsafeCoerceP really is unsafe.  Combining patches together in this way has
serious potential for repository corruption! In particular, if a file is
referenced more than once in the paths, you can get multiple copies of the
same change.

The get_unrecorded_in_files idea is great, and looks like the right sort of
interface, but concatenating patches from separate diffs is way too
fragile.  A better approach would be to pass all the interesting files into
a diff_at_paths.

I'm right now pushing a test that demonstrates this bug in whatsnew.
(Except it passes, since I'm not applying these patches just yet.)

David

P.S. This patch, of course, on its own couldn't cause repository
corruption, since whatsnew doesn't modify the repository.  But it would
only be a matter of time before someone realized that we could apply the
same optimization to record, and then we'd be seriously hosed.  Which is
why patch review is *very* important!

P.P.S. It's nice to see a case where the type witnesses really did catch us
a bug... Note that this code is right at the border between where the type
witnesses protect us and where they don't, since Diff itself isn't
protected by type witnesses.  Fortunately, it was on the safe side of the
border, or I might not have caught it... also fortunately, the bit on the
diff side of the border is sufficiently simple so as to be easily
reviewable.