-
Notifications
You must be signed in to change notification settings - Fork 690
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
Unrecognised option produces wrong error message #522
Comments
Maybe this is because But if this really is the issue at least the error message should be better and list the unknown option instead of the URL. |
Thank you, my bad |
@EchoZulu No problem. This points to a problem: the wrong error message. So I’ll reopen this issue and retitle it, so we can fix this in a feature release. |
I did some research on this. The wrong error message is caused by the processing of undefined options. Lines 1727 to 1737 in d496de9
Everything that not matched previously is passed to this lines. And the first unknown thing is treated as the URL. The next unknown option produces the error since it is passed to So in case of In my opinion, this could be fixed by setting the URL only if the unknown option is the very last option of the command: # Line 1730
if [ -z "$URL" -a $# == 1 ]; then According to the manual the URL must be the last part of the command so this would still meet the specification, wouldn’t it? ( |
Okay. It’s not that easy because the tests don’t match this specification 😆 Many of the tests execute a command structured like this: This is because the options and the url are stored in So we could rewrite all the tests or find another solution to this issue. |
Yes, I would like to keep that flexibility to mix up options. Many commands support that. These are the same: cp -r a b
cp a b -r Maybe we should check the option for a valid URL structure. So when it comes to
What do you think? Update: We can probably leave the message as is then. Just declaring the URL as one option:
|
Did I get you right? Instead of setting the url from the first unknown option (current state) or assuming the url is always the last part of the command (my suggestion from above) we could do something like this: # Line 1730
if [ -z "$URL" -a is_URL "$1" ]; then Whatever the function I skipped this in my thoughts because I don’t know if it’s that easy to validate a URL in our use case. We have some protocols to support. What if the URL contains the path? Is it different for SFTP ( But I agree. It would be the best and cleanest solution for this. Setting the URL only if it is one seams reasonable for me. 😉 And maybe a Regex check is enough to catch all cases. But I think it would be good to adopt the error message, since then we can have a unrecognised option or an invalid URL. It would be quite confusing to get something like:
It should say
|
I think a regular expression would be good. It's not easy though. Technically, a URI requires the protocol but we don't enforce that and have a default protocol of Alternatively, we could try to run the URL parsing that we already have in Git-ftp and then see if we get any reasonable values out. So we would need at least a valid host after parsing the URL. Re-writing the current URL parsing is a good idea anyway. I often think that it's quite buggy but I wouldn't be able to give you examples now. There are probably some open issue for that. |
Yeah, and that’s the reason why I tried to find a easier solution in the first place 😆 But the simplest solution is not always the best. Running the URL parsing in this early state is a interesting idea, but could lead to other problems. What if the URL is given by command but other parts (user, path, …) are stored in a config? What if the option So I’d try to find a regex in the first place. |
Okay, I'm happy to go with a regex. |
Turns out its actually not easy aka. super complicated to find a regex that matches everything we need. There are a lot of patterns out there, but none of the ones I tried covered all the cases we needed. So I would propose to go with an easier approach: Define case where something is meant as an option (not a url) and exit in this cases. This is quite easy:
It’s not 100% perfect. As you said before a URL hostnames or domains can’t start with a |
The first unknown thing was always treated as the URL. So in cases where the unrecognised option was before the URL the URL caused the error. This fixes this by checking the unknown thing if it could be meant as a option. If it is a option it is not set as the URL but produces the error. In addition two tests are added to check the produced error messages in both cases. fixes git-ftp#522
The first unknown thing was always treated as the URL. So in cases where the unrecognised option was before the URL the URL caused the error. This fixes this by checking the unknown thing if it could be meant as a option. If it is a option it is not set as the URL but produces the error. In addition two tests are added to check the produced error messages in both cases. fixes git-ftp#522
The first unknown thing was always treated as the URL. So in cases where the unrecognised option was before the URL the URL caused the error. This fixes this by checking the unknown thing if it could be meant as a option. If it is a option it is not set as the URL but produces the error. In addition two tests are added to check the produced error messages in both cases. fixes git-ftp#522
The first unknown thing was always treated as the URL. So in cases where the unrecognised option was before the URL the URL caused the error. This fixes this by checking the unknown thing if it could be meant as a option. If it is a option it is not set as the URL but produces the error. In addition two tests are added to check the produced error messages in both cases. fixes git-ftp#522
This may mean we need to "initialize" the push first time -- or ensure ubuntu 18.04 LTS installs a newer version. The issue was found to be discussed in git-ftp GitHub page as git-ftp/git-ftp#522.
Bug
Run
git-ftp push --auto-init "sftp://example.com"
Option "--auto-init" produce error
fatal: Unrecognised option: sftp://example.com
Environment
/usr/bin/zsh
Linux DESKTOP-11HGVEI 4.4.0-17763-Microsoft #379-Microsoft Wed Mar 06 19:16:00 PST 2019 x86_64 GNU/Linux
git-ftp version 1.3.1
The text was updated successfully, but these errors were encountered: