Use a uniform output format for searching ../ #113

jakwings · 2017-10-14T11:31:15Z

closes #107, fixes #82 by the way (Yeah, I don't want to split this patch.)

jakwings · 2017-10-14T11:46:57Z

Ouch, now rebased after cargo fmt.
Again, rebased after fixing get_absolute_root_path() for Windows. :P

jakwings · 2017-10-14T12:43:05Z

@sharkdp Could you figure out what happened here: https://ci.appveyor.com/project/sharkdp/fd/build/1.0.108/job/agyctswxi2ymut4m#L202

reima

I've added some minor nitpicks as comments.

reima · 2017-10-14T12:36:35Z

src/output.rs

@@ -11,7 +11,11 @@ use std::os::unix::fs::PermissionsExt;
 use ansi_term;

 pub fn print_entry(base: &Path, entry: &PathBuf, config: &FdOptions) {
-    let path_full = base.join(entry);
+    let path_full = if *entry != PathBuf::new() {


Consider if !entry.as_os_str().is_empty() to avoid the allocation.

reima · 2017-10-14T12:43:09Z

tests/tests.rs

@@ -4,6 +4,21 @@ mod testenv;

 use testenv::TestEnv;

+fn get_absolute_root_path(env: &TestEnv) -> String {
+    let path = (*env)


No need to dereference env here. Rust does it automatically.

jakwings · 2017-10-14T13:00:36Z

tests/tests.rs

+    te.assert_output_subdirectory(
+        &abs_path,
+        &[
+            "--absolute-path",


This flag is not needed, will be removed.

sharkdp · 2017-10-14T13:20:17Z

@sharkdp Could you figure out what happened here: https://ci.appveyor.com/project/sharkdp/fd/build/1.0.108/job/agyctswxi2ymut4m#L202

Not sure ... does Windows treat ../.. correctly? Or should this be ..\..?

reima · 2017-10-14T14:13:41Z

Windows can work with / as a path separator just fine.

I think the problem is that symlinks work differently on Windows. On Unix, a symlink symlink -> one/two is like an alias for one/two, so symlink/.. is equivalent to one/two/.., which is one. But on Windows symlink\.. is whatever directory symlink resides in.

I'm not certain if or how we should fix this. In my experience symlinks are still very rarely used in Windows land, so personally I wouldn't bother too much with this (i.e. disable the test on Windows or special case it).

sharkdp · 2017-10-14T14:18:19Z

I think the problem is that symlinks work differently on Windows. On Unix, a symlink symlink -> one/two is like an alias for one/two, so symlink/.. is equivalent to one/two/.., which is one. But on Windows symlink.. is whatever directory symlink resides in.

Thanks, I didn't know that.

I'm not certain if or how we should fix this. In my experience symlinks are still very rarely used in Windows land, so personally I wouldn't bother too much with this (i.e. disable the test on Windows or special case it).

👍, in particular if we can add your explanation above as a comment why it's disabled/special-cased on Windows.

sharkdp · 2017-10-14T14:50:54Z

Playing around with this, I've noticed the following (I'm inside a test folder, in the symlink directory):

> echo "$PWD" 
/home/shark/fd-tests.4ULpgs5vXW/symlink

> fd '' "$PWD"
/home/shark/fd-tests.4ULpgs5vXW/one/two/../../symlink/C.Foo2
/home/shark/fd-tests.4ULpgs5vXW/one/two/../../symlink/c.foo
/home/shark/fd-tests.4ULpgs5vXW/one/two/../../symlink/three
/home/shark/fd-tests.4ULpgs5vXW/one/two/../../symlink/three/d.foo
/home/shark/fd-tests.4ULpgs5vXW/one/two/../../symlink/three/directory_foo

jakwings · 2017-10-14T15:21:48Z

Building on AppVeyor is really slow, can you enable cache for the "target" directory?

~~UPDATE: Ok, I modified appveyor.yml target\debug\deps -> Cargo.lock~~ (failed builds do not cache, wait)

jakwings · 2017-10-14T17:10:24Z

~~@reima Simply put, symlinks on Windows can refer to symlinks. They are references, not aliases to fixed real paths.~~

Corrected:

The path of the current working directory of a Unix process cannot contain symlinks.
The path of the current working directory of a Windows process can contain symlinks.
On Windows, symlinks are resolved after the ".." component.
On Unix, symlinks are resolved immediately as encountered.

closes #107, fixes #82 by the way

sharkdp · 2017-10-14T18:55:53Z

@iology Nice! Do you think there is anything we can do about #113 (comment) ?

jakwings · 2017-10-14T19:01:38Z

Nice! Do you think there is anything we can do about #113 (comment) ?

It should be fixed, but you can try to create more complicated examples.

Also please add/amend comments in the code if you see fit. Must have a nice holidy after this tiresome work. :P

sharkdp · 2017-10-15T08:24:02Z

src/main.rs

-        Err(_) => error("Error: could not get current directory."),
-    };
-    let current_dir = current_dir_buf.as_path();
+    let current_dir = Path::new(".");


Why did you change this?

The detection of the current directory being missing does not seem to work anymore.

So it seems that .is_dir() doesn't really check the existence of path. :(

Maybe Path::exists helps? Does this have to be changed?

.exists() doesn't help too, on both stable&nightly branches. Path::new("") is ok, but "." and ".." are not.

Oh, "" doesn't mean current directory. I will ask the rust team.

What was wrong with the old way (env::current_dir)?

env::current_dir() does check the existence. The returned path is resolved, so its value is used only when we don't have an absolute path.

if let Err(_) = env::current_dir() can help, but for root_dir, user still can pass in "." or "..". I hope there is an elegant way to check their existence.

Now I'm not sure if we should care the existence of "." and "..". Just let it crash?

sharkdp · 2017-10-15T08:28:12Z

src/main.rs

+    };
+
+    if path_display == PathDisplay::Absolute && root_dir_buf.is_relative() {
+        root_dir_buf = fshelper::absolute_path(root_dir_buf.as_path()).unwrap();


We should avoid using unwrap here:

> fd -a non/existing/relative/path thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Error { repr: Os { code: 2, message: "No such file or directory" } }', /checkout/src/libcore/result.rs:906:4 note: Run with `RUST_BACKTRACE=1` for a backtrace.

I think you forgot to add the pattern.

You are absolutely right 😄. So unwrap was only called because the current directory did not exist (the issue above).

sharkdp · 2017-10-15T08:35:28Z

tests/tests.rs

@@ -4,6 +4,20 @@ mod testenv;

 use testenv::TestEnv;

+fn get_absolute_root_path(env: &TestEnv) -> String {


sharkdp · 2017-10-15T13:01:45Z

Thank you very much! Very nice work.

* Fix path check * Fix full path matching * Allow more simple driver names in Windows tests * Factor out special is_dir() check for "." and ".."

reima reviewed Oct 14, 2017

View reviewed changes

jakwings commented Oct 14, 2017

View reviewed changes

Use a uniform output format for searching ../

61c8c59

closes #107, fixes #82 by the way

sharkdp self-requested a review October 15, 2017 08:08

sharkdp reviewed Oct 15, 2017

View reviewed changes

tests/tests.rs

@@ -4,6 +4,20 @@ mod testenv;

use testenv::TestEnv;

fn get_absolute_root_path(env: &TestEnv) -> String {

Copy link

Owner

sharkdp Oct 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Use .canonicalize() to check paths & Reword comments

b25bbaa

sharkdp merged commit 54d9dde into sharkdp:master Oct 15, 2017

sharkdp mentioned this pull request Oct 15, 2017

Implement the --exec flag #116

Merged

sharkdp pushed a commit that referenced this pull request Oct 18, 2017

Refactor path handling (fixes #113)

8d85deb

* Fix path check * Fix full path matching * Allow more simple driver names in Windows tests * Factor out special is_dir() check for "." and ".."

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a uniform output format for searching ../ #113

Use a uniform output format for searching ../ #113

jakwings commented Oct 14, 2017

jakwings commented Oct 14, 2017 •

edited

Loading

jakwings commented Oct 14, 2017

reima left a comment

reima Oct 14, 2017

reima Oct 14, 2017

jakwings Oct 14, 2017

sharkdp commented Oct 14, 2017

reima commented Oct 14, 2017

sharkdp commented Oct 14, 2017

sharkdp commented Oct 14, 2017

jakwings commented Oct 14, 2017 •

edited

Loading

jakwings commented Oct 14, 2017 •

edited

Loading

sharkdp commented Oct 14, 2017

jakwings commented Oct 14, 2017

sharkdp Oct 15, 2017

jakwings Oct 15, 2017

sharkdp Oct 15, 2017

jakwings Oct 15, 2017

jakwings Oct 15, 2017

sharkdp Oct 15, 2017 •

edited

Loading

jakwings Oct 15, 2017

jakwings Oct 15, 2017

jakwings Oct 15, 2017

sharkdp Oct 15, 2017

jakwings Oct 15, 2017

sharkdp Oct 15, 2017

sharkdp Oct 15, 2017

sharkdp commented Oct 15, 2017

		@@ -4,6 +4,20 @@ mod testenv;

		use testenv::TestEnv;

		fn get_absolute_root_path(env: &TestEnv) -> String {

Use a uniform output format for searching ../ #113

Use a uniform output format for searching ../ #113

Conversation

jakwings commented Oct 14, 2017

jakwings commented Oct 14, 2017 • edited Loading

jakwings commented Oct 14, 2017

reima left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharkdp commented Oct 14, 2017

reima commented Oct 14, 2017

sharkdp commented Oct 14, 2017

sharkdp commented Oct 14, 2017

jakwings commented Oct 14, 2017 • edited Loading

jakwings commented Oct 14, 2017 • edited Loading

sharkdp commented Oct 14, 2017

jakwings commented Oct 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharkdp Oct 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharkdp commented Oct 15, 2017

jakwings commented Oct 14, 2017 •

edited

Loading

jakwings commented Oct 14, 2017 •

edited

Loading

jakwings commented Oct 14, 2017 •

edited

Loading

sharkdp Oct 15, 2017 •

edited

Loading