Closed eepykate closed 2 years ago
Could you perhaps make this part of/adjacent to the resolve
function? It feels a little out of place in the middle of arg parsing.
Like that?
I think putting it in the resolve()
function would be good.
I mean, resolve is currently only used in xbps.
Do you want me to add it to every other PM?
to be clear,
cd -P "${1%/*}" 2>/dev/null || PWD="${1%/*}"
what's this meant to do?
Rename this PR and make it both update resolve()
and use it for other PMs as well.
to be clear,
cd -P "${1%/*}" 2>/dev/null || PWD="${1%/*}"
what's this meant to do?
Example: I want to know what package provides /bin/something
, so I do cpm from /bin/something
. This fails. There are two possible reasons:
/bin
is a symlink to /usr/bin
/bin/something
is a symlink to something elseThis function resolves symlinks in paths so that this does not happen. You can just add this functionality to it.
does it.. even work?
Oh, the next line makes it work
yeah i'm never gonna look at those lines again, it hurts
@willeccles You might wanna link the issue to this PR in this PR's sidebar btw.
Oh, the next line makes it work
yeah i'm never gonna look at those lines again, it hurts
You'll never guess who I got them from.
Does this actually work with more than one argument?
You'll never guess who I got them from.
honestly the natural leadon from that is that I did it? ​ Fairly sure I didn't, and I didn't even know that -P existed. Foxsouns, I guess?
Does this actually work with more than one argument?
Yeah.
At least xbps only allows one argument, and I can't imagine a use-case for multiple anyway. I'd change all the $@
to $1
and make the function only take one argument.
Oh, wait, it did work but i think i broke it.
At least xbps only allows one argument, and I can't imagine a use-case for multiple anyway. I'd change all the $@ to $1 and make the function only take one argument.
Heavily disagree.
Even if some PMs only support one, others support more, and the ones that don't support >1 will have an error message.
The xbps error message is literally just printing the usage information. I also can't imagine why you'd want to do cpm from file1 file2
, especially if they're from different packages anyway.
I also can't imagine why you'd want to do
So you don't have to run multiple commands..? The whole reason cpm exists is for laziness.
If this was files
, I'd agree, but for from
, nah.
At any rate, like we do for other things, PMs which only support one argument should only be given one.
oh, that explains it
resolve() is broken anyway
first == my edits
second == original
What is test
?
I assume that's more clear.
This does not happen on my machine.
It does on mine.
Oh, it's my changes outside of the function it seems
in my defense that function is a curse from satan and you should have just used realpath
no thoughts, head empty
resolve() {
cd -P "${1%/*}" 2>/dev/null || PWD="${1%/*}"
printf '%s\n' "${PWD}/${1##*/}"
}
realpath /usr/bin/test
cd /usr/bin
resolve test
resolve /usr/bin/test
sorry for spam
in my defense that function is a curse from satan and you should have just used realpath
realpath
isn't POSIX, though.
pain
honestly, i'd just not bother resolving symlinks at all. It doesn't work, and there's no good solution, especially not a short & understandable one.
Also, half the reason I caved into splitting f/F was because you wanted relative paths, which resolve() doesn't even support.....
I have fixed the resolve function. Also, xbps does not work with symlinks, and other PMs may also not work with symlinks. Here's a fixed version, I think:
resolve() {
p="${1%/*}"
[ -f "$p" ] && p="."
cd -P "$p" 2>/dev/null || PWD="$p"
printf '%s\n' "${PWD}/${1##*/}"
}
oh, i was just going to go back to the original code because it is literally much better
The -f
here may need to be updated, just in case you try to use it on a special file or something, I suppose.
Just don't support multiple arguments.
No.
Actually, I think we can improve my fixed version:
resolve() {
p="${1%/*}"
[ "$p" = "$1" ] && p="."
cd -P "$p" 2>/dev/null || PWD="$p"
printf '%s\n' "${PWD}/${1##*/}"
}
I'll even commit this myself to save you the trouble.
:+1: Unless it's a complete rewrite of it, I would much prefer never touching resolve() again.
Your fix is a pretty trivial one-line addition to the resolve function, and adding it to all PMs is quite simple.
But, adding resolve() to all PMs is a downgrade.
But, adding resolve() to all PMs is a downgrade.
How?
1. resolve() only exists to fix xbps being bad 2. other PMs support multiple arguments, and there is no good way to make this function _that only exists to fix xbps being bad_ support multiple arguments.
I only created it for xbps, but I have no idea if other package managers require it as well. Not to mention, your current patch doesn't even support your multiple arguments stance.
doesn't even support your multiple arguments stance.
Difference is, that's for one scenario, and not a constant thing.
doesn't even support your multiple arguments stance.
Difference is, that's for one scenario, and not a constant thing.
Inconsistently handling multiple arguments is much worse than consistently allowing only one argument.
Heavily disagree.
I'd prefer sometimes being shit (but that time also has a different improvement) over always being shit (even when not using the addition).
I don't think it's always bad in either case. If you want to lookup multiple files at once and your PM allows it, you can use the PM directly if you'd prefer not to write two commands. If you moved your change to the resolve function and then applied that to all PMs, every package manager benefits from both symlink resolution and your patch. If you also want to make it work for multiple arguments, you can do that, too. Just don't do it for PMs which only support one.
example (From a dir without that filename):
cpm f test
will be replaced withcpm f /bin/test
, which will probably be coreutils.