#475 - Allow changing current working directory of fd #509

hajdamak · 2019-11-20T10:02:11Z

Hi, this PR solves #475 by implementing --base-directory option and using std::env::set_current_dir. Feedback appreciated.

sharkdp · 2019-11-24T23:02:27Z

Looks good, thank you.

Can we please implement a simple test for this option?

hajdamak · 2019-12-10T10:45:54Z

Sure. Pull request updated.
Is this OK?

hajdamak · 2019-12-18T16:07:51Z

Is this fine to merge it?

sharkdp

Thank you very much. The code looks good.

Could you please elaborate a little bit on the interplay between this new option and --search-path and --full-path?

sharkdp · 2019-12-19T20:57:16Z

tests/tests.rs

+        two/three/directory_foo",
+    );
+
+    // Ignore base directory when aboslute path is used


typo: absolute

sharkdp · 2019-12-19T20:59:07Z

src/main.rs

+        let basedir = Path::new(base_directory);
+        if !fshelper::is_dir(basedir) {
+            print_error_and_exit!(
+                "'{}' passed as '--base-directory' value is not a directory.",


Maybe:

The '--base-directory' path ('{}') is not a directory.

sharkdp · 2019-12-19T21:00:59Z

src/main.rs

+        }
+        if let Err(e) = env::set_current_dir(basedir) {
+            print_error_and_exit!(
+                "Cannot set '{}' as current working directory: {}", 


Minor rewording:

Could not set '{}' as the current working directory: {}

hajdamak · 2019-12-20T13:20:28Z

My current understanding is as follows:
--base-directory only applies if positional path argument is relative or is not passed (which means relative .). Changing current working directory only influences relative paths used, so if path is absolute --base-directory does not change anything.
When --base-directory is used with relative path it just means that path is resovled relative to the directory passed as --base-directory.
--search-path works the same as positional path argument so everything above applies.
--full-path is not influenced by --base-directory in any way, it just causes pattern to be applied on full path (instead of file name) for all the files that reside in path or --search-path.
So in summary --base-directory influences how relative paths passed ass paths or --search-paths are resolved.
When mutliple paths or --serach-paths are passed, --base-directory only applies to relative ones.
Does this make sense?

sharkdp · 2019-12-20T18:36:36Z

--base-directory only applies if positional path argument is relative or is not passed (which means relative .). Changing current working directory only influences relative paths used, so if path is absolute --base-directory does not change anything.

👍

When --base-directory is used with relative path it just means that path is resovled relative to the directory passed as --base-directory.

right.

--search-path works the same as positional path argument so everything above applies.

makes sense.

--full-path is not influenced by --base-directory in any way, it just causes pattern to be applied on full path (instead of file name) for all the files that reside in path or --search-path.

Yes, right. Because --full-path always searches on the absolute path.

So in summary --base-directory influences how relative paths passed ass paths or --search-paths are resolved.

There is one more thing that is potentially very confusing. Consider the following (let's say we are inside the fd repository directory):

❯ fd input
src/exec/input.rs

❯ fd input src/
src/exec/input.rs

❯ fd input --base-directory=src/
exec/input.rs

❯ fd input src/ --base-directory=src/
[fd error]: 'src/' is not a directory.

I guess this happens because fd is now trying to resolve the search path (src/) relative to the base directory (also src/), resulting in a lookup for src/src.

I guess we should wait with changing the directory until we have resolved other (relative) path arguments.

We should also add a test for this.

hajdamak · 2019-12-20T18:49:11Z

I guess we should wait with changing the directory until we have resolved other (relative) path arguments.

The question is whether we want to keep path relative to current working directory where fd was executed or the passed --base-directory. I would rather stay with latter because it seems to have more use cases. In that case only error message should be changed to make it less confusing, for example also show how provided path is resolved relative to --base-directory.
What do you think?

sharkdp · 2019-12-20T19:18:07Z

The question is whether we want to keep path relative to current working directory where fd was executed or the passed --base-directory.

I think it should be relative to the CWD of the terminal (i.e. where fd was executed). This is how paths are passed to programs. This is how the shell completion in terminals works. Everything else is confusing, IMO.

hajdamak · 2019-12-21T08:50:00Z

Yeah, I get your point. Especially that docs in path state:

The directory where the filesystem search is rooted (optional). If omitted, search the current working directory.

Such an approach in practice means that --base-directory only works if no path or --search-path argument is provided.

sharkdp · 2019-12-21T09:02:24Z

The directory where the filesystem search is rooted (optional). If omitted, search the current working directory.

Such an approach in practice means that --base-directory only works if no path or --search-path argument is provided.

Oh.. you mean because we would search the <base-directory> instead of the current directory if no path is supplied?

I think the following is a reasonable use case:

fd --base-directory=<base-path> pattern <base-path>/<search-path-below-base>

The <base-path>/<search-path-below-base> path would be searched, but all results are relative to <base-path>.

I think we should also look into what other programs do. Like git with its -C <path> option.

hajdamak · 2019-12-21T10:08:28Z

Oh.. you mean because we would search the base-directory instead of the current directory if no path is supplied?

Yeap. And if path is supplied --base-directory will have no effect because paths are resolved relative of CWD of fd instead of --base-directory.

I think the following is a reasonable use case:
fd --base-directory=base-path pattern base-path/search-path-below-base
The base-path/search-path-below-base path would be searched, but all results are relative to base-path.

This is the use case I had in mind when opted for making paths resolved relative to --base-directory but with following usage:
fd --base-directory=<base-path> pattern <search-path-below-base>

I think we should also look into what other programs do. Like git with its -C option.

In Git using -C causes other relative paths passed as command parameters to be resolved relative to -C directory. This is what man states in -C section:

This option affects options that expect path name like --git-dir and --work-tree in that their interpretations of the path names would be made relative to the working directory caused by the -C option. For example the following invocations are equivalent:
git --git-dir=a.git --work-tree=b -C c status
git --git-dir=c/a.git --work-tree=c/b status

I still think having --base-directory just change CWD of the process with all the consequences that it has on passed relative paths is more simple and straightforward approach though confusing at first. In docs we could just write that it changes CWD so fd runs as if executed in different directory and passed paths are resolved relative to it.

sharkdp · 2019-12-22T13:58:57Z

In Git using -C causes other relative paths passed as command parameters to be resolved relative to -C directory. This is what man states in -C section:

Ok, then let's do it the same way and keep the logic as is.

I still think having --base-directory just change CWD of the process with all the consequences that it has on passed relative paths is more simple and straightforward approach though confusing at first.

👍

In docs we could just write that it changes CWD so fd runs as if executed in different directory and passed paths are resolved relative to it.

Sounds great. Looking forward to merging this, if you could extend the documentation in that respect.

hajdamak · 2019-12-23T14:12:48Z

Great. Docs updated. Are they OK from your POV?

sharkdp · 2019-12-23T14:42:50Z

Thank you very much. I might change the wording a little bit, but apart from that, it looks great.

hajdamak · 2019-12-23T14:46:23Z

Thanks a lot for your help and merge.

hajdamak mentioned this pull request Nov 20, 2019

Feature request: Option to run fd in different directory than pwd #475

Closed

hajdamak force-pushed the 475-base-directory branch from 7f1f989 to 1e81907 Compare December 10, 2019 10:43

sharkdp reviewed Dec 19, 2019

View reviewed changes

hajdamak force-pushed the 475-base-directory branch from 1e81907 to 2f8c466 Compare December 20, 2019 14:13

Allow changing current working directory of fd

7204c85

hajdamak force-pushed the 475-base-directory branch from 2f8c466 to 7204c85 Compare December 23, 2019 14:05

sharkdp merged commit fb205f5 into sharkdp:master Dec 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#475 - Allow changing current working directory of fd #509

#475 - Allow changing current working directory of fd #509

hajdamak commented Nov 20, 2019

sharkdp commented Nov 24, 2019

hajdamak commented Dec 10, 2019

hajdamak commented Dec 18, 2019

sharkdp left a comment

sharkdp Dec 19, 2019

hajdamak Dec 20, 2019

sharkdp Dec 19, 2019

hajdamak Dec 20, 2019

sharkdp Dec 19, 2019

hajdamak Dec 20, 2019

hajdamak commented Dec 20, 2019

sharkdp commented Dec 20, 2019

hajdamak commented Dec 20, 2019

sharkdp commented Dec 20, 2019

hajdamak commented Dec 21, 2019 •

edited

Loading

sharkdp commented Dec 21, 2019

hajdamak commented Dec 21, 2019

sharkdp commented Dec 22, 2019

hajdamak commented Dec 23, 2019 •

edited

Loading

sharkdp commented Dec 23, 2019

hajdamak commented Dec 23, 2019

#475 - Allow changing current working directory of fd #509

#475 - Allow changing current working directory of fd #509

Conversation

hajdamak commented Nov 20, 2019

sharkdp commented Nov 24, 2019

hajdamak commented Dec 10, 2019

hajdamak commented Dec 18, 2019

sharkdp left a comment

Choose a reason for hiding this comment

sharkdp Dec 19, 2019

Choose a reason for hiding this comment

hajdamak Dec 20, 2019

Choose a reason for hiding this comment

sharkdp Dec 19, 2019

Choose a reason for hiding this comment

hajdamak Dec 20, 2019

Choose a reason for hiding this comment

sharkdp Dec 19, 2019

Choose a reason for hiding this comment

hajdamak Dec 20, 2019

Choose a reason for hiding this comment

hajdamak commented Dec 20, 2019

sharkdp commented Dec 20, 2019

hajdamak commented Dec 20, 2019

sharkdp commented Dec 20, 2019

hajdamak commented Dec 21, 2019 • edited Loading

sharkdp commented Dec 21, 2019

hajdamak commented Dec 21, 2019

sharkdp commented Dec 22, 2019

hajdamak commented Dec 23, 2019 • edited Loading

sharkdp commented Dec 23, 2019

hajdamak commented Dec 23, 2019

hajdamak commented Dec 21, 2019 •

edited

Loading

hajdamak commented Dec 23, 2019 •

edited

Loading