[darcs-users] [patch368] Fix for issue1932

Eric Kow kowey at darcs.net
Mon Aug 30 09:46:19 UTC 2010


On Sat, Aug 28, 2010 at 00:50:54 +0300, Dmitry Astapov wrote:
> My chain of reasoning:
> * I assume that isRelative has a latent bug that will fire on Windows in
> certain cases by treating relative paths as absolute
> * There is a single call site for isRelative - it's the simpleSubPath
> * simpleSubPath, in turn, is used in two places:
>   - In Add.lhs to fix/check names of the files added to the repo
>   - In Arguments.lhs, in function fixSubPaths, for processing command line
> arguments and detecting (im)proper directory names.

> * Now, this latent bug could not fire during "darcs add" since all filepaths
> processed there are generated by traversing directory tree, and they, by
> definition, are relative to the repo root and would not contain drive
> letters.

Is this link (in the chain) correct?  Doesn't the user sometimes supply
filepaths from the command line (with Darcs having to verify that they
really are under the repo root once you've canonised them)?

> * We are left with the case when user-supplied filepath is used as the repo
> name (for darcs get or push, or similar). Voila, that is what the issue1240
> is about!
> 
> So maybe reporter of the issue1240 could be prodded to produce the shell
> test harness and test whether this patch fixes that bug as well. Or maybe
> someone could build a win32 darcs for me :)
 
> > One possibility actually is if isRelative is only used by RepoPath.hs to
> > just
> > nuke it altogether and have RepoPath do the reasoning via FilePath.  There
> > we'll have to ask ourselves if at RepoPath time, we need to care about
> > remote
> > paths or not, which we probably don't
> >
> 
> But we probably do!  Since simpleSubPath (which calls isRelative) is
> used to process command line arguments as well (see above). Maybe the
> proper course of action would be to move simpleSubPath out of RepoPath
> into Arguments.lhs, and replace call to simpleSubPath in Add.lhs with
> something simplier.

Hmm, it looks like either that type signature is wrong (it claims to
go from FilePath -> Maybe SubPath) or it's being too pessimistic.

Or maybe we need two layers of path control: one that distinguishes
between remote and local paths [I guess "remote" meaning without the
filesystem abstraction given to us by the OS, so something like NFS
or even sshfs would be perfectly "local"], and one that cares only
about what happens once you have local paths.

> > Meanwhile, could I abuse you to somehow make our path testing logic
> > much more aggressive and evil?

> Maybe :)
> 
> What do you mean by that?

First the background: We've been talking a lot about the need for more
aggressive testing in Darcs, but we never take very much concrete action
due to a perpetual time starvation (which is a vicious cycle of course,
because part of the time starvation is due to us fixing bugs from
insufficient bug pre-emption).  So we kind of need an intervention...
somebody to roll up their sleeves and show us how a test-heavy culture
is run.

So I don't really know what I mean!  I guess Darcs.URL is fairly small
and self-contained, and it would be silly to replicate the already good
tests in the filepath package.  But are there tests for, say the
interaction between isUrl, isSsh and the others?  What about the
Darcs.RepoPath module?  Would it be appropriate to add tests for that?

Vague ideas I was hoping for:

 - tests that go out of their way to push the limits of what various
   popular filesystems can support

 - tests of the "darcs should not accept this innocent looking but evil
   thing" variety

 - tests for the interaction between remote path and windows path
   detection (on two levels, this could be [A] unit-level, darcs treats
   this as remote, but treats that as windows, etc (HUnit?) and [B]
   functional-level, the implications of darcs treating this path as
   local/remote for specific commands is... (what if I want to push to
   c:myrepo)?
 
 - QuickCheck properties that should hold true about paths? 
   (maybe mutually exclusiveness of isAbsolute and isRelative?
   of course, without trying to duplicate the already good battery
   in the filepath package, maybe round-tripping from subpath and
   back)

Now I don't mean to shove a bunch of demands on your plate. It certainly
wouldn't be fair for us to set this kind of standard on you that we
haven't already set on ourselves in the past, but if you want to help us
out of the woods a bit, this is the sort of thing you could consider
helping with.

What do you think? Basically, do you see anything that jumps out at you
as something that needs explicit testing?

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
For a faster response, try +44 (0)1273 64 2905 or
xmpp:kowey at jabber.fr (Jabber or Google Talk only)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.osuosl.org/pipermail/darcs-users/attachments/20100830/a2c17581/attachment.pgp>


More information about the darcs-users mailing list