On Mon, Aug 30, 2010 at 12:46 PM, Eric Kow <ko...@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
_______________________________________________ darcs-users mailing list darcs-users@darcs.net http://lists.osuosl.org/mailman/listinfo/darcs-users
