[darcs-users] [patch368] Fix for issue1932

Dmitry Astapov dastapov at gmail.com
Thu Aug 26 19:26:26 UTC 2010


Hi,

On Thu, Aug 26, 2010 at 6:48 PM, Eric Kow <kowey at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osuosl.org/pipermail/darcs-users/attachments/20100826/dbc27930/attachment.html>


More information about the darcs-users mailing list