[darcs-devel] darcs patch: add function for finding all file names ... (and 4 more)

[Tommy Pettersson]

On Mon, Nov 13, 2006 at 04:36:01PM +0100, Eric Y. Kow wrote:
> I'm going to accept these patches for more testing.  If I understand
> correctly, the changes are that:
> 
> 1. the check is performed in the outer layer (commands)
>    instead of within the inner layer (patches)
> 2. the check is now optional (and it is the possibility of disabling this check
>    that resolves issue177

Correct.

> > The main problem with the old way is the apply function in
> > PatchApply is not the only "entry point" to applying patches.
> 
> It's not?  That's interesting.  What other entry points are there?
> [ i'm not asking for a full list :-) ] 

Here are the main points of the problem:

* apply calls apply_list for composite patches, which recursively
  calls apply for the sub patches, unless sometimes it performs
  the apply itself, for example to handle the
  set-script-executable hack. (This is how I discovered there were
  problems.)

* force_replace_slurpy is an exported function that doesn't call
  apply.

* apply_to_filepaths and apply_to_slurpy are exported functions
  (that does call apply, I think, but) that removes the options
  (e.g., to turn the check off) for some particular reason.

* Git imports applyBinary and applyHunkLines (very primitive
  functions) and uses them directly.

All of this can be handled (Turing complete, and so on...), but
there are other issues too. Darcs often apply the same patch
more than once, e.g., to both working and pristine, and also to
produce intermittent results. Checking the same patch over and
over again is not good. It's better to check on every write or
every read. Checking writes is probably safest. Checking reads
gives the best error messages. Which of checking reads or
checking writes is most efficient is harder to tell. It depends
on if darcs reads many more patches without writing them then
writing patches several times.

> > It unfortunately only checks
> > remote patches fetched with Apply or Pull, but it does check
> > them reliably.
> 
> Did you overlook Pull by any chance?  I see only code for checking with
> Check and Apply.  Note that Pull does not invoke anything from Apply,
> so either a refactor or a separate check would be necessary.

Argh! I removed the wrong lines. The patch contains the Check
check that freezes darcs without feedback for a while. I'll
attach a patch that restores order.

> > +is_malicious_path :: String -> Bool
> > +is_malicious_path fp =
> > +    not $ is_explicitly_relative fp  &&
> > +          null (intersect (breakup fp) [ "..", "_darcs" ])
> 
> I personally found the old code easier to understand.  How about
> something like this
> 
> is_malicious_path :: FilePath -> Bool
> is_malicious_path fp =
>     not (is_explicitly_relative fp) || 
>     breakup fp `contains_any` [ "..", "_darcs" ]
>  where
>     contains_any a b = not . null $ intersect a b

I'll throw this in with an extra patch too.


-- 
Tommy Pettersson <ptp at lysator.liu.se>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ,bundle
Type: text/x-darcs-patch
Size: 3765 bytes
Desc: not available
Url : http://lists.osuosl.org/pipermail/darcs-devel/attachments/20061113/8cb84079/bundle.bin