Skip to content

completions: invoke "cd" as "builtin cd" #528

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

Merged
merged 3 commits into from
Jun 1, 2021

Conversation

akinomyoga
Copy link
Collaborator

@akinomyoga akinomyoga commented May 23, 2021

In searching the uses of cd command in completions, I found that cd is directly called instead of through builtin cd. The builtin cd is one of the commands that users define an alias or a function (to save the visited directory, etc.). I suggest prefixing them with builtin keyword.

I searched for the policy of bash-completion on this kind of treatment and found in CONTRIBUTING.md the following item:

  • Use printf(1) instead of echo(1) for portability reasons, and be sure to invoke commands that are often found aliased (such as ls or grep etc) using the command (or builtin) command as appropriate.

So I think we should also apply this rule to cd builtin. (By the way, I found that so far no builtin commands have been prefixed by builtin in any completions, so cd is the first one).

I have also added a rule to test/runLint. There are a few false positives, and many cd in test codes. If we should also prefix cd in test codes, please let me know. Then, I will add them.

@scop
Copy link
Owner

scop commented May 24, 2021

Hm, I don't think I've heard of cd being aliased to something, and nobody has to my knowledge reported a problem with this in nearly two decades. (BTW cd does have some/previous visited dir support out of the box.)

I've resisted changes like this, because one could make the same argument for pretty much any command out there leading to prefixing everything which would be annoying. In some cases would not be the right thing to do either, because there might be very real reasons for why something is aliased and having bash-completion use it is desirable.

If there is a case why we'd like to do this for cd anyway despite the above, why wouldn't we do it via command like we do for others, but use builtin for it instead? (Sure, builtin is not applicable to sed, grep and the like, but on the other hand command would seem to work for cd.)

@akinomyoga
Copy link
Collaborator Author

akinomyoga commented May 24, 2021

Hm, I don't think I've heard of cd being aliased to something,

For some simple aliases, maybe you can search with alias cd='cd -P' or alias cd=pushd (though I don't think this kind of alias is useful). More typical cases define a function named cd (or define a function mycd and alias it as cd). Usually, people define these tiny functions by themselves, but there is also a popular plugin:

I also wrote my own, being inspired by some blog article (it is about ten years ago, so I don't remember the page. Anyway it was written in Japanese). Also, I have received in my project a report on the problem caused by alias/function cd before. I can also find many examples by searching in the internet:

There seem to be also discussions on alias cd='cd -P' and alias cd=pushd

and nobody has to my knowledge reported a problem with this in nearly two decades.

I think that is because these aliases and functions usually preserve the visible behavior of cd so do not cause problems immediately, but later, for example, one would notice that their cd history is contaminated by some paths the user didn't visit. This is not a serious problem, and it is not clear at a glance when the contamination has occurred and what is the culprit, but it is simply annoying.

(BTW cd does have some/previous visited dir support out of the box.)

builtin cd only remembers one previous place. pushd may be used to remember all the visited directory in the session, but it is not shared with other sessions and will disappear once the session ends.
A useful example that cannot be achieved by the built-in bash features is above-mentioned b4b4r07/enhancd. Another plugin wting/autojump doesn't override cd but define a different command j, but it shows how the behavior of cd can be usefully modified.

I've resisted changes like this, because one could make the same argument for pretty much any command out there leading to prefixing everything which would be annoying.

I think cd is one of the most frequently used commands in the interactive shells, so it is not surprising to me that people modify the behavior by alias/functions.

In some cases would not be the right thing to do either, because there might be very real reasons for why something is aliased and having bash-completion use it is desirable.

I don't think these interactive features (history recording, automatic corrections/expansions, etc.) are wanted for internal completions by users.

If there is a case why we'd like to do this for cd anyway despite the above, why wouldn't we do it via command like we do for others, but use builtin for it instead? (Sure, builtin is not applicable to sed, grep and the like, but on the other hand command would seem to work for cd.)

OK, I don't have particular preferences on them so will make them command the same as sed, grep, etc.

@akinomyoga
Copy link
Collaborator Author

akinomyoga commented May 24, 2021

I think I should move the change cd => builtin cd in #527 to here as you have requested to separate the PR. It will introduce conflicts with #527, so I'll update this PR after #527 is merged.

@scop
Copy link
Owner

scop commented May 25, 2021

I don't think these interactive features (history recording, automatic corrections/expansions, etc.) are wanted for internal completions by users.

That's right. But with this I was trying to refer to blanket builtin/command prefixing everything, not cd in particular which I agree would unlikely be adversely affected.

@scop
Copy link
Owner

scop commented May 25, 2021

Anyway, thank you for the references, I'm sold on it now :) Let's have a look after #527 as you said.

@akinomyoga akinomyoga force-pushed the invoke-cd-throught-builtin branch 2 times, most recently from e5b5937 to cac9293 Compare May 28, 2021 11:09
@akinomyoga
Copy link
Collaborator Author

I have updated to use command cd (instead of builtin cd). I also included the changes on cd which was originally in #527.

@akinomyoga akinomyoga force-pushed the invoke-cd-throught-builtin branch from cac9293 to b7c68e1 Compare May 29, 2021 12:18
@scop scop merged commit a7aa054 into scop:master Jun 1, 2021
@scop
Copy link
Owner

scop commented Jun 1, 2021

Thanks!

@akinomyoga akinomyoga deleted the invoke-cd-throught-builtin branch June 5, 2021 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants