Make route optional in git-bundle-server init
#36
Conversation
If a positional (string) arg is missing, the 'Arg()' function will return an empty string without indicating that the value is missing. Since (for the moment) positional args are always required, add an explicit check that the arg is present and, if not, exit with usage. Signed-off-by: Victoria Dye <[email protected]>
Add 'optional' property to 'argparse.positionalArg'. This leads to the following behavior if an arg isn't specified on the command line: - If 'required' is true, exit with the appropriate usage message. - If 'required' is false, stop processing positional arguments. The arg values are never set, leaving them with the default values with which they were originally specified. For now, mark all existing positional arguments as required to avoid changes to behavior. Signed-off-by: Victoria Dye <[email protected]>
Add a printout of positional args (name, description, '(optional)' or not) in the 'Usage()' string. Signed-off-by: Victoria Dye <[email protected]>
derrickstolee
left a comment
derrickstolee
left a comment
There was a problem hiding this comment.
I'm incapable of parsing regexes. Much like Perl, it's a "write only" language (understandable only by the author). This is somewhat of a personal failure, but it just means I rely on tests instead of my own understanding.
I have some suggestions for unit tests on the regex that will ease my concerns.
cmd/git-bundle-server/init.go
Outdated
|
|
||
| // Set route value, if needed | ||
| if *route == "" { | ||
| urlMatcher := regexp.MustCompile(`^.*(?:/|:)([\w\.-]+)/([\w\.-]+).git$`) |
There was a problem hiding this comment.
Interesting, so you require the .git naming convention? It's not necessary, and I usually omit it when I type Git URLs.
There was a problem hiding this comment.
This is also a great candidate for a unit test, because regexes are hard to understand and are poorly tested.
There was a problem hiding this comment.
Example cases to check (consider adding .git as a second case for each):
- SSH URLs.
[email protected]:git/git - URLs without both
org/repo.https://github.com/git - URLs with more than
org/repo.https://dev.azure.com/gvfs/ci/_git/ForTests - What about
file://?file://my/local/path/is/strange
(I can see some of these being worthy to consider in the future, such as ADO URLs, but having the tests now will help make it explicit that it's not the design of the parser at the moment.)
There was a problem hiding this comment.
I split out the matching routine into its own function, and created different regexes for SSH, HTTP, and filesystem routes. I know more regexes probably isn't what you want 😁 but hopefully the added unit tests (and slightly better commenting) will help clarify the expected behavior to readers.
There was a problem hiding this comment.
The only regex I trust is a well-tested one. Thanks for beefing them up.
Make the '<route>' positional arg optional in 'git-bundle-server init'. If
it is not specified (or is an empty string), extract a route from the
supplied clone URI.
Called by 'init', 'core.GetRouteFromUrl()' attempts to match the URL against
SSH, HTTP(S), and filesystem ('file://') URLs. If the URL cannot be matched,
'init' will exit with an error and usage printout.
Also include unit tests of the route matching function to demonstrate its
behavior.
Signed-off-by: Victoria Dye <[email protected]>
vdye
force-pushed
the
vdye/derived-route
branch
from
e4b7bf2 to
088f4ac
Compare
derrickstolee
left a comment
derrickstolee
left a comment
There was a problem hiding this comment.
Excellent tests, on both sides of the pattern match.
| false, | ||
| }, | ||
| { | ||
| "HTTPS://SCREAM/INTOTHEVOID", |
This pull request does two main things:
argparsewith a distinction between "required" and "optional". If a positional arg is required but not specified, return a usage error; if a positional arg is optional, do not assign it a value. (commits 1-3)routean optional positional arg ingit-bundle-server init; if it is unspecified/empty, try to derive it from the given URL. (commit 4)Part of #35