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

`fish_realpath` should behave like the `realpath` command when invoked with no options #3400

Closed
krader1961 opened this Issue Sep 22, 2016 · 6 comments

Comments

Projects
None yet
3 participants
@krader1961
Contributor

krader1961 commented Sep 22, 2016

The intent when implementing fish_realpath was that is would behave like both the BSD and GNU realpath command when invoked with no options. Which means that the all but the final path component must exist and be resolvable. I expected the libc realpath implementation (e.g., "man 3 realpath") to provide that baseline behavior. That expectation was incorrect. See, for example, PR #3374, which added another workaround for a missing realpath command but did not address the problem with the fish implementation. This issue is being opened to ensure the fish_realpath builtin is fixed to provide the behavior that is expected by 99.9% of realpath invocations.

@krader1961 krader1961 added the bug label Sep 22, 2016

@krader1961 krader1961 added this to the fish-future milestone Sep 22, 2016

@krader1961 krader1961 self-assigned this Sep 22, 2016

@zanchey zanchey modified the milestones: fish 2.4.0, fish-future Oct 2, 2016

@zanchey

This comment has been minimized.

Show comment
Hide comment
@zanchey

zanchey Oct 2, 2016

Member

I'd like to get this squared away before we release 2.4.0 in order not to have to change its behaviour later.

Member

zanchey commented Oct 2, 2016

I'd like to get this squared away before we release 2.4.0 in order not to have to change its behaviour later.

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Oct 2, 2016

Member

Why is just this one builtin using fish_* nomenclature? How about if you improve this just rename that builtin to realpath and make a fish_realpath.fish compatibility script. The normal way to discern is command foo vs. builtin foo.

Member

floam commented Oct 2, 2016

Why is just this one builtin using fish_* nomenclature? How about if you improve this just rename that builtin to realpath and make a fish_realpath.fish compatibility script. The normal way to discern is command foo vs. builtin foo.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Oct 3, 2016

Contributor

Okay, I've got an implementation that seems to mirror GNU realpath. We have two internal implementations. Which one is used depends on the autoconf HAVE_REALPATH_NULL value so I need to copy the fix to the other implementation and update unit tests. Should be ready by tomorrow evening if not this evening.

Why is just this one builtin using fish_* nomenclature?

I honestly don't know other than everyone seemed to think it was a good idea at the time it was discussed and implemented. So I'm also going to fix the naming per your suggestion.

P.S., The reason this bug exists is I blindly assumed the wrealpath() function that was added four years ago implemented the behavior we wanted. But it was previously only used in one place, expand_home_directory(), where the rather odd behavior of the libc realpath(3) function isn't an issue.

Contributor

krader1961 commented Oct 3, 2016

Okay, I've got an implementation that seems to mirror GNU realpath. We have two internal implementations. Which one is used depends on the autoconf HAVE_REALPATH_NULL value so I need to copy the fix to the other implementation and update unit tests. Should be ready by tomorrow evening if not this evening.

Why is just this one builtin using fish_* nomenclature?

I honestly don't know other than everyone seemed to think it was a good idea at the time it was discussed and implemented. So I'm also going to fix the naming per your suggestion.

P.S., The reason this bug exists is I blindly assumed the wrealpath() function that was added four years ago implemented the behavior we wanted. But it was previously only used in one place, expand_home_directory(), where the rather odd behavior of the libc realpath(3) function isn't an issue.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Oct 4, 2016

Contributor

Can anyone explain to me why we have the autoconf HAVE_REALPATH_NULL symbol today? It was introduced ten years ago by @axel in commit 06fd1aa. That distinction between realpath(3) implementations made sense ten years ago. Today it appears that every implementation we run on supports passing NULL for the resolved_path argument.

The reason I ask is that I am reticent to duplicate the fix I mentioned above into the code for the none #ifdef HAVE_REALPATH_NULL version. I've made a couple of attempts to abstract the behavior into a common function but that introduces as much complexity as it eliminates. I would rather eliminate the code path for systems that do not allow NULL for the second argument as being too old and broken to support.

Also, of course, I can merge my change that fixes the fish realpath builtin behavior on systems that do support NULL for the second argument. While not fixing it for systems that do support NULL as the second argument for realpath(3).

Contributor

krader1961 commented Oct 4, 2016

Can anyone explain to me why we have the autoconf HAVE_REALPATH_NULL symbol today? It was introduced ten years ago by @axel in commit 06fd1aa. That distinction between realpath(3) implementations made sense ten years ago. Today it appears that every implementation we run on supports passing NULL for the resolved_path argument.

The reason I ask is that I am reticent to duplicate the fix I mentioned above into the code for the none #ifdef HAVE_REALPATH_NULL version. I've made a couple of attempts to abstract the behavior into a common function but that introduces as much complexity as it eliminates. I would rather eliminate the code path for systems that do not allow NULL for the second argument as being too old and broken to support.

Also, of course, I can merge my change that fixes the fish realpath builtin behavior on systems that do support NULL for the second argument. While not fixing it for systems that do support NULL as the second argument for realpath(3).

@zanchey

This comment has been minimized.

Show comment
Hide comment
@zanchey

zanchey Oct 4, 2016

Member

Can anyone explain to me why we have the autoconf HAVE_REALPATH_NULL symbol today?

Nope! The behaviour was specified in POSIX.1-2008 so I think we can nuke it.

Member

zanchey commented Oct 4, 2016

Can anyone explain to me why we have the autoconf HAVE_REALPATH_NULL symbol today?

Nope! The behaviour was specified in POSIX.1-2008 so I think we can nuke it.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Oct 5, 2016

Contributor

Fixed by commits f7f39b8 and 7fd3079 (I mis-formatted the commit message which is why this wasn't automatically closed).

Contributor

krader1961 commented Oct 5, 2016

Fixed by commits f7f39b8 and 7fd3079 (I mis-formatted the commit message which is why this wasn't automatically closed).

@krader1961 krader1961 closed this Oct 5, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment