Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolve relative paths in complete #7992

Closed
wants to merge 3 commits into from

Conversation

Caroleq
Copy link
Contributor

@Caroleq Caroleq commented May 11, 2021

Description

PR resolves issue that completions for paths don't work for relative paths.

Reason for a bug is that path_get_path does not return full path as it is told in the function description, but returns unmodified cmd or path + cmd where path is taken from PATH e.g. for ../../dir1/exec1 path_get_path will return ../../dir1/exec1.

From what I checked this function is used by (I skip functions which don't store path i.e. out_path is null and parse_cmd_string):

  • populate_plain_process passes value from path_get_path to exec function. Relative path should not be changed to absolute here because e.g. whole command path for a process is later seen in process info (this can be checked e.g. with ps -ef on Linux)
  • builtin_command uses value from path_get_path to print command that would be executed - I would also rather leave it as it is now

My proposal to solve it is to pass additional flag to path_get_path indicating if path should be returned as absolute. parse_cmd_string will always return absolute path which can be later matched with path completion if a one was defined. I didn't add function converting path to absolute in section checking if cmd is located in $PATH, because paths in $PATH should be absolute already and cmd doesn't have any slashes (otherwise it would be handled at the beginning of the function).

Fixes issue #6001

TODOs:

  • Changes to fish usage are reflected in user documentation/manpages.
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.rst

CHANGELOG.rst Outdated Show resolved Hide resolved
@@ -848,7 +848,7 @@ end_execution_reason_t parse_execution_context_t::populate_plain_process(
wcstring path_to_external_command;
if (process_type == process_type_t::external || process_type == process_type_t::exec) {
// Determine the actual command. This may be an implicit cd.
bool has_command = path_get_path(cmd, &path_to_external_command, parser->vars());
bool has_command = path_get_path(cmd, &path_to_external_command, parser->vars(), true);
Copy link
Member

Choose a reason for hiding this comment

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

Why does this use the absolute path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦 It was meant be be false everywhere apart from parse_cmd_string. I put it there by mistake and didn't check it after. Sorry.

@@ -103,7 +103,7 @@ maybe_t<int> builtin_command(parser_t &parser, io_streams_t &streams, const wcha
}
} else { // Either find_path explicitly or just quiet.
wcstring path;
if (path_get_path(command_name, &path, parser.vars())) {
if (path_get_path(command_name, &path, parser.vars(), true)) {
Copy link
Member

Choose a reason for hiding this comment

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

Why does this use the absolute path? Currently

cd /usr/bin
command -s ./env

prints "./env". I don't know if that's of much use, but it doesn't appear to be obviously wrong either?

Copy link
Member

@krobelus krobelus May 12, 2021

Choose a reason for hiding this comment

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

Yeah, I'd keep the current behavior (as written in the PR description) unless there is a reason for changing this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦 It was meant be be false everywhere apart from parse_cmd_string. I put it there by mistake and didn't check it after. Sorry.

src/path.h Outdated
///
/// Returns:
/// false if the command can not be found else true. The result
/// should be freed with free().
bool path_get_path(const wcstring &cmd, wcstring *out_path, const environment_t &vars);
bool path_get_path(const wcstring &cmd, wcstring *out_path, const environment_t &vars, bool absolute);
Copy link
Member

Choose a reason for hiding this comment

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

This is a case where I'd use a default argument - bool absolute = false and just leave it out in the spots you don't need it.

Then, where you do, you can comment it as

path_get_path(str, path, vars, /* absolute */ true)

Alternatively we could require those parts to do the wrealpath themselves. If it's only used in one or two places (and I count one where I'd agree that we need it) that might be preferable.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think we only want this in one place, so the caller should call wrealpath.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, this makes more sense. I just removed information from path_get_path that it returns full path and call wrealpath in parse_cmd_string.

@faho faho added this to the fish 3.3.0 milestone May 12, 2021
@zanchey zanchey linked an issue May 16, 2021 that may be closed by this pull request
@krobelus
Copy link
Member

Merged as 31f3c16 thanks! I tweaked the patch to avoid the realpath syscall in the common case (when we already have an absolute path).
Technically we should always resolve the path when complete -p is used, in case the commandline has an absolute but non-canonical path but I don't know if that matters.

@krobelus krobelus closed this May 16, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

completion for path should trigger for relative paths
3 participants