This is the mail archive of the cygwin-patches@cygwin.com mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [Patch]: chdir


Corinna Vinschen wrote:
> 
> On May  5 00:42, Pierre A. Humblet wrote:
> > At 08:25 PM 5/4/2004 -0400, Christopher Faylor wrote:
> > >Another thing occurs to me -- maybe it would simplify things just to
> > >store a patch_conv structure on the cygheap rather than use a separate
> > >cwdstuff structure.  It would take up more space on the cygheap but I
> > >think it would probably be cleaner in general.
> >
> > Maybe, but the simplicity isn't obvious to me. For example a path_conv
> > would have a win32 path and a normalized_path, but the normalized_path
> > could start with a drive. That's not what cwdstuff::posix currently
> > expects, because it might confuse Unix programs.
> 
> Erm... so what's the difference to what your patch is doing now?
> You're using path.normalized_path unchanged which means, it could
> start with a drive, too.

Erm, see below.

> > The main questions are whether using the normalized_path below is valid
> > and why pcheck_case == PCHECK_RELAXED is needed.
> >
> > -  else if ((!path.has_symlinks () && strpbrk (dir, ":\\") == NULL
> > -           && pcheck_case == PCHECK_RELAXED) || isvirtual_dev (devn))
> > -    cygheap->cwd.set (native_dir, dir);
> > +  else if (!isdrive (path.normalized_path)
> > +           && pcheck_case == PCHECK_RELAXED)
> > +    cygheap->cwd.set (native_dir, path.normalized_path);
> >    else
> >      cygheap->cwd.set (native_dir, NULL);
> 
> path.normalized_path could contain a drive letter so what your above
> change does is to let a drive letter through, while the original
> implementation doesn't.  That's what the `strpbrk (dir, ":\\")' is for.

Yes, and that's also what "isdrive (path.normalized_path)" is doing.
Except it's testing specifically for a drive, and not for a random :
that could happen with managed mounts.

> If we don't want to touch that (which is IMHO a good idea for 1.5.10 at
> least), we have to keep the strpbrk in, otherwise your patch looks ok to
> me.  The advantage is that we could apply the below patch as well which
> would simplify the dot and space stripping.
> 
> Uh, yes, the PCHECK_RELAXED test *is* needed.  Neither the path in "dir"
> in the old implementation, nor path.normalized_path are case adjusted.
> Using them would break the PCHECK_ADJUST case.

OK, but it should be fine with PCHECK_STRICT, shouldn't it?
So a better test would be pcheck_case != PCHECK_ADJUST.
Erm, what's the role of PCHECK_ADJUST in a chdir()? What could
be broken? 

I noticed that if we have a mount \\someshare\root  => /abc
then "cd //someshare/root" will result in the Posix working directory
being //someshare/root, not /abc. Do we care? If we do,
the test for a unc path should be added to the isdrive() test.
By the way, 1.5.9 also returns //someshare/root in that case.
However if there is a symlink xxxx pointing to \\someshare\root 
then "cd xxxx" yields the Posix working directory being /abc.

To simplify, why not call cygheap->cwd.set (native_dir, NULL)
all the time, except in the virtual case?
 
Pierre


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]