When running "send -o somepatch.dpatch" I would always expect the patch
to go into the current directory.  But when the --repos is used, the
patch is placed in the directory the repository is in instead.  So if I
run 'darcs send -o somepatch.dpatch --repo=my-repo' the patch will be
created as my-repo/somepatch.dpatch.

I'm guessing the code that is causing issue76 is also causing this.
Namely, we try to changing into the repository directory before doing
our operations.  In this case, it means the patch is created in the
wrong directory.  In the case of issue76, it means it tries to change
the current directory to a url.

I dare say I agree this is a bug, albeit a minor one.  I wonder if there is a
general phenomenon of slight behavioural bugs occurring because of this changing
directories business and if there is one thing that will fix them all.

The 'proper' solution may be to avoid changing directories, and instead
always passing a full path to the repository for all commands.
Unfortunately this probably would have to change a lot of code (unless
there is a good way to store the path in some data structure, but I have
a feeling we mostly just pass around strings for this sort of thing).

A different solution I can think of is to save the directory we started
in, and use that path explicitly when running things like 'send -o'.  I
took a quick look at that when I filed the bug, but didn't see any
obvious ways to do so.  Sometimes the code is quite a few layers down
from where we started.

I'll try to look more into both of these options when I have some time.

On Thu, Mar 08, 2007 at 04:08:53PM +0000, Zachary P. Landau wrote:
> A different solution I can think of is to save the directory we started
> in, and use that path explicitly when running things like 'send -o'.  I
> took a quick look at that when I filed the bug, but didn't see any
> obvious ways to do so.  Sometimes the code is quite a few layers down
> from where we started.

There's some fix_filepaths code that does essentially this.  (grep for
fix_file, as I don't remember the function precisely...)  It probably could
be cleaned up (and I think there are bugs where it's misused), but as
there's already a framework, I'd say the right idea is to use it properly
(and alas, *everywhere*).  fix_filepaths should do the right thing when
given either an absolute path or a relative path (which it should
simplify).

We *could* have always used absolute paths in darcs' code, but that's quite
a bit of work.  The new Repository framework would move us closer to that
ideal (since a Repository stores the directory it's in, and the code is
intended to be safe to call from any directory), but I'm not sure when
we'll "arrive", or that we ever will.
-- 
David Roundy
Department of Physics
Oregon State University

Just some background, assuming I'm not mistaken:

fix_filepaths - converts paths to be relative to the repo root.  Even absolute
paths are converted.  The point is to make everything uniform and consistent.

unfix_filepaths - converts them back to something more like what the user typed in

So if we are in /foo/repo/subdir:
 fix etc => subdir/etc
 fix /foo/repo/subdir/etc => subdir/etc
 unfix subdir/etc => etc

For the record, we are using the fix_filepath in the right places.  Only problem
is the interaction between fix and --repodir.  Basically, the problem is that
--repodir sets the repo root.  For most paths (try darcs diff --repodir), the
interaction is probably correct.  However, for some specific flags, (--logfile,
--output) the user is not referring to files in the repository but files in her
current directory.

The fix/unfix code needs to be made a little bit smarter to account for
--repodir and such exceptional flags.  Still interested in looking into this,
Zachary?

I'm certainly willing to look into fixing it.  Whether or not I'm able is
another question.  I've spent some time looking through the fix_filepaths and
related code, and there are quite a number of spots where I'm not sure why we
are doing what we are doing.

But (and correct me if I'm wrong) it looks like the spot I may be interested in
is  'consider_running' in Commands.lhs.  Right now we call 'fix_flag' on all the
flags to adjust path information, but that is AFTER we have already changed into
the new directory, potentially losing information about where the user wanted an
output file to end up.  I think we want a call very similar to the map on
fix_flag, but that is passed the path we started at, before the call to
command_prereq may have changed it.  This second version of fix_flag would match
Output, LogFile, and whatever else refers to non-repository files, adjusting
those to the original directory instead of the repository directory.

If any of that seems wrong or incomprehensible, let me know.

Just a short note while it's on my mind. It's not only flag
arguments that can be non-repo. 'darcs pull ../source --repo
./target' will (in error?) pull from ./target/../source.

Please have a look at the similar issue427.  

Zachary: That sounds about right, although one thing that might be tricky is
what the 'unfix' counterpart gets as arguments.  Perhaps the second fix should
convert to absolute paths.  Not sure how the second unfix would work there, though.

What would be the purpose of unfixing in this case?  Is it used when
trying to display paths to the user after the path has already been
fixed?  If that's the case, unfixing is probably less important for
'filesystem' paths than it is for 'repository' paths.

Unfixing the absolute path may work by being given the original working
directory and stripping that part of the path out again.  But I'm not
sure if it will be easy to pass that information to the part of the code
where we'd need to call unfix (especially given that I'm still uncertain
as to when we'd need unfix).

> What would be the purpose of unfixing in this case?  Is it used when
> trying to display paths to the user after the path has already been
> fixed?  If that's the case, unfixing is probably less important for
> 'filesystem' paths than it is for 'repository' paths.

Good point.  I was afraid that the reporting would get 'heavy' without
an unfix, but then again, if we unfix and the user is both passing in
a --repodir AND in a repository, the reported path would be ambiguous.
 So even from a UI point of view, it actually makes more sense not to
unfix the 'filesystem' paths.

On 7/23/07, Eric Kow <bugs@darcs.net> wrote:

> Good point.  I was afraid that the reporting would get 'heavy' without
> an unfix, but then again, if we unfix and the user is both passing in
> a --repodir AND in a repository, the reported path would be ambiguous.

Whoops!  It gets ambiguous the minute the user passes in --repodir,
regardless of whether or not s/he is in a repository.
-- 
Eric Kow                     http://www.loria.fr/~kow
PGP Key ID: 08AC04F9         Merci de corriger mon français.

[CC-ing darcs-devel]

I'm just brain storming a little here.

We have absolute paths, relative paths, and repo-root-relative
paths. There is no "file system" way to express a
repo-root-relative path.

What if darcs set the environment variable $rrr to the repo
root, be it ../../some/where, /exactly/here, or
foo://way.over.here? Then you could write rrr paths as
$rrr/src/foo.c. Hm, no you couldn't since the shell expands the
variable before it is set.

What if the user could to type rrr paths as :src/foo.c or
//src/foo.c? File path auto completion won't work anyway.

What I wish would work is

  darcs changes --repo ../here ../here/src/foo.c

since that's who I'd expect it to work (and do the same as the
proposed

  darcs changes --repo ../here :src/foo.c

).

Am I on to something or just on something? ;-)

A lot of talk for what seems like a small patch.  Can someone take a look at
this and see if it makes sense to them?  It seems to me that all of the flags
that were in fix_flag should be made absolute, because all of them reference
'filesystem' paths rather than repository paths.  Someone yell at me if I am wrong.

I hope to write some tests for these, but my brain just isn't working well
tonight, so I'm gonna take a stab at that a bit later.  But I figured I'd throw
the patch here first so people can point out more ways that my brain isn't
working properly.

Attachments

• makes-non_repository-paths-in-darcsflags-absolute-issue427.dpatch

Hey, moving fast on this! :-)

On Tue, Jul 24, 2007 at 01:41:43 -0000, Zachary P. Landau wrote:
> A lot of talk for what seems like a small patch.  Can someone take a look at
> this and see if it makes sense to them?  It seems to me that all of the flags
> that were in fix_flag should be made absolute, because all of them reference
> 'filesystem' paths rather than repository paths.  Someone yell at me if I am wrong.

The naming might be awkward, but that's my fault (I think).  The 'idea'
behind fix_maybe_absolute was that the path being fixed might be
absolute... which sounds rather silly come to think of it.  I'm not sure
why I felt it had to be made clear.  In that respect fix_maybe_relative
wouldn't make sense.  Maybe you could find a better name for both
functions so that future darcs hackers don't get misled :-)

I haven't thought about the actual contentful part of the patch yet.

On Mon, Jul 23, 2007 at 22:33:56 +0200, Tommy Pettersson wrote:
> since that's who I'd expect it to work (and do the same as the
> proposed
> 
>   darcs changes --repo ../here :src/foo.c

What's interesting about this idea is that it would provide an
unambiguous way to refer to a path; the user knows exactly what he is
going to get.

That said, I'm not sure anybody would actually use it.  Hmm...

Not sure what else to make of it.  Maybe this is the kind of thing
that fancy shell completion tools should do and not darcs?

And since we're on the subject of random FilePath thoughts.  Would it be
useful to distinguish between such paths in our types?  Are there errors
we could avoid with this?

data DarcsPath = RepoPath FilePath
               | AbsPath  FilePath
               | UserPath FilePath

Or would that just make things clunky?

On Tue, Jul 24, 2007 at 09:12:55PM +0200, Eric Y. Kow wrote:
> And since we're on the subject of random FilePath thoughts.  Would it be
> useful to distinguish between such paths in our types?  Are there errors
> we could avoid with this?
> 
> data DarcsPath = RepoPath FilePath
>                | AbsPath  FilePath
>                | UserPath FilePath
> 
> Or would that just make things clunky?

I think that's worth considering.  Possibly we could even specify different
types:

newtype RepoPath = RepoPath FilePath
newtype AbsolutePath = AbsolutePath FilePath
newtype RelativePath = RelativePath FilePath

data DarcsPath = RepoDP RepoPath
               | AbsoluteDP AbsolutePath
               | RelativeDP RelativePath

Then we could make the typesystem ensure that certain paths are always
absolute (for instance, any path that is passed to a lazy IO function, so
that it won't get messed up if the current directory changes).

I'd certainly appreciate having this sort of documentation of which sort of
path is expected in certain functions.  It'd also mean that we could avoid
redundant calls converting paths to absolute by putting the requirement
into the type.

class FilePath p where
  toFilePath :: p -> FilePath
instance FilePath RepoPath
instance FilePath AbsolutePath
instance FilePath RelativePath
instance FilePath DarcsPath
instance FilePath FilePath

Then code that doesn't care what sort of FilePath is given could just be
polymorphic, which sounds like a good plan.

We also might want to distinguish possibly-remote filepaths.
-- 
David Roundy
Department of Physics
Oregon State University

In the ensuing discussion (which should be continued on darcs-devel), we forgot
to mention that is is now resolved in unstable with:

Tue Jul 24 03:34:25 CEST 2007  Zachary P. Landau <kapheine@divineinvasion.net>
  * Makes non-repository paths in DarcsFlags absolute [issue427].