Repository.IsValid should not throw for null/empty argument

[sharwell]

Currently Repository.IsValid throws an exception if the argument is null or empty. Rather than throw an exception, this method should simply return false in this case, similar to the behavior of Directory.Exists or File.Exists.

[nulltoken]

@sharwell Thanks for putting this under the light. This is indeed a very valid argument in the sense that we expect the method to answer the "is this valid" question, not to throw.

However, I'm a bit torn because... well this method expects a path. And I'm not sure empty or null would actually qualify as valid paths.

Besides returning false for invalid paths, null, empty, perms related issues may hinder a potential troubleshooting process.

Maybe could you please share with us a use case where passing in null or empty would be common and the friction added by dealing with those exceptions should be alleviated?

/cc @dahlbyk @jamill @ethomson @Therzok

[Therzok]

@nulltoken let's say you recursively go down from a folder to root. Path.GetDirectoryName("/") or Path.GetDirectoryName("C:") both return null, so it should not just throw. Also, the argument with File/Directory.Exists accepting null or empty string is also valid.

[Therzok]

To add to the above, GetDirectoryName returns a path (in that specific case, it's null). null is considered a valid path as a parameter to File.Exists and Directory.Exists. I think the IsValid function should be tweaked.

[nulltoken]

@Therzok I understand your point. However, even with the System.IO namespace, this design decision is not uniformely implemented.

• Path.Combine() doesn't like nulls
• Path.GetPathRoot() doesn't like empty strings
• Path.GetFullPath() doesn't like any of them
• Directory.EnumerateDirectories() doesn't like any of them
• ...

[Therzok]

They're not validation functions in my opinion, they're transformation functions.

[jamill]

Interesting - as it stands, the documentation for Repository.IsValid would leave me to expect that it wouldn't throw (Check if a parameter leads to a valid git repository, True if a repository can be resolved through this path; false otherwise). File.Exists or Directory.Exists return false on any errors, which I am not sure is the behavior we want to follow.

I think it depends on what the intent of this method is. If it is trying to determine if a repository exists at a path? Then, if the input is invalid or another error is encountered trying to determine if repository is valid (e.g. user does not have permissions to the read the path), then I think it is valid to return an exception. If the method is only trying to determine if the current user can open the repository with the provided input, then I think returning a false on error or invalid input would be ok (as, a repository cannot be resolved through the input).

I think I would lean towards IsValid returning errors if there is an error in determining whether a valid repository exists at a given path. If we want a function to determine if the current user can open a repository for a given string, maybe that is a separate function.

[carlosmn]

IsValidPath() isn't really a validation function either. It's looking at the path and looking up in the registered scheme list for a prefix. You can pass http://foo:bar:baz;quux and it'll return True.

This method was removed from libgit2, and now only git_remote_supported_url() exists, which hopefully has a clearer name.

Sorry, I misread the function at play here.

[sharwell]

I want to disable functionality for an IDE extension when the user opens a file which is not located within a Git repository. The most efficient check appears to be:

if (!Repository.IsValid(Repository.Discover(filePath)))
    return;

[carlosmn]

That check is useless. If Repository.Discover() returns a path, then that's the path to the repository and you know that it exists. If it's null, then there's no repository in which the file is contained.

Adding Repsitory.IsValid() there doesn't add anything.

[JPaulDuncan]

Sounds like you need a Repository.Exists(path) AND a Repository.IsValid(path) method. One determines if there is a Repository on the given path, and one determines if the Repository is valid (such as what @jamill was saying).

[nulltoken]

I think I would lean towards IsValid returning errors if there is an error in determining whether a valid repository exists at a given path. If we want a function to determine if the current user can open a repository for a given string, maybe that is a separate function.

@jamill So if I understand you correctly, updating the xml documentation on the existing function, to reflect that exceptions could be thrown, would be enough?

[ethomson]

Passing NULL to this function is a bug, and we should continue to throw. If we want to accept an empty string here (and always return false), I guess we can, but let's drop the stabilization tag for this.