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

Use 'grealpath' if installed for realpath fallback #3374

Merged
merged 2 commits into from Sep 18, 2016

Conversation

Projects
None yet
3 participants
@floam
Member

floam commented Sep 12, 2016

Description

Per #3370, our realpath builtin/wrapper function will not use GNU realpath if installed on BSD with a 'g' prefix.

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Sep 12, 2016

Member

The idea is probably correct, though I'd actually define the function here instead of going via alias - all the stuff that does is irrelevant here.

Member

faho commented Sep 12, 2016

The idea is probably correct, though I'd actually define the function here instead of going via alias - all the stuff that does is irrelevant here.

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Sep 13, 2016

Member

II thought alias was just syntax to define a wrapped function. What else does it do?

Member

floam commented Sep 13, 2016

II thought alias was just syntax to define a wrapped function. What else does it do?

Show outdated Hide outdated share/functions/realpath.fish
function realpath --description 'fallback realpath implementation'
builtin fish_realpath $argv[-1]
end
if not command -v realpath > /dev/null

This comment has been minimized.

@krader1961

krader1961 Sep 13, 2016

Contributor

FWIW, I think we should be using command -s rather than command -v in the scripts we ship since our command(1) man page documents -s as a first class flag and -v as being recognized only for compatibility with other implementations.

@krader1961

krader1961 Sep 13, 2016

Contributor

FWIW, I think we should be using command -s rather than command -v in the scripts we ship since our command(1) man page documents -s as a first class flag and -v as being recognized only for compatibility with other implementations.

Show outdated Hide outdated share/functions/realpath.fish
end
if not command -v realpath > /dev/null
if command -v grealpath > /dev/null
alias realpath=grealpath

This comment has been minimized.

@krader1961

krader1961 Sep 13, 2016

Contributor

As @faho pointed out I also agree this should explicitly define the function rather than using alias. Look at the description for the alias function.

@krader1961

krader1961 Sep 13, 2016

Contributor

As @faho pointed out I also agree this should explicitly define the function rather than using alias. Look at the description for the alias function.

This comment has been minimized.

@floam

floam Sep 13, 2016

Member

Right - the alias builtin implements aliases by making a function for us - I'd be making a function anyhow. The syntax is nicer. The description it adds to aliases does suck - they're nice and I don't see why we'd discourage them - that they become functions is an implementation detail.

@floam

floam Sep 13, 2016

Member

Right - the alias builtin implements aliases by making a function for us - I'd be making a function anyhow. The syntax is nicer. The description it adds to aliases does suck - they're nice and I don't see why we'd discourage them - that they become functions is an implementation detail.

This comment has been minimized.

@floam

floam Sep 13, 2016

Member

Oh - alias is not a builtin. I still like aliases in fish - but I guess that's more heavyweight how they get constructed than I assumed, I'll change this.

@floam

floam Sep 13, 2016

Member

Oh - alias is not a builtin. I still like aliases in fish - but I guess that's more heavyweight how they get constructed than I assumed, I'll change this.

Show outdated Hide outdated share/functions/realpath.fish
if command -v grealpath > /dev/null
alias realpath=grealpath
else
alias realpath=fish_realpath

This comment has been minimized.

@krader1961

krader1961 Sep 13, 2016

Contributor

I'm ambivalent about this aspect of the proposed change. Since nearly all invocations specify only a single path, with no flags, this specific change won't matter in practice. On the other hand the extremely small number of invocations which might pass a flag (in particular -P or --physical) will be broken since our builtin doesn't recognize any flags.

@krader1961

krader1961 Sep 13, 2016

Contributor

I'm ambivalent about this aspect of the proposed change. Since nearly all invocations specify only a single path, with no flags, this specific change won't matter in practice. On the other hand the extremely small number of invocations which might pass a flag (in particular -P or --physical) will be broken since our builtin doesn't recognize any flags.

This comment has been minimized.

@floam

floam Sep 13, 2016

Member

Then let's nuke realpath.fish. It was a mistake. Scripts should have been using fish_realpath directly.

@floam

floam Sep 13, 2016

Member

Then let's nuke realpath.fish. It was a mistake. Scripts should have been using fish_realpath directly.

This comment has been minimized.

@floam

floam Sep 13, 2016

Member

I mean - what is it supposed to do? The manpage acts like it is supposed to use a better realpath, like GNUs, if installed. It does on Linux. It does on your system since you indicated you unprefixed the homebrew coreutils utilities.

@floam

floam Sep 13, 2016

Member

I mean - what is it supposed to do? The manpage acts like it is supposed to use a better realpath, like GNUs, if installed. It does on Linux. It does on your system since you indicated you unprefixed the homebrew coreutils utilities.

This comment has been minimized.

@krader1961

krader1961 Sep 13, 2016

Contributor

I mean - what is it supposed to do? The manpage acts like it is supposed to use a better realpath, like GNUs, if installed. It does on Linux. It does on your system since you indicated you unprefixed the homebrew utilities.

The man page implies nothing of the sort. The fish_realpath(1) man page wording can certainly be improved. Especially with respect to --canonicalize-existing. But I don't see how you reached that conclusion. It seems to me that you simply dislike that our fallback implementation doesn't implement capabilities you believe are useful but all the available evidence suggests are so seldom used that our current fallback is "good enough". If you want to expend the effort to provide a more feature full fallback no one is stopping you.

@krader1961

krader1961 Sep 13, 2016

Contributor

I mean - what is it supposed to do? The manpage acts like it is supposed to use a better realpath, like GNUs, if installed. It does on Linux. It does on your system since you indicated you unprefixed the homebrew utilities.

The man page implies nothing of the sort. The fish_realpath(1) man page wording can certainly be improved. Especially with respect to --canonicalize-existing. But I don't see how you reached that conclusion. It seems to me that you simply dislike that our fallback implementation doesn't implement capabilities you believe are useful but all the available evidence suggests are so seldom used that our current fallback is "good enough". If you want to expend the effort to provide a more feature full fallback no one is stopping you.

This comment has been minimized.

@floam

floam Sep 13, 2016

Member

The manpage says specifically to not use fish_realpath directly, because we provide an interface through the function "realpath" which will find "an external command" else fallback to fish_realpath - implication: an external command we can find is preferable - ours is a fallback. But you don't like the change here to actually find an external command in a way that works on BSD where it will commonly be installed as grealpath?

I mean, if you want to double down on the builtin - I'd rather remove it - I guess you should rename it to just "realpath" and prefer it over BSD realpath as well.

I'd prefer removing the builtin entirely - we use it nowhere in our project and searching github for fish script with realpath in it nets few results. It looks like it was implemented rather hastily after one guy filed an issue - what's the bar for a fish builtin?

@floam

floam Sep 13, 2016

Member

The manpage says specifically to not use fish_realpath directly, because we provide an interface through the function "realpath" which will find "an external command" else fallback to fish_realpath - implication: an external command we can find is preferable - ours is a fallback. But you don't like the change here to actually find an external command in a way that works on BSD where it will commonly be installed as grealpath?

I mean, if you want to double down on the builtin - I'd rather remove it - I guess you should rename it to just "realpath" and prefer it over BSD realpath as well.

I'd prefer removing the builtin entirely - we use it nowhere in our project and searching github for fish script with realpath in it nets few results. It looks like it was implemented rather hastily after one guy filed an issue - what's the bar for a fish builtin?

This comment has been minimized.

@floam

floam Sep 13, 2016

Member

Anyhow, if you're merely ambivalent - I insist :)

@floam

floam Sep 13, 2016

Member

Anyhow, if you're merely ambivalent - I insist :)

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Sep 18, 2016

Contributor

I'm closing this because it does not solve the real problem. That problem is that fish_realpath a/b/c fails if a/b exists but a/b/c does not. The intent was that it would require all path components to exist except the final one since that is the overwhelming use case.

This change does not fix that error. It instead papers over the problem with a OS X/macOS specific change. And it does so in a manner that introduces other problems.

Contributor

krader1961 commented Sep 18, 2016

I'm closing this because it does not solve the real problem. That problem is that fish_realpath a/b/c fails if a/b exists but a/b/c does not. The intent was that it would require all path components to exist except the final one since that is the overwhelming use case.

This change does not fix that error. It instead papers over the problem with a OS X/macOS specific change. And it does so in a manner that introduces other problems.

@krader1961 krader1961 closed this Sep 18, 2016

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Sep 18, 2016

Member

No - what this was trying to fix was just the fish script failing to pick up realpath if it's prefixed with a 'g'.

Member

floam commented Sep 18, 2016

No - what this was trying to fix was just the fish script failing to pick up realpath if it's prefixed with a 'g'.

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Sep 18, 2016

Member

I think it's unlikely even in the cases where it is called with the same number of arguments, that our bare-bones builtin would be preferred.

Member

floam commented Sep 18, 2016

I think it's unlikely even in the cases where it is called with the same number of arguments, that our bare-bones builtin would be preferred.

@floam floam reopened this Sep 18, 2016

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Sep 18, 2016

Member

I have also since gotten this a little less likely to do wrong things.

Member

floam commented Sep 18, 2016

I have also since gotten this a little less likely to do wrong things.

fish_realpath: filter out dangerous options
Per feedback do not use aliases to declare wrapped functions.
@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Sep 18, 2016

Member

Meh - I'm just going to merge it. It appears to be working better.

Member

floam commented Sep 18, 2016

Meh - I'm just going to merge it. It appears to be working better.

@floam floam merged commit 96ebfaa into fish-shell:master Sep 18, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment