Community
Participate
Working Groups
Currently, Snipmatch's "Add repository" dialog only checks the URI scheme. This leads to issues like Bug 479710, where the users enters an invalid URI (but of a supported scheme) and only later gets an error message. We should check this up front to enhance the user experience. Relevant code seems to be JGit's Transport.getTransportProtocols() and TransportProtocol.canHandle(...).
Hi Andreas, I've been working on this, as suggested, the "Add Repository" dialog now uses JGit's TransportProtocol.canHandle to determine if the typed Uri can be handled by one of the transport protocols. I attached a screenshot of the error message that is displayed when the URI is not valid. Besides this verification, I think the dialog should try to open the repository to validate if it is correct, before allowing the user to save it. I'm not sure if this is what you had in mind from the beginning. PD: I am aware of the "code freeze" phase, so I haven't done any commits yet.
Created attachment 259579 [details] Demo of the proposed fix for "Add New Repository" dialog
(In reply to Yasett Acurana from comment #1) > Hi Andreas, > > I've been working on this, as suggested, the "Add Repository" dialog now > uses JGit's TransportProtocol.canHandle to determine if the typed Uri can be > handled by one of the transport protocols. > > I attached a screenshot of the error message that is displayed when the URI > is not valid. Looks good, but (if possible) I would prefer if it the "Fetch URI is not valid" message could provide some more detail, e.g. Fetch URI is not valid: unsupported protocol where "unsupported protocol" is some (localized!) error message that JGit provided to us, perhaps as part of an exception. > Besides this verification, I think the dialog should try to open the > repository to validate if it is correct, before allowing the user to save > it. I don't think this is the Eclipse way of doing things. The "Import" wizards, e.g., also don't try to import (as in download files/create workspace projects) stuff before you click "Finish". That being said, Snipmatch is currently not very good at reporting errors when a Git clone went wrong for whatever reason. But tackling this should IMHO be a separate bug. So, please submit the URI checking logic only to Gerrit; we can talk about the next steps on recommenders-dev then. Thank you. > PD: I am aware of the "code freeze" phase, so I haven't done any commits yet. No worries. You can push commits to the master branch for review at any time; the code freeze is restricted to the maintenance branch (2.3.0).
New Gerrit change created: https://git.eclipse.org/r/66088
New Gerrit change created: https://git.eclipse.org/r/67401
Gerrit change https://git.eclipse.org/r/67401 was merged to [maintenance]. Commit: http://git.eclipse.org/c/recommenders/org.eclipse.recommenders.git/commit/?id=675bfbbabf6445219c9ce019c02befbc7279c9c0
(In reply to Eclipse Genie from comment #6) > Gerrit change https://git.eclipse.org/r/67401 was merged to [maintenance]. > Commit: > http://git.eclipse.org/c/recommenders/org.eclipse.recommenders.git/commit/ > ?id=675bfbbabf6445219c9ce019c02befbc7279c9c0 Merged for 2.3.1. Thanks Yasett.
There are still some problems with the implementation. While we use JGit classes to check validity, we don't use its output. Thus, we have a long if-then-else chains of checks where we generate error messages. These error messages are not always helpful though. In a case taken from the tests http:// i would have thought the URL to be valid. Somehow it is not though, but the error message just states the protocol is unsupported. We should let JGit code do all the checking and then return its error message. Ideally, we wouldn't need to write any error message ourselves. @Yasset: Are you interested in continuing on this change?
(In reply to Johannes Dorn from comment #8) > > @Yasset: Are you interested in continuing on this change? Yes, I am. > We should let JGit code do all the checking and then return its error > message. Ideally, we wouldn't need to write any error message ourselves. > Browsing JGit's API I found that LsRemoteCommand can help to verify if a URI corresponds to a valid -and existent- Git repository, without having to open or clone it. Do you agree with using this command? Any other ideas?
Thank you. I don't think we need to check whether the repository exists. This would be almost impossible to pull of while user enters the URL. You cannot do the check after each entered character. Checking whether the URL is valid is enough.
I haven't seen in JGit's API a method with this kind of format/syntax validation. Do you know of one? However, it looks like one alternative would be using regular expressions. The main disadvantage is that we would not have a specific message for the user (other than the URI syntax is not valid). I will keep searching for potential solutions. Again, any suggestion or comment is greatly appreciated.
(In reply to Yasett Acurana from comment #11) > I haven't seen in JGit's API a method with this kind of format/syntax > validation. Do you know of one? > > However, it looks like one alternative would be using regular expressions. > The main disadvantage is that we would not have a specific message for the > user (other than the URI syntax is not valid). Coming up with correct regexes for URIs is *hard*. Also, you could just create a URI(ish) from the users input and get essentially the same effect. > I will keep searching for potential solutions. Again, any suggestion or > comment is greatly appreciated. Anyway, what Johannes and I don't like is that the code you wrote essentially ignores any hints JGit might have as to why the URI is incorrect. In [1], for example, URISyntaxException#getLocalizedMessage should find its way to the user. Also, in [2] there are very many cases that do error handling/reporting, of which I believe some can be removed (empty URI, URI not absolute maybe) if only we defer the checking to RepositoryUrlValidator and directly display its message. I hope this is clear. If not, please ask. [1] <https://git.eclipse.org/r/#/c/67401/3/plugins/org.eclipse.recommenders.snipmatch.rcp/src/org/eclipse/recommenders/internal/snipmatch/rcp/util/RepositoryUrlValidator.java> [2] <https://git.eclipse.org/r/#/c/67401/3/plugins/org.eclipse.recommenders.snipmatch.rcp/src/org/eclipse/recommenders/internal/snipmatch/rcp/GitBasedRepositoryConfigurationWizard.java>
(In reply to Andreas Sewe from comment #12) > Anyway, what Johannes and I don't like is that the code you wrote > essentially ignores any hints JGit might have as to why the URI is > incorrect. In [1], for example, URISyntaxException#getLocalizedMessage > should find its way to the user. Using: URIish uri = new URIish(repoUri) we can display a localized message, when a UriSyntaxException occurs. Even so, a Uri like 'http://' will not produce an exception, but at the same time, it cannot be handled by any protocol. What would be the proper message in this case? > Also, in [2] there are very many cases that do error handling/reporting, > which I believe some can be removed (empty URI, URI not absolute maybe) if > only we defer the checking to RepositoryUrlValidator and directly display > its message. When a Uri is empty, RepositoryUrlValidator does the checking with a try-catch statement. Would it be ok to remove the if-else check on [2] and leave the responsibility to the try-catch of [1]? [1] <https://git.eclipse.org/r/#/c/67401/3/plugins/org.eclipse.recommenders.snipmatch.rcp/src/org/eclipse/recommenders/internal/snipmatch/rcp/util/RepositoryUrlValidator.java> [2] <https://git.eclipse.org/r/#/c/67401/3/plugins/org.eclipse.recommenders.snipmatch.rcp/src/org/eclipse/recommenders/internal/snipmatch/rcp/GitBasedRepositoryConfigurationWizard.java>
(In reply to Yasett Acurana from comment #13) > (In reply to Andreas Sewe from comment #12) > > > Anyway, what Johannes and I don't like is that the code you wrote > > essentially ignores any hints JGit might have as to why the URI is > > incorrect. In [1], for example, URISyntaxException#getLocalizedMessage > > should find its way to the user. > > Using: > > URIish uri = new URIish(repoUri) > > we can display a localized message, when a UriSyntaxException occurs. > > Even so, a Uri like 'http://' will not produce an exception, but at the same > time, it cannot be handled by any protocol. What would be the proper message > in this case? I would test that certain components of the URI (like the authority/host) are present. If not, I would warn about a missing host component, for example. > > Also, in [2] there are very many cases that do error handling/reporting, > > which I believe some can be removed (empty URI, URI not absolute maybe) if > > only we defer the checking to RepositoryUrlValidator and directly display > > its message. > > When a Uri is empty, RepositoryUrlValidator does the checking with a > try-catch statement. Would it be ok to remove the if-else check on [2] and > leave the responsibility to the try-catch of [1]? Yes. Maybe you can make isValidUri return an IStatus? That way you can convey both a message to the caller as well as a validation state (isOK()). Moreover, you can even embed the exception (if any). > [1] > <https://git.eclipse.org/r/#/c/67401/3/plugins/org.eclipse.recommenders. > snipmatch.rcp/src/org/eclipse/recommenders/internal/snipmatch/rcp/util/ > RepositoryUrlValidator.java> > [2] > <https://git.eclipse.org/r/#/c/67401/3/plugins/org.eclipse.recommenders. > snipmatch.rcp/src/org/eclipse/recommenders/internal/snipmatch/rcp/ > GitBasedRepositoryConfigurationWizard.java>
(In reply to Andreas Sewe from comment #14) > I would test that certain components of the URI (like the authority/host) > are present. If not, I would warn about a missing host component, for > example. > I found a (complementary?) way of doing this: Using: URI uri = new URI("http://") It will throw a UriSyntaxException if the URI format is not valid. After creating the URI, we can check its format with the method calls: uri.getHost() uri.getPath() Which, as far as I know, should not be null in a valid Uri. Then, we could create the URIish object with: URIish urish = new URIish(uri.toUrl()) (This may throw exceptions that will be handled too) and continue the validation as it is now in RepositoryUrlValidator, with the TransportProtocol.canHandle() method. Does this make sense?* *If so, I saw that a URI creation is already present in Urls.parseURI(), but this method does not display any message if an exception occurs. Do you think I could remove the call to parseURI() and do the checking on RepositoryUrlValidator? > Yes. Maybe you can make isValidUri return an IStatus? That way you can > convey both a message to the caller as well as a validation state (isOK()). > Moreover, you can even embed the exception (if any). > Totally agree.
(In reply to Yasett Acurana from comment #15) > Does this make sense?* Yes. > *If so, I saw that a URI creation is already present in Urls.parseURI(), but > this method does not display any message if an exception occurs. Do you > think I could remove the call to parseURI() and do the checking on > RepositoryUrlValidator? Urls.parseURI is a low-level utility which was not meant to produce precise error messages. RepositoryUrlValidator, in particular if it returns IStatus, should do better. So, please do all checking in the wizard using RepositoryUrlValidator. Once the URI passed that stage, reading with Urls.parseURI is fine, however; atthe low level we assume that the URI is correct. In the wizard, it will very likely be incorrect some of the time.
Created attachment 260188 [details] Example of error messages displayed Hi! I have a new set of changes. I attached screenshots of some of the localized exception messages that the wizard displays. I will push these changes to the maintenance branch. As the current change is already merged, I will have to generate a new Id.
(In reply to Yasett Acurana from comment #17) > I have a new set of changes. I attached screenshots of some of the localized > exception messages that the wizard displays. Thank you. The screenshots look good already, although I would probably parenthesize the low-level explanation from the exception to avoid multiple colons. But if you have externalized the message, that should be a very easy change. > I will push these changes to the maintenance branch. As the current change > is already merged, I will have to generate a new Id. Right, you need a new Change-Id. If you commit with EGit, there's a checkbox for that as well as one for added the Signed-Off-By header.
New Gerrit change created: https://git.eclipse.org/r/68311
Gerrit change https://git.eclipse.org/r/68183 was merged to [maintenance]. Commit: http://git.eclipse.org/c/recommenders/org.eclipse.recommenders.git/commit/?id=48e33a43114f333415cd1f276af31592b51323ef
(In reply to Eclipse Genie from comment #20) > Gerrit change https://git.eclipse.org/r/68183 was merged to [maintenance]. > Commit: > http://git.eclipse.org/c/recommenders/org.eclipse.recommenders.git/commit/ > ?id=48e33a43114f333415cd1f276af31592b51323ef Closing for 2.3.1. Thanks again, Yasett.
