Fix: Give correct error message for unrecognised options #533
Conversation
LukasFritzeDev
force-pushed
the
fix-error-message
branch
from
27a1de7
to
cf1aa45
Compare
git-ftp
Outdated
| @@ -1831,7 +1831,7 @@ do | |||
| *) | |||
| # Pass thru anything that may be meant for fetch. | |||
| if [ -n "$1" ]; then | |||
| if [ -z "$URL" ]; then | |||
| if [ -z "$URL" ] && ! echo "$1" | egrep -q '^(-[A-Za-z])|^(--[A-Za-z0-9-]+)'; then | |||
There was a problem hiding this comment.
I personally prefer grep's -v option instead of the !. Do you mind?
What do we do if someone registers the domain name -my-domain.example? I guess they have to give the full URL with the protocol then: ftp://-my-domain.example
And What if someone types the option wrong? Example: git ftp push +insecure example.net
It would still take +insecure as URL, right?
There was a problem hiding this comment.
I personally prefer grep's
-voption instead of the!. Do you mind?
Oh yes. sure. I’ll have a look.
What do we do if someone registers the domain name
-my-domain.example? I guess they have to give the full URL with the protocol then:ftp://-my-domain.example
This is matched by the expression as described here: #522 (comment)
And What if someone types the option wrong? Example:
git ftp push +insecure example.net
It would still take+insecureas URL, right?
Yes you’re right. But at it will show this as “Cant connect to '+insecure'” so the user can get where the problem is, can’t he.
If you want to have some details why I choose this solution please refer to the comments for the issue #522.
There was a problem hiding this comment.
Oh no. It does not say Can’t connect but "fatal: Remote host not set.". This is because of set_remote_host but I can fix this by checking it there.
LukasFritzeDev
force-pushed
the
fix-error-message
branch
from
cf1aa45
to
5379603
Compare
git-ftp
Outdated
| @@ -1831,7 +1831,7 @@ do | |||
| *) | |||
| # Pass thru anything that may be meant for fetch. | |||
| if [ -n "$1" ]; then | |||
| if [ -z "$URL" ]; then | |||
| if [ -z "$URL" ] && echo "$1" | egrep -q -v '^(-[A-Za-z])|^(--[A-Za-z0-9-]+)'; then | |||
There was a problem hiding this comment.
Do we need to test for the end of the option as well? I think it's matching too much and should be this:
egrep -q -v '^(-[A-Za-z])$|^(--[A-Za-z0-9-]+)$'
There was a problem hiding this comment.
Sorry. What would be the difference with the $? I know it asserts the end of line, but how would this take effect? Isn’t $1 always just one word? $2 is the value, isn’t it?
There was a problem hiding this comment.
The regex ^-[A-Za-z] matches everything that starts with - and a character. But it doesn't restrict anything afterwards. So it matches -h but also -hello and -A-B-C8_*. The $ marks the end of the match. So ^-[A-Za-z]$ only matches strings that start with a - and then have one letter afterwards. And after that letter the string must be over, nothing else can come afterwards.
There was a problem hiding this comment.
Sorry, I skipped the $ for the first pattern. This makes sense.
But what if somebody tries to do something like -vv (which is a valid option) but accidentally hits the key beside v and types -bb or he hits the button 3 times and types -vvv. With your pattern this would be set as URL, wouldn’t it? And this is exactly, what we wanted to avoid.
As described in #522 (comment) URL hostnames or domains can’t start with a -, so we would be fine with this.
We only get problems if the user gives a url with the pattern username:[password]@host AND the username (not the password) starts with - or --, but I think in the real world this scenario would be really rare, wouldn’t it? It's at least rarer than -vvv.
There was a problem hiding this comment.
I agree that it would be really rare. But those are the cases that can take a long time to debug. I forgot that -vvv is a valid option. In that case I would rewrite the command like this:
egrep -q -v '^(-[A-Za-z]+)$|^(--[A-Za-z0-9-]+)$'And in the case that a username starts with -, people will get a nice error message that -username@example is not a valid option and then they need to use the full URL with protocol: ftp://-username@example. That should work, shouldn't it?
There was a problem hiding this comment.
people will get a nice error message that -username@example is not a valid option
In fact, no. I thought that too, but I just noticed while trying it out that there are other characters in the URL than A-Za-z. And therefore the pattern does not match. So we are fine here.
But I agree. Your pattern with + and $ would be the best of all. But now the two OR parts are very similar so we could simplify it and make the second dash optional and use
egrep -q -v '^(--?[A-Za-z0-9-]+)$'
This would allow numbers in case of one dash, but I don’t thinks that’s a problem. What do you think?
There was a problem hiding this comment.
Oh. Have you ever tried to use a URL that follows the pattern username:[password]@host and is not just host?
I tried a few things and run into the following:
$ git ftp init -username@example
fatal: Can't access remote 'ftp://-username'. Network down? Wrong URL? exiting...
The given URL is cut at the @ character.
The line where this happens is:
Line 618 in ae1a683
The manual for expr says:
If the match succeeds and the pattern contains at least one regular expression subexpression ``\(...\)'', the string corresponding to ``\1'' is returned;
Since -username matches the group only this part is returned and the rest gets lost. So it looks like this kind of compact URLs is not supported at all at the moment, is it?
If this is not supported I think we should create a new enhancement issue for this.
There was a problem hiding this comment.
I personally prefer grep's -v option instead of the !. Do you mind?
One thing I just found: there are 16 instances of ! echo "something" | grep -q … and only my new created one with egrep -q -v. So maybe we should stay consistent with this, even if you don’t like it that much 😉
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
In some cases a URL can be invalid, i.e. when mistyped an option like `+insecure`. In this cases `fatal: Remote host not set.` was printed as error message. Now this message says `... is not a valid host URL.` so the user can see what's wrong.
LukasFritzeDev
force-pushed
the
fix-error-message
branch
from
5379603
to
086615d
Compare
Details for this issue: #522 (comment)
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 codes and messages in both cases.
fixes #522