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

[Reinier Lamers]

2008/10/29 David Roundy <droundy at darcs.net>:
>> ] 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 have to confess that I hadn't thought very hard about the case that
a user would specify overlapping paths (which, as I understand it, is
where the problem lies).

But with darcs-2 semantics, two identical primitive patches just add
up to one copy of that change, right? It's still heavily broken on
darcs-1 repos of course.

> 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.)

I'll try out the test and see what it does with these patches applied.

> 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!

In fact, making record use this function was (and is) in my planning.
I'll just fix it first.

Reinier