[darcs-users] [patch368] Fix for issue1932

Eric Kow kowey at darcs.net
Fri Aug 27 12:25:48 UTC 2010


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 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/20100827/aa93b30b/attachment.pgp>


More information about the darcs-users mailing list