1 patch for repository http://darcs.net/repos/unstable:

Wed Aug 25 20:30:45 EEST 2010  Dmitry Astapov <dastapov@gmail.com>
  * Fix for issue1932
  Do _not_ check for colons in "isRelative" - everything is covered
  inside Add.lhs and governed by use of "--reserved-ok" option.
  
  Replace isRelative and isAbsolute with functions from System.FilePath
  for correct handling of platform specifics.


I've run unit tests and shell tests - everything seems to be in order.

Attachments

• fix-for-issue1932.dpatch
• unnamed

On Wed, Aug 25, 2010 at 19:34:05 +0300, Dmitry Astapov wrote:
> Let's chat a bit first, though. The real issue at hand is with handling
> "tricky" characters in file names. On Unix, I can have ':' and '?' or '<' in
> my filename, while on Windows they are forbidden. It seems to me that no
> attempt is made to handle this characters in some special way anywhere in
> the darcs, so I guess that the official position is that users are allowed
> to shoot themselves in the foot however they like.

Note that Darcs makes some effort to detect case sensitivity issues and
reserved filenames (eg. AUX or COM[1-9]), saying "hey, if you add this
file, Darcs is going to have a lot of trouble on Windows" (not an actual
UI message).

So we do seem to discourage shooting self in foot, but making provisions
for people to opt-in with flags like --case-ok and --reserved-ok. Is it
just the case that --reserved-ok isn't working hard enough to pick up
the ':', '?' and '<' characters?

See issue53 for more details

> I am fine with that and will assume this much for the rest of the text. So,
> the correct behavior for darcs would be to add files like "aaa:bbb" and even
> "c:\src" to the repository, as long as those files really exist and are
> really files.

So following the current Darcs UI, even if we did discourage the user
for adding these, it is conceivable for such filenames to exist

> Now, colon has a special role in Darcs, since it is used in names of remote
> scp-accessible repositories. It seems that a colon in repository name should
> automatically mean that it is either a HTTP repo or an scp repo.

Oh, of course!  Very sorry for forgetting about this; it didn't occur to
me as I was mechanically doing my BTS triage.

> Unless we make a drastical change and start requiring "scp://" in
> front of the scp repo names, all colons in repo dir names should be
> disallowed.

*Requiring* ssh repos to be URIs could be tricky and perhaps involve
a painful deprecation process (which isn't to say we shouldn't do it
just that it will take some dedication)

On the other hand, supporting such paths seems to be quite clearly
a good thing (see issue1575, and also issue1576 for a bonus feature)

> Then, functions in Darcs.URL clearly serve to distinguish between possible
> forms of repo names.
> * isUrl should be true for names like http://....
> * isSsh should be true for all names that are not URLs and containin ':'
> * isFile should be true whenever both isSSH and isURL are false.
> 
> I browsed through the source and isFile, isSsh, and isUrl are indeed used to
> differentiate between possible forms of repository names everywhere, except
> for the "isRelative" in URL.hs

By the way, isRelative seems to now have a redundant colon-checking case
(redundant with isFile).  Not sure if there's some sort of wise
robustness thing behind it (like code-change-proofing) or if it was just
an oversight.

> 1)Replace isRelative from URL.hs by isRelative from System.FilePath, which
> should take into account differences between platforms and handle colon
> under unix in sensible way.

Would isFile && System.FilePath.isRelative have the same benefits?

Note that in *general* Darcs code, we have a policy of using exclusively
System.FilePath.Posix out of conservatism (filepaths are a really
important part of darcs as they are part of many patch types, eg.
hunk patches, so we want to make sure that we're always handling them
*exactly* the same way)

> 2)Submit the proposal to mark ssh-accesible repositories by "ssh://" and
> replace a horrible ad-hoc implementations of isSsh and isFile with much
> better ad-hoc implementation:
> 
> isSsh f = "ssh://" `isPrefixOf` f
> isUrl f = not (isSsh f) && "://" `isInfixOf` f
> isFile f = not (isSsh f || isUrl f)

Part of this is issue1575.  Well, again here we're running into the
conservatism issue.  This time it's the
dont-break-what-users-are-used-to-unless-you-have-a-good-reason
conservatism (and not the
whoah-be-careful-not-to-accidentally-make-something-wrong
conservatism from above).

So I want to be very very very cautious about it and make sure that we
are thinking long and hard about what we're about to do.

Thanks for checking carefully!  The path handling in Darcs is in a
somewhat sorry state, and I would be thrilled to see it cleaned up.
You may want to use Search functionality on the tracker to look for
tickets with topic FilePath and status "not resolved".  Also,
patch343 could be of interest.

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
For a faster response, try +44 (0)1273 64 2905 or
xmpp:kowey@jabber.fr (Jabber or Google Talk only)

Sorry if anybody was confused by the traffic.  I've added msg12315 
(originally made in context of patch362) to this ticket as I believe 
it's relevant as sort of general commentary.

So I think this is a sort of patch that *may* have wider reaching 
implications that it looks and we want to be very very careful that we 
think through the effects of accepting this patch.

While I can see how this might improve the portability of Darcs, I'd be 
worried about any sort of impact to consistency in its behaviour (if 
each platform that Darcs is built on will have its own idiosyncratic 
behaviour).

We particularly want to worry about what the implications are for 
Darcs.Patch.*

Maybe we need to send this through quite a few pairs of eyes to make 
sure we're not missing something?

Hi,

On Thu, Aug 26, 2010 at 6:48 PM, Eric Kow <kowey@darcs.net> wrote:

> On Wed, Aug 25, 2010 at 19:34:05 +0300, Dmitry Astapov wrote:
>


> Note that Darcs makes some effort to detect case sensitivity issues and
> reserved filenames (eg. AUX or COM[1-9]), saying "hey, if you add this
> file, Darcs is going to have a lot of trouble on Windows" (not an actual
> UI message).
>
> So we do seem to discourage shooting self in foot, but making provisions
> for people to opt-in with flags like --case-ok and --reserved-ok. Is it
> just the case that --reserved-ok isn't working hard enough to pick up
> the ':', '?' and '<' characters?
>

No, that's not the whole story. When file with colon in the filename is
present inside a repo (but not yet added):
1)"darcs add" without "--reserved-ok" will abnormally terminate with
"fromJust: Nothing"
2)"darcs add" with "--reserved-ok" will terminate in exactly the same way

For other reserved chars the behavior is correct, though: skipping files
without --reserved-ok, adding them with --reserved-ok.

So, colon is a special case among all reserved characters, and it is solely
because the same function (isFile) is used to distinguish between local and
remote repositories in "darcs get" and "darcs put" and to distinguish
between absolute and relative filepaths in isRelative.

My intention is to make colon less special, at least when adding files to
the repo.


> See issue53 for more details
>
Yep, thank you for the information.


>
> > I am fine with that and will assume this much for the rest of the text.
> So,
> > the correct behavior for darcs would be to add files like "aaa:bbb" and
> even
> > "c:\src" to the repository, as long as those files really exist and are
> > really files.
>
> So following the current Darcs UI, even if we did discourage the user
> for adding these, it is conceivable for such filenames to exist
>

Yep. The previous paragraph should actually say "the correct behavoir  ....
would be to add files ... given that --reserved-ok is provided".


>
> > Unless we make a drastical change and start requiring "scp://" in
> > front of the scp repo names, all colons in repo dir names should be
> > disallowed.
>
> *Requiring* ssh repos to be URIs could be tricky and perhaps involve
> a painful deprecation process (which isn't to say we shouldn't do it
> just that it will take some dedication)
>

It would definitely be nice to implement them as a feature. Then a smooth
plan for phasing-out old repo names could be devised.


>
> On the other hand, supporting such paths seems to be quite clearly
> a good thing (see issue1575, and also issue1576 for a bonus feature)
>
> > Then, functions in Darcs.URL clearly serve to distinguish between
> possible
> > forms of repo names.
> > * isUrl should be true for names like http://....
> > * isSsh should be true for all names that are not URLs and containin ':'
> > * isFile should be true whenever both isSSH and isURL are false.
> >
> > I browsed through the source and isFile, isSsh, and isUrl are indeed used
> to
> > differentiate between possible forms of repository names everywhere,
> except
> > for the "isRelative" in URL.hs
>
> By the way, isRelative seems to now have a redundant colon-checking case
> (redundant with isFile).  Not sure if there's some sort of wise
> robustness thing behind it (like code-change-proofing) or if it was just
> an oversight.
>

According to the comments in URL.hs, path like "C:blah-blah-blah" is
considered absolute under Windows with rationale that mentioning drive name
in path makes it absolute. In fact, it is not the case, since "C:blah" is
relative to current working dir on drive C: and only stuff like "C:\blah"
would be absolute.

So I treated that clause as a bug lying in waiting and removed it.



> > 1)Replace isRelative from URL.hs by isRelative from System.FilePath,
> which
> > should take into account differences between platforms and handle colon
> > under unix in sensible way.
>
> Would isFile && System.FilePath.isRelative have the same benefits?
>

I don't really understand the question. Please note that all current call
sites for isFile are left intact, and code for isFile is left intact. Only
isRelative is changed, and it is used in the single place (RepoPath.hs), and
thus it is easy to reason about possible repercussions. See more below.


>
> Note that in *general* Darcs code, we have a policy of using exclusively
> System.FilePath.Posix out of conservatism (filepaths are a really
> important part of darcs as they are part of many patch types, eg.
> hunk patches, so we want to make sure that we're always handling them
> *exactly* the same way)
>

Since isRelative should really-really-really detect the relative paths, and
do it in platform-specific way, I believe that System.FilePath is the way to
go in this particular case.

Consider issue1240. I believe that broken "isRelative" is a strong candidate
for being a root cause of this issue. Specifically, user provided a path
that should be considered absolute under the platform-specific conventions,
and isRelative failed to handle that. I have a strong belief that this patch
solves issue1240 - maybe someone with Windows could check that.

Summarizing: I still insist that my patch fixes the issue at hand, and has
no ill side-effects (pun intended).


> > 2)Submit the proposal to mark ssh-accesible repositories by "ssh://" and
> > replace a horrible ad-hoc implementations of isSsh and isFile with much
> > better ad-hoc implementation:
> >
> > isSsh f = "ssh://" `isPrefixOf` f
> > isUrl f = not (isSsh f) && "://" `isInfixOf` f
> > isFile f = not (isSsh f || isUrl f)
>
> Part of this is issue1575.  Well, again here we're running into the
> conservatism issue.  This time it's the
> dont-break-what-users-are-used-to-unless-you-have-a-good-reason
> conservatism (and not the
> whoah-be-careful-not-to-accidentally-make-something-wrong
> conservatism from above).
>

I clearly have not done my homework on this "URL-style paths for ssh repos".
Since it is out of scope of the current bug and fix, I suggest we drop it
for now, and I might resurface later in the discussions for appropriate
issues.


> Thanks for checking carefully!  The path handling in Darcs is in a
> somewhat sorry state, and I would be thrilled to see it cleaned up.
> You may want to use Search functionality on the tracker to look for
> tickets with topic FilePath and status "not resolved".  Also,
> patch343 could be of interest.
>

Yes, thank you! Will do.

-- 
Dmitry Astapov

Attachments

• unnamed

On Thu, Aug 26, 2010 at 22:26:26 +0300, Dmitry Astapov wrote:
> 1)"darcs add" without "--reserved-ok" will abnormally terminate with
> "fromJust: Nothing"
> 2)"darcs add" with "--reserved-ok" will terminate in exactly the same way

Great (and thanks for marking Kirstin's bug a duplicate).
Nice to see we're starting to nail things down properly.

> So, colon is a special case among all reserved characters, and it is
> solely because the same function (isFile) is used to distinguish
> between local and remote repositories in "darcs get" and "darcs put"
> and to distinguish between absolute and relative filepaths in
> isRelative.

Right, and I have trouble keeping the two thoughts in my head at the
same time (colon for Windows, and colon for SSH).  You push one thought
in, and the other thought slides out.

> > > Unless we make a drastical change and start requiring "scp://" in
> > > front of the scp repo names, all colons in repo dir names should be
> > > disallowed.
> >
> > *Requiring* ssh repos to be URIs could be tricky and perhaps involve
> > a painful deprecation process (which isn't to say we shouldn't do it
> > just that it will take some dedication)
> >
> 
> It would definitely be nice to implement them as a feature. Then a smooth
> plan for phasing-out old repo names could be devised.

Sounds good.

> According to the comments in URL.hs, path like "C:blah-blah-blah" is
> considered absolute under Windows with rationale that mentioning drive name
> in path makes it absolute. In fact, it is not the case, since "C:blah" is
> relative to current working dir on drive C: and only stuff like "C:\blah"
> would be absolute.
> 
> So I treated that clause as a bug lying in waiting and removed it.

It may even be worth filing a ticket for, not sure.
At the very least mentioning in the patch comments

> > > 1)Replace isRelative from URL.hs by isRelative from System.FilePath,
> > which
> > > should take into account differences between platforms and handle colon
> > > under unix in sensible way.
> >
> > Would isFile && System.FilePath.isRelative have the same benefits?
> >
> 
> I don't really understand the question. Please note that all current call
> sites for isFile are left intact, and code for isFile is left intact. Only
> isRelative is changed, and it is used in the single place (RepoPath.hs), and
> thus it is easy to reason about possible repercussions. See more below.

I saw this particular bit of the hunk:

-isRelative f@(c:_) = isFile f && c /= '/' && c /= '~'

And I was afraid of losing the check, but on further inspection isFile
has the undesired checking-for-colon behaviour (which I think you were
saying and I was just missing).  So sorry, I was just being silly.

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

> Since isRelative should really-really-really detect the relative paths, and
> do it in platform-specific way, I believe that System.FilePath is the way to
> go in this particular case.

Sounds convincing.
 
> Summarizing: I still insist that my patch fixes the issue at hand, and has
> no ill side-effects (pun intended).

OK! So I'm a lot less nervous about this having slept on it (I realised that
this is actually still quite surface level stuff that shouldn't affect any of
the inner patch-level stuff).  Let me see if I can find you a patch reviewer.

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

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
For a faster response, try +44 (0)1273 64 2905 or
xmpp:kowey@jabber.fr (Jabber or Google Talk only)

Are you ready to take on more review, Florent?  Thanks!

On Fri, Aug 27, 2010 at 3:25 PM, Eric Kow <kowey@darcs.net> wrote:

>
> > According to the comments in URL.hs, path like "C:blah-blah-blah" is
> > considered absolute under Windows with rationale that mentioning drive
> name
> > in path makes it absolute. In fact, it is not the case, since "C:blah" is
> > relative to current working dir on drive C: and only stuff like "C:\blah"
> > would be absolute.
> >
> > So I treated that clause as a bug lying in waiting and removed it.
>
> It may even be worth filing a ticket for, not sure.
> At the very least mentioning in the patch comments
>

I have a strong suspicion that issue1240 is already a proper ticket for
this. Unfortunately, right now I dont have access to Windows box capable of
building bleeding-edge darcs in order to test it.

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


> > Summarizing: I still insist that my patch fixes the issue at hand, and
> has
> > no ill side-effects (pun intended).
>
> OK! So I'm a lot less nervous about this having slept on it (I realised
> that
> this is actually still quite surface level stuff that shouldn't affect any
> of
> the inner patch-level stuff).  Let me see if I can find you a patch
> reviewer.
>
> Meanwhile, could I abuse you to somehow make our path testing logic much
> more
> aggressive and evil?
>

Maybe :)

What do you mean by that?

-- 
Dmitry Astapov

Attachments

• unnamed

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@jabber.fr (Jabber or Google Talk only)

On Mon, Aug 30, 2010 at 12:46 PM, Eric Kow <kowey@darcs.net> wrote:

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

This is taken care of in Arguments.lhs. When the absolute filepath or the
like is specified on the command like, darcs would either say "Ignoring
non-repository path /foo" or convert path to the relative one even before it
gets to the Add.lhs.

I've just verified that by tracing the code and by adding "trace" and
running through test suite.


> > * 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 :)
>
I hope to check this with darcs-2.4.4-release on Windows soon.



>  > > 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 I am slightly wrong - when I added trace to the simpleSubPath and
run through the testsuite, I haven't seen a single invocation of
simpleSubPath for handling of command line args.

I'll re-check this.


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

Something like this seems to be already in place in RepoPath.hs, I need to
look better/deeper.


>
> > > Meanwhile, could I abuse you to somehow make our path testing logic
> > > much more aggressive and evil?
>
> > Maybe :)
> >
> > What do you mean by that?
>
> [snip]


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

Definitely. I even set out to do something like that, but quickly found out
that RepoPath does not export constructors of AbsolutePath and SubPath,
which makes writing tests very-very-very hard, since there is no easy way to
construct , for example, AbsolutePath out of String or FilePath.

What would be the best approach for tackling that?

[skip]

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

Yes, I do.
For example, I see lots of candidates in the RepoPath that could be replaced
by their counterparts from System.FilePath. But I would be more confident
with writing tests first and replacing them second.

But I would like all that to be handled separately from this simple issue
and humble patch  :)

-- 
Dmitry Astapov

Attachments

• unnamed

>
>New patches:
>
>[Fix for issue1932
>Dmitry Astapov <dastapov@gmail.com>**20100825173045
> Ignore-this: 68db2c42ef376307271a7f609e62481c
> Do _not_ check for colons in "isRelative" - everything is covered
> inside Add.lhs and governed by use of "--reserved-ok" option.
> 
> Replace isRelative and isAbsolute with functions from System.FilePath
> for correct handling of platform specifics.
>] hunk ./src/Darcs/Commands/Add.lhs 48
>                    , doesFileReallyExist, treeHas, treeHasDir,
treeHasAnycase )
> import Darcs.RepoPath ( SubPath, toFilePath, simpleSubPath, toPath )
> import Darcs.Repository.Prefs ( darcsdirFilter, boringFileFilter )
>-import Data.Maybe ( maybeToList, fromJust )
>+import Data.Maybe ( maybeToList )
> import System.FilePath.Posix ( takeDirectory, (</>) )
> import qualified System.FilePath.Windows as WindowsFilePath
> import Printer( text )
>hunk ./src/Darcs/Commands/Add.lhs 54
> 
> #include "gadts.h"
>+#include "impossible.h"
> 
> addDescription :: String
> addDescription = "Add one or more new files or directories."
>
Why exchange the import of fromJust for the inclusion of impossible? I don't
see the point.

>hunk ./src/Darcs/URL.hs 30
>     * A URL contains the sequence @\":\/\/\"@.
> 
>     * A local filepath does not contain colons, except
>-      as second character (windows drives).
>+      as second character (windows drives) when this
>+      filepath is meant to be used as repository name
> 
>     * A path that is neither a URL nor a local file
>       is an ssh-path.
>hunk ./src/Darcs/URL.hs 56
>     isSshNopath
>   ) where
> 
>+import qualified System.FilePath as FP (isRelative, isAbsolute, isValid)
>+
> #include "impossible.h"
> 
> isRelative :: String -> Bool
>hunk ./src/Darcs/URL.hs 61
>-isRelative (_:':':_) = False
>-isRelative f@(c:_) = isFile f && c /= '/' && c /= '~'
> isRelative "" = bug "Empty filename in isRelative"
>hunk ./src/Darcs/URL.hs 62
>+isRelative f  = FP.isRelative f
> 
> isAbsolute :: String -> Bool
> isAbsolute "" = bug "isAbsolute called with empty filename"
>hunk ./src/Darcs/URL.hs 66
>-isAbsolute f = isFile f && (not $ isRelative f)
>+isAbsolute f = FP.isAbsolute f
>
We are now using the stdlib isRelative and isAbsolute. We still have
isAbsolute = not . isRelative .
 
> isFile :: String -> Bool
>hunk ./src/Darcs/URL.hs 69
>-isFile (_:_:fou) = ':' `notElem` fou
>-isFile _ = True
>+isFile f@(_:_:fou) = ':' `notElem` fou && FP.isValid f
>+isFile f = FP.isValid f
> 
> isUrl :: String -> Bool
> isUrl (':':'/':'/':_:_) = True
>

We still have :

isSsh :: String -> Bool
isSsh s = not (isFile s || isUrl s)

This means (IIUC) that on windows, trying to use a "special" filename
such as "prn" would try to do an "ssh prn". I'm applying this anyway,
but we should give some other error message ("darcs pull prn" --> "no
route to host prn" would be a bit puzzling). Can you do that in a
followup patch?

On Wed, Sep 1, 2010 at 6:39 PM, Florent Becker <bugs@darcs.net> wrote:

>
> Florent Becker <florent.becker@ens-lyon.org> added the comment:
> >
> Why exchange the import of fromJust for the inclusion of impossible? I
> don't
> see the point.
>


So that definition of "fromMaybe" from "impossible.h" would be used. This
would simplify detection of the similar bugs in the future. If that include
was here in the first place, then this bug would have manifested itself with
the useful error message (complete with module name and line number), and
not with "fromJust: Nothing".

As far as I can see, this is a widespread practice (I've grepped for
'#include "impossible.h" to confirm this).


>
> >+isRelative f  = FP.isRelative f
> >
> > isAbsolute :: String -> Bool
> > isAbsolute "" = bug "isAbsolute called with empty filename"
> >hunk ./src/Darcs/URL.hs 66
> >-isAbsolute f = isFile f && (not $ isRelative f)
> >+isAbsolute f = FP.isAbsolute f
> >
> We are now using the stdlib isRelative and isAbsolute. We still have
> isAbsolute = not . isRelative .
>

I'm not sure that I understand the question (if there is any).

We still have :
>
> isSsh :: String -> Bool
> isSsh s = not (isFile s || isUrl s)
>
> This means (IIUC) that on windows, trying to use a "special" filename
> such as "prn" would try to do an "ssh prn". I'm applying this anyway,
> but we should give some other error message ("darcs pull prn" --> "no
> route to host prn" would be a bit puzzling). Can you do that in a
> followup patch?
>

Yes. I'm going to produce a unit test module for RepoPath & URL first,
though, to reduce a chance of nasty regression.

-- 
Dmitry Astapov

Attachments

• unnamed

Hi.

I've just identified the nasty regression this patch causes somewhere deep
inside "darcs pull". I'll investigate and get back to you.

On Wed, Sep 1, 2010 at 6:39 PM, Florent Becker <bugs@darcs.net> wrote:

>
> Florent Becker <florent.becker@ens-lyon.org> added the comment:
>
> >
> >New patches:
> >
> >[Fix for issue1932
> >Dmitry Astapov <dastapov@gmail.com>**20100825173045
> > Ignore-this: 68db2c42ef376307271a7f609e62481c
> > Do _not_ check for colons in "isRelative" - everything is covered
> > inside Add.lhs and governed by use of "--reserved-ok" option.
> >
> > Replace isRelative and isAbsolute with functions from System.FilePath
> > for correct handling of platform specifics.
> >] hunk ./src/Darcs/Commands/Add.lhs 48
> >                    , doesFileReallyExist, treeHas, treeHasDir,
> treeHasAnycase )
> > import Darcs.RepoPath ( SubPath, toFilePath, simpleSubPath, toPath )
> > import Darcs.Repository.Prefs ( darcsdirFilter, boringFileFilter )
> >-import Data.Maybe ( maybeToList, fromJust )
> >+import Data.Maybe ( maybeToList )
> > import System.FilePath.Posix ( takeDirectory, (</>) )
> > import qualified System.FilePath.Windows as WindowsFilePath
> > import Printer( text )
> >hunk ./src/Darcs/Commands/Add.lhs 54
> >
> > #include "gadts.h"
> >+#include "impossible.h"
> >
> > addDescription :: String
> > addDescription = "Add one or more new files or directories."
> >
> Why exchange the import of fromJust for the inclusion of impossible? I
> don't
> see the point.
>
> >hunk ./src/Darcs/URL.hs 30
> >     * A URL contains the sequence @\":\/\/\"@.
> >
> >     * A local filepath does not contain colons, except
> >-      as second character (windows drives).
> >+      as second character (windows drives) when this
> >+      filepath is meant to be used as repository name
> >
> >     * A path that is neither a URL nor a local file
> >       is an ssh-path.
> >hunk ./src/Darcs/URL.hs 56
> >     isSshNopath
> >   ) where
> >
> >+import qualified System.FilePath as FP (isRelative, isAbsolute, isValid)
> >+
> > #include "impossible.h"
> >
> > isRelative :: String -> Bool
> >hunk ./src/Darcs/URL.hs 61
> >-isRelative (_:':':_) = False
> >-isRelative f@(c:_) = isFile f && c /= '/' && c /= '~'
> > isRelative "" = bug "Empty filename in isRelative"
> >hunk ./src/Darcs/URL.hs 62
> >+isRelative f  = FP.isRelative f
> >
> > isAbsolute :: String -> Bool
> > isAbsolute "" = bug "isAbsolute called with empty filename"
> >hunk ./src/Darcs/URL.hs 66
> >-isAbsolute f = isFile f && (not $ isRelative f)
> >+isAbsolute f = FP.isAbsolute f
> >
> We are now using the stdlib isRelative and isAbsolute. We still have
> isAbsolute = not . isRelative .
>
> > isFile :: String -> Bool
> >hunk ./src/Darcs/URL.hs 69
> >-isFile (_:_:fou) = ':' `notElem` fou
> >-isFile _ = True
> >+isFile f@(_:_:fou) = ':' `notElem` fou && FP.isValid f
> >+isFile f = FP.isValid f
> >
> > isUrl :: String -> Bool
> > isUrl (':':'/':'/':_:_) = True
> >
>
> We still have :
>
> isSsh :: String -> Bool
> isSsh s = not (isFile s || isUrl s)
>
> This means (IIUC) that on windows, trying to use a "special" filename
> such as "prn" would try to do an "ssh prn". I'm applying this anyway,
> but we should give some other error message ("darcs pull prn" --> "no
> route to host prn" would be a bit puzzling). Can you do that in a
> followup patch?
>
> ----------
> status: review-in-progress -> accepted-pending-tests
>
> __________________________________
> Darcs bug tracker <bugs@darcs.net>
> <http://bugs.darcs.net/patch368>
> __________________________________
>



-- 
Dmitry Astapov

Attachments

• unnamed

Hi.

This regression is not related to this patch :)

I will be filing separate bug soonish.

On Wed, Sep 1, 2010 at 11:43 PM, Dmitry Astapov <dastapov@gmail.com> wrote:

> Hi.
>
> I've just identified the nasty regression this patch causes somewhere deep
> inside "darcs pull". I'll investigate and get back to you.
>
> On Wed, Sep 1, 2010 at 6:39 PM, Florent Becker <bugs@darcs.net> wrote:
>
>>
>> Florent Becker <florent.becker@ens-lyon.org> added the comment:
>>
>> >
>> >New patches:
>> >
>> >[Fix for issue1932
>> >Dmitry Astapov <dastapov@gmail.com>**20100825173045
>> > Ignore-this: 68db2c42ef376307271a7f609e62481c
>> > Do _not_ check for colons in "isRelative" - everything is covered
>> > inside Add.lhs and governed by use of "--reserved-ok" option.
>> >
>> > Replace isRelative and isAbsolute with functions from System.FilePath
>> > for correct handling of platform specifics.
>> >] hunk ./src/Darcs/Commands/Add.lhs 48
>> >                    , doesFileReallyExist, treeHas, treeHasDir,
>> treeHasAnycase )
>> > import Darcs.RepoPath ( SubPath, toFilePath, simpleSubPath, toPath )
>> > import Darcs.Repository.Prefs ( darcsdirFilter, boringFileFilter )
>> >-import Data.Maybe ( maybeToList, fromJust )
>> >+import Data.Maybe ( maybeToList )
>> > import System.FilePath.Posix ( takeDirectory, (</>) )
>> > import qualified System.FilePath.Windows as WindowsFilePath
>> > import Printer( text )
>> >hunk ./src/Darcs/Commands/Add.lhs 54
>> >
>> > #include "gadts.h"
>> >+#include "impossible.h"
>> >
>> > addDescription :: String
>> > addDescription = "Add one or more new files or directories."
>> >
>> Why exchange the import of fromJust for the inclusion of impossible? I
>> don't
>> see the point.
>>
>> >hunk ./src/Darcs/URL.hs 30
>> >     * A URL contains the sequence @\":\/\/\"@.
>> >
>> >     * A local filepath does not contain colons, except
>> >-      as second character (windows drives).
>> >+      as second character (windows drives) when this
>> >+      filepath is meant to be used as repository name
>> >
>> >     * A path that is neither a URL nor a local file
>> >       is an ssh-path.
>> >hunk ./src/Darcs/URL.hs 56
>> >     isSshNopath
>> >   ) where
>> >
>> >+import qualified System.FilePath as FP (isRelative, isAbsolute, isValid)
>> >+
>> > #include "impossible.h"
>> >
>> > isRelative :: String -> Bool
>> >hunk ./src/Darcs/URL.hs 61
>> >-isRelative (_:':':_) = False
>> >-isRelative f@(c:_) = isFile f && c /= '/' && c /= '~'
>> > isRelative "" = bug "Empty filename in isRelative"
>> >hunk ./src/Darcs/URL.hs 62
>> >+isRelative f  = FP.isRelative f
>> >
>> > isAbsolute :: String -> Bool
>> > isAbsolute "" = bug "isAbsolute called with empty filename"
>> >hunk ./src/Darcs/URL.hs 66
>> >-isAbsolute f = isFile f && (not $ isRelative f)
>> >+isAbsolute f = FP.isAbsolute f
>> >
>> We are now using the stdlib isRelative and isAbsolute. We still have
>> isAbsolute = not . isRelative .
>>
>> > isFile :: String -> Bool
>> >hunk ./src/Darcs/URL.hs 69
>> >-isFile (_:_:fou) = ':' `notElem` fou
>> >-isFile _ = True
>> >+isFile f@(_:_:fou) = ':' `notElem` fou && FP.isValid f
>> >+isFile f = FP.isValid f
>> >
>> > isUrl :: String -> Bool
>> > isUrl (':':'/':'/':_:_) = True
>> >
>>
>> We still have :
>>
>> isSsh :: String -> Bool
>> isSsh s = not (isFile s || isUrl s)
>>
>> This means (IIUC) that on windows, trying to use a "special" filename
>> such as "prn" would try to do an "ssh prn". I'm applying this anyway,
>> but we should give some other error message ("darcs pull prn" --> "no
>> route to host prn" would be a bit puzzling). Can you do that in a
>> followup patch?
>>
>> ----------
>> status: review-in-progress -> accepted-pending-tests
>>
>> __________________________________
>> Darcs bug tracker <bugs@darcs.net>
>> <http://bugs.darcs.net/patch368>
>> __________________________________
>>
>
>
>
> --
> Dmitry Astapov
>



-- 
Dmitry Astapov

Attachments

• unnamed

This patch bundle (with 1 patches) was just applied to the repository http://darcs.net/.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-1bd604d835979374a02bb7bdb57a7e7063a30a67

On Tue, Aug 31, 2010 at 18:36:08 +0300, Dmitry Astapov wrote:
> Definitely. I even set out to do something like that, but quickly found out
> that RepoPath does not export constructors of AbsolutePath and SubPath,
> which makes writing tests very-very-very hard, since there is no easy way to
> construct , for example, AbsolutePath out of String or FilePath.
> 
> What would be the best approach for tackling that?

We seem to be gravitating towards the idea that tests should be baked
into their host modules.  My rendition of this idea is for each module
like Darcs.RepoPath to export a single testSuite :: Test function,
which has the downside of requiring not just QC/HUnit imports in each
module but also test-framework ones as well.

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
For a faster response, try +44 (0)1273 64 2905 or
xmpp:kowey@jabber.fr (Jabber or Google Talk only)

This patch bundle (with 1 patches) was just applied to the repository http://darcs.net/reviewed.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_reviewed.html#bundle-1bd604d835979374a02bb7bdb57a7e7063a30a67