Skip to content
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

from: fallback to command for non-existent files #51

Closed
wants to merge 1 commit into from

Conversation

eepykate
Copy link
Contributor

@eepykate eepykate commented Oct 27, 2021

example (From a dir without that filename): cpm f test will be replaced with cpm f /bin/test, which will probably be coreutils.

@willeccles
Copy link
Owner

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.

@eepykate
Copy link
Contributor Author

Like that?

@willeccles
Copy link
Owner

I think putting it in the resolve() function would be good.

@eepykate
Copy link
Contributor Author

I mean, resolve is currently only used in xbps.

Do you want me to add it to every other PM?

@eepykate
Copy link
Contributor Author

eepykate commented Oct 27, 2021

to be clear,

  cd -P "${1%/*}" 2>/dev/null || PWD="${1%/*}"

what's this meant to do?

@willeccles
Copy link
Owner

Rename this PR and make it both update resolve() and use it for other PMs as well.

@eepykate eepykate changed the title from: fallback to command for non-existent files. from: use resolve() & fallback to command for non-existent files Oct 27, 2021
@willeccles
Copy link
Owner

willeccles commented Oct 27, 2021

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:

  1. /bin is a symlink to /usr/bin
  2. /bin/something is a symlink to something else

This function resolves symlinks in paths so that this does not happen. You can just add this functionality to it.

@eepykate
Copy link
Contributor Author

does it.. even work?

image

@eepykate
Copy link
Contributor Author

Oh, the next line makes it work

yeah i'm never gonna look at those lines again, it hurts

@eepykate eepykate force-pushed the master branch 2 times, most recently from dab6921 to dab69c3 Compare October 27, 2021 14:25
@eepykate
Copy link
Contributor Author

@willeccles You might wanna link the issue to this PR in this PR's sidebar btw.

@willeccles willeccles linked an issue Oct 27, 2021 that may be closed by this pull request
@willeccles
Copy link
Owner

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.

@willeccles
Copy link
Owner

Does this actually work with more than one argument?

@eepykate
Copy link
Contributor Author

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.

@willeccles
Copy link
Owner

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.

@eepykate
Copy link
Contributor Author

Oh, wait, it did work but i think i broke it.

@eepykate
Copy link
Contributor Author

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.

@willeccles
Copy link
Owner

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.

@eepykate
Copy link
Contributor Author

eepykate commented Oct 27, 2021

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.

@willeccles
Copy link
Owner

At any rate, like we do for other things, PMs which only support one argument should only be given one.

@eepykate
Copy link
Contributor Author

oh, that explains it

resolve() is broken anyway

@eepykate
Copy link
Contributor Author

image

first == my edits

second == original

@willeccles
Copy link
Owner

What is test?

@eepykate
Copy link
Contributor Author

image

@eepykate
Copy link
Contributor Author

I assume that's more clear.

@eepykate
Copy link
Contributor Author

no thoughts, head empty

@eepykate
Copy link
Contributor Author

eepykate commented Oct 27, 2021

image

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

@willeccles
Copy link
Owner

in my defense that function is a curse from satan and you should have just used realpath

realpath isn't POSIX, though.

@eepykate
Copy link
Contributor Author

eepykate commented Oct 27, 2021

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.....

@willeccles
Copy link
Owner

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##*/}"
}

@eepykate eepykate changed the title from: use resolve() & fallback to command for non-existent files from: fallback to command for non-existent files Oct 27, 2021
@eepykate
Copy link
Contributor Author

oh, i was just going to go back to the original code because it is literally much better

@willeccles
Copy link
Owner

The -f here may need to be updated, just in case you try to use it on a special file or something, I suppose.

@eepykate
Copy link
Contributor Author

  1. i'll have to remove the quotes for multiple arguments to work with a function, ergo breaking spaces
  2. resolve() is from hell and i hate it
  3. it's a simple one line improvement

@willeccles
Copy link
Owner

Just don't support multiple arguments.

@eepykate
Copy link
Contributor Author

No.

@willeccles
Copy link
Owner

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##*/}"
}

@willeccles
Copy link
Owner

I'll even commit this myself to save you the trouble.

@eepykate
Copy link
Contributor Author

eepykate commented Oct 27, 2021

👍 Unless it's a complete rewrite of it, I would much prefer never touching resolve() again.

@willeccles
Copy link
Owner

Your fix is a pretty trivial one-line addition to the resolve function, and adding it to all PMs is quite simple.

@eepykate
Copy link
Contributor Author

eepykate commented Oct 27, 2021

But, adding resolve() to all PMs is a downgrade.

@willeccles
Copy link
Owner

But, adding resolve() to all PMs is a downgrade.

How?

@eepykate
Copy link
Contributor Author

  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.

@willeccles
Copy link
Owner

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.

@eepykate
Copy link
Contributor Author

eepykate commented Oct 27, 2021

doesn't even support your multiple arguments stance.

Difference is, that's for one scenario, and not a constant thing.

@willeccles
Copy link
Owner

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.

@eepykate
Copy link
Contributor Author

eepykate commented Oct 27, 2021

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).

@willeccles
Copy link
Owner

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.

@eepykate
Copy link
Contributor Author

every package manager benefits from both symlink resolution

like... no? ​ It's just xbps that needs it.

and your patch

Does in this patch too, just without it being a constant downgrade.

you can use the PM directly

It's literally a feature that cpm already supports, why are you suggesting "don't use cpm for something that it supports which doesn't go against its philosophy so i can downgrade it for no sane reason"

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.

I have no idea what you mean with this, but tbh I'm done, it's clear that you just want to downgrade it.

@eepykate eepykate closed this Oct 27, 2021
@willeccles
Copy link
Owner

like... no? ​ It's just xbps that needs it.

Some package managers may return the "wrong" package if the target file is a symlink. While it may point to a file installed by another package (and which you are almost certainly looking for), you will get the package that installed the symlink, which may not be the same. It's a very rare case that you'd be interested in which package installed the symlink.

I have no idea what you mean with this, but tbh I'm done, it's clear that you just want to downgrade it.

If you want to allow multiple arguments, at least make your patch actually handle this case. If you added it to the resolve function and used that for every PM, you could make it support multiple arguments there, too, if you wanted. Just don't use $@ for (for example) xbps, which only allows one argument. I'm not sure why this is so difficult.

@willeccles
Copy link
Owner

I'd be willing to merge even if you didn't use resolve for everything, but at the very least make your fix work for all arguments rather than just the first one.

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