cantino / mcfly

Fly through your shell history. Great Scott!
MIT License
6.76k stars 176 forks source link

command -v when in root directory #292

Closed joefiorini closed 1 year ago

joefiorini commented 1 year ago

I found a bug introduced in #216. It turns out when you are in the root directory (/) and you have mcfly installed in /bin (on a lot of Linux distros that is symlinked to /usr/bin) then command -v mcfly returns ./bin/mcfly. There is no option to command to make it return an absolute path. However, wrapping the call in realpath fixes the issue.

The original change cites a deprecation warning in which. I have been unable to reduce this warning (using Arch Linux). Unfortunately due to the name of the command being such a common english word it's very difficult to search for why this warning appears. Reading the man page says nothing about a deprecation. Maybe the OP of #216 was using Mac and there is something in BSD that is causing it?

Either way, I think it should either use realpath around command -v or go back to using which (maybe without using fish's command and relying on $PATH), as it's a ubiquitous command across Mac & Linux. The existing behavior is very bad.

EDIT: I just noticed that mcfly uses command -v in all of its shell scripts, and it seems that, at least in the case of bash, it suffers from the same issue mentioned above. On Linux it takes a -p argument that prints the absolute path, but that's a POSIX standard command so I don't know what the support is on Mac.

Is there a problem with just using plain which and relying on it being in $PATH?

cantino commented 1 year ago

Thank you for identifying this @joefiorini, and for the clear writeup.

I never had a problem with which on my system, but @Efreak was having issues and the fixed seemed to work everywhere I tested it until you found this issue.

realpath `command -v mcfly`

seems to work for me on linux and OS X. Do you know if it exists everywhere?

cantino commented 1 year ago

@Efreak I'd love your input on this.

cantino commented 1 year ago

Does this https://github.com/cantino/mcfly/pull/304 fix it for you @joefiorini?

Efreak commented 1 year ago

I'm not sure what package includes which on Arch, as most of my current Linux systems are derived from Debian (primarily termux). In this case the command which comes from the package debianutils, where it was clearly deprecated (it posted a warning to stderr on every use, which is why I submitted #216) in favor of POSIX-mandated type and command.

Regarding the deprecation warning: there's discussion of this here and here. This LWN article seems to be saying that the deprecation notice has been removed; if this is the case (and my tests show that it is) then there's really no problem with reverting #216 entirely. As long as I'm not getting constant stderr spam, I'm happy.

testing `command -v` on bash and fish On bash it doesn't seem to matter that the executable is in a descendant of the cwd; it outputs the full path regardless ``` ❯ echo $BASH_VERSION; cd /; pwd; command -v mcfly; cd; pwd; command -v mcfly 5.2.9(1)-release / /data/data/com.termux/files/home/.cargo/bin/mcfly /data/data/com.termux/files/home /data/data/com.termux/files/home/.cargo/bin/mcfly ``` This might also be a version-specific or configuration issue (your configuration or odd defaults in termux) as fish returns the same thing (neither of these tests is run as root, since I'm on an unrooted tablet; unlike bash, fish refuses to change to a directory I can't access; running as non-root may also cause this result?): ``` u0_a1429@localhost ~> echo $FISH_VERSION; cd /; pwd; command -v mcfly; cd; pwd; command -v mcfly 3.5.1 cd: Permission denied: '/' /data/data/com.termux/files/home /data/data/com.termux/files/home/.cargo/bin/mcfly /data/data/com.termux/files/home /data/data/com.termux/files/home/.cargo/bin/mcfly ```
cantino commented 1 year ago

Should be fixed in #309. Thanks @joefiorini and @Efreak!