[darcs-users] [patch374] ADV: the initial pile

Petr Rockai me at mornfall.net
Thu Sep 2 17:58:57 UTC 2010


Hi,

second part of the reaction.

Eric Kow <kowey at darcs.net> writes:

>> Thu Aug 12 00:36:15 CEST 2010  Petr Rockai <me at mornfall.net>
>>   * Merge Darcs.Patch.FileName into Darcs.Path.
>> 
>> Thu Aug 12 00:47:39 CEST 2010  Petr Rockai <me at mornfall.net>
>>   * Remove the now-redundant sp2fn.
>> 
>> Thu Aug 12 01:05:34 CEST 2010  Petr Rockai <me at mornfall.net>
>>   * Fix announceFiles in WhatsNew (abolish unsafePathFrom*).
>> 
>> Thu Aug 12 01:06:04 CEST 2010  Petr Rockai <me at mornfall.net>
>>   * Restore the ".." check in isMaliciousPath.
>> 
>> Thu Aug 12 01:29:40 CEST 2010  Petr Rockai <me at mornfall.net>
>>   * Fix a subtle bug in onlyHunks with rather curious side-effects.
>
> I stopped here.
>
>> Thu Aug 12 01:30:27 CEST 2010  Petr Rockai <me at mornfall.net>
>>   * Merge Darcs.RepoPath into Darcs.Path.
>
> More to go!
And even more now...

> Merge Darcs.Patch.FileName into Darcs.Path.
> -------------------------------------------
>> -     then do has_target <- treeHasDir cur (fn2fp $ superName $ fp2fn new)
>> +     then do has_target <- treeHasDir cur (fn2fp $ parent $ fp2fn new)
>
> Getting some sort of notion that we a get little renames like this
>
>> -                             result <- foldM removeOnePath (ftf,recorded,unrecorded, []) $
>> -                                           map pathFromSubPath files
>> +                             result <- foldM removeOnePath (ftf,recorded,unrecorded, []) files
>
> And less need for conversions like this.
> (Presumably because we're accepting more FileName and fewer FilePath)

At this point I think we work with FileName (Relative) most of the
time. Although actual filesystem access is still using FilePath. That
will come in later.

>> hunk ./src/Darcs/Patch/Named.lhs 42
>> -import Darcs.Witnesses.Eq ( EqCheck(..) )
>> -import Darcs.Witnesses.Ordered ( FL(..), RL(..), mapFL, concatFL
>> -                               , mapFL_FL )
>> -import Darcs.Patch.Prim ( Prim(..), FromPrim(..), Effect(effect, effectRL), nFn )
>> -#include "impossible.h"
>> -
>> -data Patch C(x y) where
>> -    PP :: Prim C(x y) -> Patch C(x y)
>> -    ComP :: FL Patch C(x y) -> Patch C(x y)
>> -    Merger :: Patch C(x y)
>> -           -> RL Patch C(x b)
>> -           -> Patch C(c b)
>> -           -> Patch C(c d)
>> -           -> Patch C(x y)
>> -    Regrem :: Patch C(x y)
>> -           -> RL Patch C(x b)
>> -           -> Patch C(c b)
>> -           -> Patch C(c a)
>> -           -> Patch C(y x)
>> -
>> -instance FromPrim Patch where
>> -    fromPrim = PP
>> +import Darcs.Witnesses.Ordered ( concatFL )
>> +import Darcs.Patch.Prim ( Effect(effect, effectRL), nFn )
>> hunk ./src/Darcs/Patch/Named.lhs 42
>> -import Darcs.Witnesses.Ordered ( concatFL )
>> hunk ./src/Darcs/Patch/Named.lhs 40
>> -import Darcs.Patch.Info ( PatchInfo, patchinfo, makeFilename )
>> -import Darcs.Patch.Patchy ( Patchy )
>> -import Darcs.Patch.Prim ( Effect(effect, effectRL), nFn )
>> +import Darcs.Patch.Info ( PatchInfo, patchinfo, makeFilename, invertName, idpatchinfo )
>> +import Darcs.Patch.Patchy ( Patchy, Commute(..), Invert(..) )
>> +import Darcs.Patch.Prim ( Effect(effect, effectRL), nFn, Conflict(..) )
>> +import Darcs.Witnesses.Eq ( MyEq(..) )
>> +import Darcs.Witnesses.Ordered ( (:>)(..), (:\/:)(..), (:/\:)(..) )
>> hunk ./src/Darcs/Patch/Named.lhs 41
>> -import Darcs.Patch.Patchy ( Patchy, Commute(..), Invert(..) )
>> +import Darcs.Patch.Patchy ( Patchy, Commute(..), Merge(..), PatchInspect(..), Invert(..) )
>> ]
>> :
>> hunk ./src/Darcs/Patch/Named.lhs 45
>> -import Darcs.Patch.Prim ( Prim(..), FromPrim(..), Effect(effect, effectRL), nFn )
>> +import Darcs.Patch.Prim ( Prim(..), FromPrim(..), Effect(effect, effectRL) )
>
>> hunk ./src/Darcs/Patch/FileName.hs 1
>
> Module is nuked.  I'm assuming that the code moved in Darcs.Path is only
> hunk-moved.
Yes, should be the case.

> I'll just restate my reservations about the future stability of
> Darcs.Path, that we'll need to make it sure that should Darcs.Path
> change in the future our behaviour with respect to pre-existing patches
> does not also change.  Our of the challenges in the Darcs hacking
> context is that correctness for Darcs is partly defined by historical
> precedent, ie.  "does not behave like it used to" can be considered a
> bug. :-/ [I guess folks working on browser etc have similar problems,
> cf. quirks mode, just with somewhat lower stakes]

I keep that in mind, don't worry. Fortunately, darcs is not writing out
particularly stupid filenames into patches, so it should not really
matter much. If the path code breaks on paths as canonic as those in
darcs patches, we will see that right away.

> Remove the now-redundant sp2fn.
> -------------------------------
> Skipping this and just assuming that it's a natural consequence of
> the new consolidated FileName == SubPath == Storage.Hashed.Relative
Indeed.

> Fix announceFiles in WhatsNew (abolish unsafePathFrom*).
> --------------------------------------------------------
>> -      let paths = map toFilePath files
>> -          check = virtualTreeIO (mapM exists $ map unsafePathFromString paths)
>> +      let check = virtualTreeIO (mapM exists files)
>
> Seems nice, but it's not clear to me what the fix actually is.

Because toFilePath was producing a non-canonic path, which then broke
the Path invariants (which is what is unsafe about unsafePathFromString
-- it allows you to build an invariant-breaking path). The type of
"files" and argument type of exists is now the same, so this was a
redundant roundtrip, too.

> Restore the ".." check in isMaliciousPath.
> ------------------------------------------
>> +isMaliciousPath = forbidden [ "_darcs", ".." ]
>> + where forbidden bad (directory -> dir :/: rest) = dir `elem` bad || forbidden bad rest
>> +       forbidden _ _ = False
>
> This is one of those functions where you don't need to check old
> behaviour, just the new one.
>
> forbidden: Nice use of English for clarity.
>
> Unless I misunderstand the (foo -> pattern) syntax, this is just
> recursively walking the path elements and checking to see if any of the
> elements are forbidden.  Why have explicit recursion when we can just
> convert to a list and use any?

For now, I am not allowing a Path to be converted to a list. It will
need a bit of thought whether this is desirable and whether it could
open a loophole in the API.

> Fix a subtle bug in onlyHunks with rather curious side-effects.
> ---------------------------------------------------------------
>>  onlyHunks :: [Sealed (FL Prim C(x))] -> Bool
>>  onlyHunks [] = False
>> -onlyHunks pss = fn2fp f /= "" && all oh pss
>> +onlyHunks pss = fn2fp f /= "." && all oh pss
>>      where f = getAFilename pss
>>            oh :: Sealed (FL Prim C(x)) -> Bool
>>            oh (Sealed (p:>:ps)) = primIsHunk p &&
>
> So we have a problem with assumptions about the representation of "."
> Perhaps we need to introduce some sort of "isRelativeRoot" function and
> abolish explicit checking on "."/"" instead?

We actually have isRoot which we should probably use here. Any use of
fn2fp is unsafe when it comes to path representation (as opposed to
actually looking up files in the filesystem).

Yours,
   Petr.


More information about the darcs-users mailing list