Summary:

Update and streamline remote repository detection.

Review Request #11536 — Created March 21, 2021 and submitted March 28, 2021, 10:34 a.m.

Information
Owner:	david
Repository:	RBTools
Branch:	master
Bugs:
Depends On:
Reviewers
Groups:	rbtools
People:

Description

This is a major rework of the remote repository detection mechanism
within RBTools. The old implementation had a lot of issues, worst of
which was that there was a lot of inconsistency across different tool
implementations (for example, rbt post had a lot of additional
detection code that wasn't shared with other tools).

In the new implementation, we first try to fetch remote repositories,
filtering by the configured repository name, or detected repository
path(s). If this returns a single result, we can immediately return
that. If that fails, it means we're in a situation where the configured
paths on the server are different from the local path (including any
mirror_path), and we need to match in a different way. Individual tools
may implement find_matching_server_repository in order to do
tool-specific comparisons (which right now is done by SVN with
repository UUID and ClearCase with VOB UUID). This new method replaces
the old RepositoryInfo.find_server_repository_info method, which
always felt super ugly and clunky. In lieu of that, RepositoryInfo
subclasses can implement update_from_remote, which allows them to
include information from the remote repository or repository info
resource.

Commands which want to make use of the repository can now set the
needs_repository attribute. In this case, the command initialization
will handle finding the remote repository and updating the
RepositoryInfo appropriately. This corrects a couple issues where
commands were using the repository but forgetting to call the old
find_server_repository_info, potentially ending up with incorrect
data. The setup-repo command works a little differently because it
needs to potentially do that multiple times, but it uses the same
underlying mechanism to attempt to pre-filter the list of matching
repositories in a useful way.

This includes several other improvements and cleanups along the way:

Various places that fetch repositories to find matches have all been
updated to use the locally detected tool to filter the response from
the server. In the case where the server has a mix of repository
types, this will dramatically improve the results.
rbt setup-repo's server initialization has been fixed to merge
handling of --server and interactive entry. This both improves the
code, and has the side effect of making it so that if the server
specified on the command line doesn't work, it will prompt the user.

Testing Done


Ran unit tests.
Verified that repository match fetches properly passed tool= for API

  requests to filter the responses based on the locally detected tool.
rbt land: Tested basic behavior.
rbt patch: Tested basic behavior.
rbt post: Posted new and updated changes against Git, including with

-u. Created a remote SVN repository and posted purely remote commits

  using --repository-url.
rbt setup-repo: Tested with both --server and specifying the

  server name interactively.
rbt stamp: Tested basic behavior.
rbt status: tested both with and without --all. Verified that

  repository detection was making the most minimal API requests possible

  based on various .reviewboardrc configurations.

Commits

Summary
	Update and streamline remote repository detection.

Screenshots

Issues

Description	From	Last Updated
E501 line too long (82 > 79 characters)	reviewbot	March 21, 2021, 4:56 p.m.
E501 line too long (82 > 79 characters)	reviewbot	March 21, 2021, 4:56 p.m.
E501 line too long (82 > 79 characters)	reviewbot	March 21, 2021, 4:56 p.m.
E501 line too long (82 > 79 characters)	reviewbot	March 21, 2021, 4:56 p.m.
F841 local variable 'e' is assigned to but never used	reviewbot	March 21, 2021, 4:56 p.m.
E501 line too long (82 > 79 characters)	reviewbot	March 21, 2021, 4:56 p.m.
E501 line too long (82 > 79 characters)	reviewbot	March 21, 2021, 4:56 p.m.
E501 line too long (166 > 79 characters)	reviewbot	March 21, 2021, 4:56 p.m.
E501 line too long (170 > 79 characters)	reviewbot	March 21, 2021, 4:56 p.m.
E501 line too long (161 > 79 characters)	reviewbot	March 21, 2021, 4:56 p.m.
E501 line too long (157 > 79 characters)	reviewbot	March 21, 2021, 4:56 p.m.
E501 line too long (127 > 79 characters)	reviewbot	March 21, 2021, 4:56 p.m.
E501 line too long (118 > 79 characters)	reviewbot	March 21, 2021, 4:56 p.m.
E501 line too long (145 > 79 characters)	reviewbot	March 21, 2021, 4:56 p.m.
E501 line too long (146 > 79 characters)	reviewbot	March 21, 2021, 4:56 p.m.
E501 line too long (153 > 79 characters)	reviewbot	March 21, 2021, 4:56 p.m.
E501 line too long (146 > 79 characters)	reviewbot	March 21, 2021, 4:56 p.m.
E501 line too long (111 > 79 characters)	reviewbot	March 21, 2021, 4:56 p.m.
Can you add a Version Added and Type (in that order)?	chipx86	March 21, 2021, 9:59 p.m.
Can we make this a list of strings instead? I feel like this would be a bit nicer.	chipx86	March 21, 2021, 11:08 p.m.
Can you add Version Added?	chipx86	March 21, 2021, 10 p.m.
Can you add Version Added?	chipx86	March 21, 2021, 10 p.m.
Can you add Deprecated in here?	chipx86	March 21, 2021, 10:04 p.m.
Can we move these private methods after the public ones?	chipx86	March 21, 2021, 10:06 p.m.
This needs to be a full class path.	chipx86	March 21, 2021, 10:07 p.m.
No blank line here.	chipx86	March 21, 2021, 10:07 p.m.
This also needs to be a full class path.	chipx86	March 21, 2021, 10:07 p.m.
Can you add Deprecated here, and any other methods that emit a deprecation warning?	chipx86	March 21, 2021, 10:08 p.m.
The pattern I've been moving to is to import kgb directly, so we don't have to bother updating the import …	chipx86	March 21, 2021, 10:10 p.m.
E501 line too long (166 > 79 characters)	reviewbot	March 21, 2021, 4:57 p.m.
Review Bot is going to complain about this forever. We could maybe do: ('http://...... '......'): { .... }, ... Also, …	chipx86	March 21, 2021, 10:29 p.m.
E501 line too long (170 > 79 characters)	reviewbot	March 21, 2021, 4:57 p.m.
E501 line too long (161 > 79 characters)	reviewbot	March 21, 2021, 4:57 p.m.
E501 line too long (157 > 79 characters)	reviewbot	March 21, 2021, 4:57 p.m.
E501 line too long (127 > 79 characters)	reviewbot	March 21, 2021, 4:57 p.m.
E501 line too long (118 > 79 characters)	reviewbot	March 21, 2021, 4:57 p.m.
While here, want to move to @self.spy_for(urlopen)?	chipx86	March 21, 2021, 10:30 p.m.
Can we move this to the top? Or is this leftover debugging?	chipx86	March 21, 2021, 10:30 p.m.
Can you add Version Added and Type (in that order)?	chipx86	March 21, 2021, 11:05 p.m.
Can these be keyword arguments?	chipx86	March 21, 2021, 11:06 p.m.
The description shouldn't be indented.	chipx86	March 21, 2021, 11:06 p.m.
Can you add a Version Added?	chipx86	March 21, 2021, 11:07 p.m.
Missing a trailing comma.	chipx86	March 21, 2021, 11:07 p.m.
Alternatively: query.pop('path', None)	chipx86	March 21, 2021, 11:07 p.m.
To render correctly, this needs to be above Args, so it fits with the doc body.	chipx86	March 21, 2021, 10:41 p.m.
I like the , deprecated. We haven't done that before, but I think that's a great tag. We should put …	chipx86	March 21, 2021, 10:58 p.m.
A bunch of args (these onward) are documented as non-optional, but have a preset value. I assume this is because …	chipx86	March 21, 2021, 11:04 p.m.
This will also need to move above Args.	chipx86	March 21, 2021, 10:41 p.m.
For new code, let's do import kgb and then reference kgb.SpyAgency. This makes it easy to later use kgb.SpyOpReturn or …	chipx86	March 21, 2021, 10:57 p.m.
No need for parens here, or in the ones below.	chipx86	March 21, 2021, 10:51 p.m.
E501 line too long (145 > 79 characters)	reviewbot	March 21, 2021, 4:57 p.m.
E501 line too long (146 > 79 characters)	reviewbot	March 21, 2021, 4:57 p.m.
E501 line too long (153 > 79 characters)	reviewbot	March 21, 2021, 4:57 p.m.
E501 line too long (146 > 79 characters)	reviewbot	March 21, 2021, 4:57 p.m.
E501 line too long (111 > 79 characters)	reviewbot	March 21, 2021, 4:57 p.m.
Let's do: @self.spy_for(urlopen)	chipx86	March 21, 2021, 10:57 p.m.
Shouldn't these all say "Testing get_repository_resource"?	chipx86	March 21, 2021, 10:57 p.m.
E128 continuation line under-indented for visual indent	reviewbot	March 21, 2021, 11:16 p.m.
E128 continuation line under-indented for visual indent	reviewbot	March 21, 2021, 11:16 p.m.
E128 continuation line under-indented for visual indent	reviewbot	March 21, 2021, 11:15 p.m.
E128 continuation line under-indented for visual indent	reviewbot	March 21, 2021, 11:15 p.m.
E128 continuation line under-indented for visual indent	reviewbot	March 21, 2021, 11:15 p.m.
E128 continuation line under-indented for visual indent	reviewbot	March 21, 2021, 11:15 p.m.
E128 continuation line under-indented for visual indent	reviewbot	March 21, 2021, 11:15 p.m.
E128 continuation line under-indented for visual indent	reviewbot	March 21, 2021, 11:15 p.m.
Before "Args". "Args" and "Returns" are special, and must come after.	chipx86	March 28, 2021, 10:34 a.m.
E127 continuation line over-indented for visual indent	reviewbot	March 21, 2021, 11:40 p.m.
E127 continuation line over-indented for visual indent	reviewbot	March 21, 2021, 11:40 p.m.
E127 continuation line over-indented for visual indent	reviewbot	March 21, 2021, 11:40 p.m.
E127 continuation line over-indented for visual indent	reviewbot	March 21, 2021, 11:40 p.m.
E127 continuation line over-indented for visual indent	reviewbot	March 21, 2021, 11:40 p.m.
E127 continuation line over-indented for visual indent	reviewbot	March 21, 2021, 11:40 p.m.
This needs to move before Returns, since it'll be part of the description text.	chipx86	March 23, 2021, 11:18 p.m.
Same here.	chipx86	March 23, 2021, 11:18 p.m.
Can you update this to use the full class path?	chipx86	March 23, 2021, 11:19 p.m.
Missing space before "In".	chipx86	March 23, 2021, 11:19 p.m.
Looking at the latest revision, do we still have any reason to default api_client and api_root to None?	chipx86	March 23, 2021, 11:19 p.m.

flake8 failed.

JSHint passed.

flake8

rbtools/clients/__init__.py (Diff revision 1)
Show all issues
```
E501 line too long (82 > 79 characters)
```
rbtools/clients/__init__.py (Diff revision 1)
Show all issues
```
E501 line too long (82 > 79 characters)
```
rbtools/clients/clearcase.py (Diff revision 1)
Show all issues
```
E501 line too long (82 > 79 characters)
```
rbtools/clients/clearcase.py (Diff revision 1)
Show all issues
```
E501 line too long (82 > 79 characters)
```

rbtools/clients/clearcase.py (Diff revision 1)

Show all issues

F841 local variable 'e' is assigned to but never used

rbtools/clients/svn.py (Diff revision 1)
Show all issues
```
E501 line too long (82 > 79 characters)
```
rbtools/clients/svn.py (Diff revision 1)
Show all issues
```
E501 line too long (82 > 79 characters)
```
rbtools/clients/tests/test_svn.py (Diff revision 1)
Show all issues
```
E501 line too long (166 > 79 characters)
```
rbtools/clients/tests/test_svn.py (Diff revision 1)
Show all issues
```
E501 line too long (170 > 79 characters)
```
rbtools/clients/tests/test_svn.py (Diff revision 1)
Show all issues
```
E501 line too long (161 > 79 characters)
```
rbtools/clients/tests/test_svn.py (Diff revision 1)
Show all issues
```
E501 line too long (157 > 79 characters)
```
rbtools/clients/tests/test_svn.py (Diff revision 1)
Show all issues
```
E501 line too long (127 > 79 characters)
```
rbtools/clients/tests/test_svn.py (Diff revision 1)
Show all issues
```
E501 line too long (118 > 79 characters)
```
rbtools/utils/tests/test_repository.py (Diff revision 1)
Show all issues
```
E501 line too long (145 > 79 characters)
```
rbtools/utils/tests/test_repository.py (Diff revision 1)
Show all issues
```
E501 line too long (146 > 79 characters)
```
rbtools/utils/tests/test_repository.py (Diff revision 1)
Show all issues
```
E501 line too long (153 > 79 characters)
```
rbtools/utils/tests/test_repository.py (Diff revision 1)
Show all issues
```
E501 line too long (146 > 79 characters)
```
rbtools/utils/tests/test_repository.py (Diff revision 1)
Show all issues
```
E501 line too long (111 > 79 characters)
```

Commits:

	Summary
-		Update and streamline remote repository detection.
+		Update and streamline remote repository detection.

Diff:

Revision 2 (+1874 -776)

Show changes

	rbtools/clients/__init__.py
	rbtools/clients/bazaar.py
	rbtools/clients/clearcase.py
	rbtools/clients/cvs.py
	rbtools/clients/git.py
	rbtools/clients/mercurial.py
	rbtools/clients/perforce.py
	rbtools/clients/plastic.py
	14 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbtools/clients/tests/test_svn.py (Diff revision 2)
Show all issues
```
E501 line too long (166 > 79 characters)
```
rbtools/clients/tests/test_svn.py (Diff revision 2)
Show all issues
```
E501 line too long (170 > 79 characters)
```
rbtools/clients/tests/test_svn.py (Diff revision 2)
Show all issues
```
E501 line too long (161 > 79 characters)
```
rbtools/clients/tests/test_svn.py (Diff revision 2)
Show all issues
```
E501 line too long (157 > 79 characters)
```
rbtools/clients/tests/test_svn.py (Diff revision 2)
Show all issues
```
E501 line too long (127 > 79 characters)
```
rbtools/clients/tests/test_svn.py (Diff revision 2)
Show all issues
```
E501 line too long (118 > 79 characters)
```
rbtools/utils/tests/test_repository.py (Diff revision 2)
Show all issues
```
E501 line too long (145 > 79 characters)
```
rbtools/utils/tests/test_repository.py (Diff revision 2)
Show all issues
```
E501 line too long (146 > 79 characters)
```
rbtools/utils/tests/test_repository.py (Diff revision 2)
Show all issues
```
E501 line too long (153 > 79 characters)
```
rbtools/utils/tests/test_repository.py (Diff revision 2)
Show all issues
```
E501 line too long (146 > 79 characters)
```
rbtools/utils/tests/test_repository.py (Diff revision 2)
Show all issues
```
E501 line too long (111 > 79 characters)
```

David, this is great. It really came together.

My comments are almost entirely doc-related, with a couple small suggestions here and there. The overall architecture, what an improvement.

rbtools/clients/__init__.py (Diff revision 2)

Show all issues

Can you add a Version Added and Type (in that order)?

rbtools/clients/__init__.py (Diff revision 2)

Show all issues

Can we make this a list of strings instead? I feel like this would be a bit nicer.

david March 21, 2021, 11:08 p.m.

The vast majority of clients will only ever have one name in here, and the only use is for sticking into the API request. I'd rather keep it slightly uglier for the definitions (in the rare case where a single client class in RBTools can map to multiple client classes in Review Board) and avoid having to join a list when we make the API requests.

rbtools/clients/__init__.py (Diff revision 2)
Show all issues
```
Can you add Version Added?
```
rbtools/clients/__init__.py (Diff revision 2)
Show all issues
```
Can you add Version Added?
```
rbtools/clients/__init__.py (Diff revision 2)
Show all issues
```
Can you add Deprecated in here?
```

rbtools/clients/clearcase.py (Diff revision 2)

Show all issues

Can we move these private methods after the public ones?

rbtools/clients/clearcase.py (Diff revision 2)
Show all issues
```
This needs to be a full class path.
```
rbtools/clients/clearcase.py (Diff revision 2)
Show all issues
```
No blank line here.
```
rbtools/clients/clearcase.py (Diff revision 2)
Show all issues
```
This also needs to be a full class path.
```

rbtools/clients/clearcase.py (Diff revision 2)

Show all issues

Can you add Deprecated here, and any other methods that emit a deprecation warning?

rbtools/clients/tests/test_svn.py (Diff revision 2)

Show all issues

The pattern I've been moving to is to import kgb directly, so we don't have to bother updating the import list to use any specific kgb tooling.

rbtools/clients/tests/test_svn.py (Diff revision 2)

Show all issues

Review Bot is going to complain about this forever. We could maybe do:

('http://......
 '......'): {

    ....
},
...

Also, maybe take the base URL out of these and put them into a variable to both shorten these and make that part more contextually apparent?

rbtools/clients/tests/test_svn.py (Diff revision 2)
Show all issues
```
While here, want to move to @self.spy_for(urlopen)?
```

rbtools/clients/tests/test_svn.py (Diff revision 2)

Show all issues

Can we move this to the top? Or is this leftover debugging?

rbtools/commands/__init__.py (Diff revision 2)

Show all issues

Can you add Version Added and Type (in that order)?

rbtools/commands/__init__.py (Diff revision 2)
Show all issues
```
Can these be keyword arguments?
```

rbtools/commands/stamp.py (Diff revision 2)

Show all issues

The description shouldn't be indented.

rbtools/utils/repository.py (Diff revision 2)
Show all issues
```
Can you add a Version Added?
```
rbtools/utils/repository.py (Diff revision 2)
Show all issues
```
Missing a trailing comma.
```

rbtools/utils/repository.py (Diff revision 2)

Show all issues

Alternatively:

query.pop('path', None)

rbtools/utils/review_request.py (Diff revision 2)

Show all issues

To render correctly, this needs to be above Args, so it fits with the doc body.

rbtools/utils/review_request.py (Diff revision 2)

Show all issues

I like the , deprecated. We haven't done that before, but I think that's a great tag. We should put that in Notion.

rbtools/utils/review_request.py (Diff revision 2)

Show all issues

A bunch of args (these onward) are documented as non-optional, but have a preset value. I assume this is because of the deprecated parameters preceding them. We should make sure to enforce any that are indeed required through checks at the start of the function.

rbtools/utils/review_request.py (Diff revision 2)
Show all issues
```
This will also need to move above Args.
```

rbtools/utils/tests/test_repository.py (Diff revision 2)

Show all issues

For new code, let's do import kgb and then reference kgb.SpyAgency. This makes it easy to later use kgb.SpyOpReturn or other methods without extending the import list.

rbtools/utils/tests/test_repository.py (Diff revision 2)
Show all issues
```
No need for parens here, or in the ones below.
```

rbtools/utils/tests/test_repository.py (Diff revision 2)

Show all issues

Let's do:

@self.spy_for(urlopen)

rbtools/utils/tests/test_repository.py (Diff revision 2)
Show all issues
```
Shouldn't these all say "Testing get_repository_resource"?
```

Commits:

	Summary
-		Update and streamline remote repository detection.
+		Update and streamline remote repository detection.

Diff:

Revision 3 (+2358 -1112)

Show changes

	rbtools/clients/__init__.py
	rbtools/clients/bazaar.py
	rbtools/clients/clearcase.py
	rbtools/clients/cvs.py
	rbtools/clients/git.py
	rbtools/clients/mercurial.py
	rbtools/clients/perforce.py
	rbtools/clients/plastic.py
	14 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbtools/utils/tests/test_repository.py (Diff revision 3)
Show all issues
```
E128 continuation line under-indented for visual indent
```
rbtools/utils/tests/test_repository.py (Diff revision 3)
Show all issues
```
E128 continuation line under-indented for visual indent
```
rbtools/utils/tests/test_repository.py (Diff revision 3)
Show all issues
```
E128 continuation line under-indented for visual indent
```
rbtools/utils/tests/test_repository.py (Diff revision 3)
Show all issues
```
E128 continuation line under-indented for visual indent
```
rbtools/utils/tests/test_repository.py (Diff revision 3)
Show all issues
```
E128 continuation line under-indented for visual indent
```
rbtools/utils/tests/test_repository.py (Diff revision 3)
Show all issues
```
E128 continuation line under-indented for visual indent
```
rbtools/utils/tests/test_repository.py (Diff revision 3)
Show all issues
```
E128 continuation line under-indented for visual indent
```
rbtools/utils/tests/test_repository.py (Diff revision 3)
Show all issues
```
E128 continuation line under-indented for visual indent
```

Commits:

	Summary
-		Update and streamline remote repository detection.
+		Update and streamline remote repository detection.

Diff:

Revision 4 (+2358 -1112)

Show changes

	rbtools/clients/__init__.py
	rbtools/clients/bazaar.py
	rbtools/clients/clearcase.py
	rbtools/clients/cvs.py
	rbtools/clients/git.py
	rbtools/clients/mercurial.py
	rbtools/clients/perforce.py
	rbtools/clients/plastic.py
	14 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbtools/clients/tests/test_svn.py (Diff revision 4)
Show all issues
```
E127 continuation line over-indented for visual indent
```
rbtools/clients/tests/test_svn.py (Diff revision 4)
Show all issues
```
E127 continuation line over-indented for visual indent
```
rbtools/clients/tests/test_svn.py (Diff revision 4)
Show all issues
```
E127 continuation line over-indented for visual indent
```
rbtools/clients/tests/test_svn.py (Diff revision 4)
Show all issues
```
E127 continuation line over-indented for visual indent
```
rbtools/clients/tests/test_svn.py (Diff revision 4)
Show all issues
```
E127 continuation line over-indented for visual indent
```
rbtools/clients/tests/test_svn.py (Diff revision 4)
Show all issues
```
E127 continuation line over-indented for visual indent
```

Commits:

	Summary
-		Update and streamline remote repository detection.
+		Update and streamline remote repository detection.

Diff:

Revision 5 (+2358 -1112)

Show changes

	rbtools/clients/__init__.py
	rbtools/clients/bazaar.py
	rbtools/clients/clearcase.py
	rbtools/clients/cvs.py
	rbtools/clients/git.py
	rbtools/clients/mercurial.py
	rbtools/clients/perforce.py
	rbtools/clients/plastic.py
	14 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

rbtools/clients/__init__.py (Diff revision 5)

Show all issues

This needs to move before Returns, since it'll be part of the description text.

rbtools/clients/__init__.py (Diff revision 5)
Show all issues
```
Same here.
```
rbtools/commands/stamp.py (Diff revision 5)
Show all issues
```
Can you update this to use the full class path?
```
rbtools/utils/repository.py (Diff revision 5)
Show all issues
```
Missing space before "In".
```

rbtools/utils/review_request.py (Diff revision 5)

Show all issues

Looking at the latest revision, do we still have any reason to default api_client and api_root to None?

Commits:

	Summary
-		Update and streamline remote repository detection.
+		Update and streamline remote repository detection.

Diff:

Revision 6 (+2336 -1090)

Show changes

	rbtools/clients/__init__.py
	rbtools/clients/bazaar.py
	rbtools/clients/clearcase.py
	rbtools/clients/cvs.py
	rbtools/clients/git.py
	rbtools/clients/mercurial.py
	rbtools/clients/perforce.py
	rbtools/clients/plastic.py
	14 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

rbtools/clients/__init__.py (Diff revisions 4 - 6)

Show all issues

Before "Args". "Args" and "Returns" are special, and must come after.

Status: Closed (submitted)

Change Summary:

Pushed to master (3fbea5f)