Darcs maintainers,

Below is a bug report from a Debian user (coincidentally, also me).
The associated Debian BTS ticket is http://bugs.debian.org/458629.
Please ensure that 458629@bugs.debian.org remains in the nosy list for
this roundup ticket.

This bug still exists in 2.0.0.

> Where x is a repository and y is a patch bundle,
>
>   $ darcs apply --repodir x ./y
>   darcs: y: openBinaryFile: does not exist (No such file or directory)
>
> This behaviour is confusing, and the error message is unhelpful.
> Once it occurs to the user to try
>
>   $ darcs apply --repodir x $PWD/y
>   Finished applying...
>
> It becomes obvious that is chdir'ing before attempting to access the
> patch bundle.  Either it should not do so, or it should print a more
> meaningful error message (for example, including the absolute path
> that it tried to open).

Note that this is not limited to apply; other commands are affected:

  $ mkdir x y
  $ map darcs init --repodir -- x y
  $ darcs pull --repodir x y

  darcs failed:  Not a repository: y (y/_darcs/inventory: openBinaryFile: does not exist (No such file or directory))
  $ darcs pull --repodir x $PWD/y
  No remote changes to pull in!
  $ darcs push --repodir x y

  darcs failed:  Not a repository: y (y/_darcs/inventory: openBinaryFile: does not exist (No such file or directory))
  $ darcs push --repodir x $PWD/y
  Pushing to "/tmp/with-temp-dir.G26031/y"...
  No recorded local changes to push!

Hmmph :-( I don't have a clear idea how repodir should interact with paths.  

Is there any reason that a user would find it more intuitive for relative paths
to be considered as being relative to the repo-root and not the current root? 
On the surface, it seems like Trent is right, and that we should modify path
fixing accordingly.

We should probably sit down and thoroughly review the path-fixing behaviour.  I
think the key problem is that we have not fully worked out all the possible
cases (when should we translate relative paths, and to what).

The basic idea is that darcs changes directory to the repository root, so some
of the paths you supply on the command line (or in switches) must be modified
accordingly.

For example, if you are in
  repo/foo

And you do
  darcs whatsnew ./bar

Darcs must then "fix" that path to "foo/bar".  Likewise,
  darcs whatsnew ../baz

Should be fixed to just "baz"

But then there's all sorts of factors you need to control for like paths outside
the repository (when is this acceptable?) and --repodir

This bug may not be so scary to fix, as fiddly as path fixing can be.

The idea is that we want paths to be fixed so that in all cases, relative paths
are interpreted as being relative to where the user was whilst typing in the
command.  So if --repodir is used, we somehow need to pass along the cwd wrt
repodir.

== From SimonPJ on issue1027 ==

sh-2.04$ darcs2 pull ../HEAD --repodir libraries/array

darcs failed:  Not a repository: ../HEAD (../HEAD/_darcs/inventory:
openBinaryFile: does not exist (No such file or directory))

Again this is on Windows.  But "../HEAD" *is* a repository.  (Giving the full
path name works.)  Again, this is horrible for the user.

Simon

I have encountered a similar bug - 'darcs send -o ../foo.dpatch' puts patch in
the current dir, not the parent.

Looking at the code I see that fix_flag function makes absolute paths from
arguments using "make_absolute pwd arg".

But make_absolute just skips '../'. I have changes makeAbsolute to respect ../
and -o works fine now, all tests pass. But I suspect it was ignoring ../ for a
reason, so it breaks something.

Eric, you say `darcs whatsnew ../baz` should be fixed to just baz. Can you
explain why?

I think that to fix relative paths in options we should change fix_flags to
respect '../'. Fixing relative paths (like darcs apply --repodir x ./y) can be
more tricky because of chdir's. But at least we can do the first step (and I am
looking at it now).

Regards,
  Dmitry

Ah, good!  I'm glad to have somebody other than me look into path fixing (fresh
perspective, etc)

> Eric, you say `darcs whatsnew ../baz` should be fixed to just baz. Can you
> explain why?

Yes.  Because the user is in "repo/foo", but for simplicity [*], darcs changes
directory to "repo", and repo/foo/../baz is just the same as repo/foo/baz


[*] The "simplicity" here is that once we have changedired, we know exactly what
to expect; all paths are repo-relative.  The complexity is that we have to write
path-fixing code to make sure everything falls in line with the user's expectations

Thanks, now I understand.

Do you have an idea why makeAbsolute from RepoPath ignores ../? E.g.
makeAbsolute /foo ../bar = /foo/bar, not /bar as one might expect.

It seems to me makeAbsolute should be something like:

> makeAbsolute p1 (RelativePath p2) = ma p1 p2
>  where ma p ('.':'.':'/':r) = let pp = takeDirectory p
>                               in if pp == p then AbsolutePath r else ma pp r
>        ma p r = combine p (SubPath r)

This way ../ in -o and other parameters works as expected. And all tests pass.
But I am still worried if it breaks something...

Regards,
  Dmitry

> Do you have an idea why makeAbsolute from RepoPath ignores ../? E.g.
> makeAbsolute /foo ../bar = /foo/bar, not /bar as one might expect.

I don't really remember, but the behaviour you're describing does
indeed seem bizarre.

Perhaps I had some expectation that p `isPrefixOf` makeAbsolute p r
... which should either be changed or documented.  Either this
function should reject ../bar (but accept ../foo/bar) or we should
change the expectation.

Thanks!

This is what I get for not writing QuickChecks! (hint hint)

Looks like makeAbsolute is used in make_absolute only. And make_absolute is used
in fix_flags only. So my change should not break anything.

Why is there so many filepath related modules? FileName, Darcs.FilePathUtils,
Darcs.RepoPath... It seems over complicated to me. E.g. for fix_flags we could
just use (///) from FileName.

But there is another weird behaviour: FileName.norm_path "/foo/bar" = "foo/bar".
Seems like a bug to me.

If we make norm_path work correctly with absolute paths we can remove
make_absolute and makeAbsolute functions.

Regards,
  Dmitry

> Why is there so many filepath related modules? FileName, Darcs.FilePathUtils,
> Darcs.RepoPath... It seems over complicated to me. E.g. for fix_flags we could
> just use (///) from FileName.

FileNames are normalised (see norm_path) and interacts with bytestring
by encoding paths into utf-8 -- I'm not 100% sure why we keep it
around.

Darcs.FilePathsUtils does a lot of darcs-specific work (i.e. path-fixing)

Darcs.RepoPath stuff is a recent work in progress.  We want to have
typed paths so that we know the difference between absolute, relative
and repo-relative (sub) paths.  I was hoping that we could extend the
reach of Darcs.RepoPath (for example so that we are always using
SubPaths in slurpies), but never got around to it.

Careful refactoring would be welcome indeed!

One thing that would be nice is to look into System.FilePath and see
if it would indeed be feasible to use it in darcs.  David isn't very
keen on this idea because of the possibility of breaking darcs.  At
the very least, we should aim for a common-ish interface, and if we
can prove that System.FilePath does exactly what we want, maybe freeze
the version that we use.

I now see that several of my emails to this bug never reached the tracker... hmmmm.

The following patch updated the status of issue950 to be resolved:

* resolve issue950: fix fix_filepath to work with --repodir (and add test demonstrating this).