Summary

Update and streamline local repository type detection.

Review Request #11503 — Created March 4, 2021 and submitted March 23, 2021, 10:47 p.m.

Information

Owner

david

Repository

RBTools

Branch

master

Bugs

Depends On

Reviewers

Groups

rbtools

People

Description

The way that the local repository type was detected overly complicated
and inefficient. We would go through and assemble a complete
RepositoryInfo for each possible repository, potentially doing excess
computation that would just be thrown away. In addition, there were some
situtations where a repository would be operating in a remote-only mode
(for example, using SVN with the --repository-url option) where we
were scanning everything locally before finally using the remote.

This change reworks the local repository detection in RBTools to
work in three phases:

First we ask each tool if it's operating in "remote only" mode. At
the moment, SVN is the only such tool, but it's easy to imagine other
server-heavy services gaining this ability.
Second, we scan through each tool to see if it thinks there's an
active repository. This has been simplified to only fetch the local
working directory from the tool, rather than the full repository
info.
Finally, after having chosen a tool, we ask it for the repository
info structure.

Testing Done


Ran unit tests.
Smoke tested tools that use the repository.

Commits

Summary	ID
Update and streamline local repository type detection. The way that the local repository type was detected overly complicated and inefficient. We would go through and assemble a complete `RepositoryInfo` for each possible repository, potentially doing excess computation that would just be thrown away. In addition, there were some situtations where a repository would be operating in a remote-only mode (for example, using SVN with the `--repository-url` option) where we were scanning everything locally before finally using the remote. This change reworks the local repository detection in RBTools to work in three phases: 1. First we ask each tool if it's operating in "remote only" mode. At the moment, SVN is the only such tool, but it's easy to imagine other server-heavy services gaining this ability. 2. Second, we scan through each tool to see if it thinks there's an active repository. This has been simplified to only fetch the local working directory from the tool, rather than the full repository info. 3. Finally, after having chosen a tool, we ask it for the repository info structure. Testing Done: - Ran unit tests. - Smoke tested tools that use the repository.	a4ce2a41da89265ae7b025fd02327baeb3453186

Summary

Update and streamline local repository type detection.

The way that the local repository type was detected overly complicated and inefficient. We would go through and assemble a complete `RepositoryInfo` for each possible repository, potentially doing excess computation that would just be thrown away. In addition, there were some situtations where a repository would be operating in a remote-only mode (for example, using SVN with the `--repository-url` option) where we were scanning everything locally before finally using the remote. This change reworks the local repository detection in RBTools to work in three phases: 1. First we ask each tool if it's operating in "remote only" mode. At the moment, SVN is the only such tool, but it's easy to imagine other server-heavy services gaining this ability. 2. Second, we scan through each tool to see if it thinks there's an active repository. This has been simplified to only fetch the local working directory from the tool, rather than the full repository info. 3. Finally, after having chosen a tool, we ask it for the repository info structure. Testing Done: - Ran unit tests. - Smoke tested tools that use the repository.

a4ce2a41da89265ae7b025fd02327baeb3453186

Issues

Description	From	Last Updated
E501 line too long (83 > 79 characters)	reviewbot	March 4, 2021, 9:57 a.m.
Realizing it's kind of weird that we've been doing this based on path name length, and not number of directory …	chipx86	March 11, 2021, 2:02 p.m.
While here, can we change these to logging.warning?	chipx86	March 11, 2021, 1:20 p.m.
"Return ..."	chipx86	March 11, 2021, 1:20 p.m.
We can just return None at the end. Could even merge in the client_root check above with the client_root.lower() check, …	chipx86	March 11, 2021, 1:21 p.m.
We can skip the else: here, just return None.	chipx86	March 11, 2021, 1:22 p.m.
Since this is the Subversion code, we can probably make these docs more specific to Subversion's behavior.	chipx86	March 11, 2021, 1:23 p.m.
No need for parens.	chipx86	March 11, 2021, 1:23 p.m.
We can skip the else: and just return None.	chipx86	March 11, 2021, 1:23 p.m.
Totally your call here, but since we're almost certainly going to hvae a result from this call, if info is …	chipx86	March 11, 2021, 1:25 p.m.
Is this here so that Git and Mercurial can get access to it? Given how similar the name is to …	chipx86	March 11, 2021, 1:51 p.m.
Can you add a blank line between these? Any reason we can't import this at the top of the file?	chipx86	March 11, 2021, 1:27 p.m.
No need for the else:.	chipx86	March 11, 2021, 1:27 p.m.
We have a few versions of the docstring for this function. Let's copy/paste one summary for all of these, at …	chipx86	March 11, 2021, 2:02 p.m.
No need for the else:.	chipx86	March 11, 2021, 1:27 p.m.
No need for the else:.	chipx86	March 11, 2021, 1:27 p.m.
No need for the else:.	chipx86	March 11, 2021, 1:28 p.m.
No need for the else:.	chipx86	March 11, 2021, 1:28 p.m.
Why the direct comparison? Is None a valid option?	chipx86	March 11, 2021, 1:28 p.m.
Can you add Version Added?	chipx86	March 21, 2021, 9:37 p.m.
Can you add Version Added?	chipx86	March 21, 2021, 9:38 p.m.
There's a danger with this line (and any like it). One of the warts of Python. >>> d = {} …	chipx86	March 21, 2021, 9:38 p.m.
This could be: local_path = info.get('Working Copy Root Path')	chipx86	March 21, 2021, 9:39 p.m.
Can this use the new deprecated class?	chipx86	March 21, 2021, 9:39 p.m.
These still have to go before Returns, in order to render correctly.	chipx86	March 23, 2021, 10:34 p.m.
Same here.	chipx86	March 23, 2021, 10:34 p.m.

flake8 failed.

JSHint passed.

flake8

rbtools/clients/git.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E501 line too long (83 > 79 characters)
```

Commits:

	Summary	ID
	Update and streamline local repository type detection. The way that the local repository type was detected overly complicated and inefficient. We would go through and assemble a complete `RepositoryInfo` for each possible repository, potentially doing excess computation that would just be thrown away. In addition, there were some situtations where a repository would be operating in a remote-only mode (for example, using SVN with the `--repository-url` option) where we were scanning everything locally before finally using the remote. This change reworks the local repository detection in RBTools to work in three phases: 1. First we ask each tool if it's operating in "remote only" mode. At the moment, SVN is the only such tool, but it's easy to imagine other server-heavy services gaining this ability. 2. Second, we scan through each tool to see if it thinks there's an active repository. This has been simplified to only fetch the local working directory from the tool, rather than the full repository info. 3. Finally, after having chosen a tool, we ask it for the repository info structure. Testing Done: - Ran unit tests. - Smoke tested tools that use the repository.	4d2c52134ea2e68d48b3708237b234c1b7b25bb6
	Update and streamline local repository type detection. The way that the local repository type was detected overly complicated and inefficient. We would go through and assemble a complete `RepositoryInfo` for each possible repository, potentially doing excess computation that would just be thrown away. In addition, there were some situtations where a repository would be operating in a remote-only mode (for example, using SVN with the `--repository-url` option) where we were scanning everything locally before finally using the remote. This change reworks the local repository detection in RBTools to work in three phases: 1. First we ask each tool if it's operating in "remote only" mode. At the moment, SVN is the only such tool, but it's easy to imagine other server-heavy services gaining this ability. 2. Second, we scan through each tool to see if it thinks there's an active repository. This has been simplified to only fetch the local working directory from the tool, rather than the full repository info. 3. Finally, after having chosen a tool, we ask it for the repository info structure. Testing Done: - Ran unit tests. - Smoke tested tools that use the repository.	e8523194c8288854a22ec91f99a81442aca7f6a6

Diff:

Revision 2 (+848 -442)

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
	4 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Design looks great. Most of my comments are about consistency in docs and some logic flows.

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

The issue has been dropped. Show all issues

Realizing it's kind of weird that we've been doing this based on path name length, and not number of directory components. Should we revisit that?

Right now, this:

/src/oh-boy-this-repository-name-is-long`

is considered deeper than:

/src/repos/libfoo/src

david March 10, 2021, 10:57 p.m.

So in practice, everything that this is comparing is some ancestor of the CWD, and so path length is totally fine. That said, I suppose it's possible that a client would report itself as "active" with a local root that's not related in any way to the CWD. Let me think about this.

rbtools/clients/__init__.py (Diff revision 2)
The issue has been resolved. Show all issues
```
While here, can we change these to logging.warning?
```
rbtools/clients/git.py (Diff revision 2)
The issue has been resolved. Show all issues
```
"Return ..."
```

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

The issue has been resolved. Show all issues

We can just return None at the end.
Could even merge in the client_root check above with the client_root.lower() check, and simplify it all further.

rbtools/clients/plastic.py (Diff revision 2)
The issue has been resolved. Show all issues
```
We can skip the else: here, just return None.
```

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

The issue has been resolved. Show all issues

Since this is the Subversion code, we can probably make these docs more specific to Subversion's behavior.

rbtools/clients/svn.py (Diff revision 2)
The issue has been resolved. Show all issues
```
No need for parens.
```
rbtools/clients/svn.py (Diff revision 2)
The issue has been resolved. Show all issues
```
We can skip the else: and just return None.
```

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

The issue has been resolved. Show all issues

Totally your call here, but since we're almost certainly going to hvae a result from this call, if info is set, we can skip the extra checks and do:

try:
    path = info['Repository Root']
    ...
except KeyError:
    return None

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

The issue has been resolved. Show all issues

Is this here so that Git and Mercurial can get access to it?

Given how similar the name is to the SCMClient version, maybe we should rename it. I was pretty confused for a second.

david March 11, 2021, 2:03 p.m.

I think this was actually just a typo as my cursor paused here, because I was trying to figure out if this is used anywhere. It's not.

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

The issue has been resolved. Show all issues

Can you add a blank line between these?
Any reason we can't import this at the top of the file?

david March 11, 2021, 2:03 p.m.

SCMCLIENTS is set lazily. Importing at the top of the file will always result in None.

rbtools/clients/tfs.py (Diff revision 2)
The issue has been resolved. Show all issues
```
No need for the else:.
```

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

The issue has been resolved. Show all issues

We have a few versions of the docstring for this function. Let's copy/paste one summary for all of these, at least, so they don't diverge further down the road.

rbtools/clients/tfs.py (Diff revision 2)
The issue has been resolved. Show all issues
```
No need for the else:.
```
rbtools/clients/tfs.py (Diff revision 2)
The issue has been resolved. Show all issues
```
No need for the else:.
```
rbtools/clients/tfs.py (Diff revision 2)
The issue has been resolved. Show all issues
```
No need for the else:.
```
rbtools/clients/tfs.py (Diff revision 2)
The issue has been resolved. Show all issues
```
No need for the else:.
```
rbtools/commands/__init__.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Why the direct comparison? Is None a valid option?
```

Commits:

	Summary	ID
	Update and streamline local repository type detection. The way that the local repository type was detected overly complicated and inefficient. We would go through and assemble a complete `RepositoryInfo` for each possible repository, potentially doing excess computation that would just be thrown away. In addition, there were some situtations where a repository would be operating in a remote-only mode (for example, using SVN with the `--repository-url` option) where we were scanning everything locally before finally using the remote. This change reworks the local repository detection in RBTools to work in three phases: 1. First we ask each tool if it's operating in "remote only" mode. At the moment, SVN is the only such tool, but it's easy to imagine other server-heavy services gaining this ability. 2. Second, we scan through each tool to see if it thinks there's an active repository. This has been simplified to only fetch the local working directory from the tool, rather than the full repository info. 3. Finally, after having chosen a tool, we ask it for the repository info structure. Testing Done: - Ran unit tests. - Smoke tested tools that use the repository.	e8523194c8288854a22ec91f99a81442aca7f6a6
	Update and streamline local repository type detection. The way that the local repository type was detected overly complicated and inefficient. We would go through and assemble a complete `RepositoryInfo` for each possible repository, potentially doing excess computation that would just be thrown away. In addition, there were some situtations where a repository would be operating in a remote-only mode (for example, using SVN with the `--repository-url` option) where we were scanning everything locally before finally using the remote. This change reworks the local repository detection in RBTools to work in three phases: 1. First we ask each tool if it's operating in "remote only" mode. At the moment, SVN is the only such tool, but it's easy to imagine other server-heavy services gaining this ability. 2. Second, we scan through each tool to see if it thinks there's an active repository. This has been simplified to only fetch the local working directory from the tool, rather than the full repository info. 3. Finally, after having chosen a tool, we ask it for the repository info structure. Testing Done: - Ran unit tests. - Smoke tested tools that use the repository.	213a192a6ed2f21901e6b575faf42c57735a9a91

Diff:

Revision 3 (+864 -502)

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
	4 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Fix remote-only mode with SVN.

Commits:

	Summary	ID
	Update and streamline local repository type detection. The way that the local repository type was detected overly complicated and inefficient. We would go through and assemble a complete `RepositoryInfo` for each possible repository, potentially doing excess computation that would just be thrown away. In addition, there were some situtations where a repository would be operating in a remote-only mode (for example, using SVN with the `--repository-url` option) where we were scanning everything locally before finally using the remote. This change reworks the local repository detection in RBTools to work in three phases: 1. First we ask each tool if it's operating in "remote only" mode. At the moment, SVN is the only such tool, but it's easy to imagine other server-heavy services gaining this ability. 2. Second, we scan through each tool to see if it thinks there's an active repository. This has been simplified to only fetch the local working directory from the tool, rather than the full repository info. 3. Finally, after having chosen a tool, we ask it for the repository info structure. Testing Done: - Ran unit tests. - Smoke tested tools that use the repository.	213a192a6ed2f21901e6b575faf42c57735a9a91
	Update and streamline local repository type detection. The way that the local repository type was detected overly complicated and inefficient. We would go through and assemble a complete `RepositoryInfo` for each possible repository, potentially doing excess computation that would just be thrown away. In addition, there were some situtations where a repository would be operating in a remote-only mode (for example, using SVN with the `--repository-url` option) where we were scanning everything locally before finally using the remote. This change reworks the local repository detection in RBTools to work in three phases: 1. First we ask each tool if it's operating in "remote only" mode. At the moment, SVN is the only such tool, but it's easy to imagine other server-heavy services gaining this ability. 2. Second, we scan through each tool to see if it thinks there's an active repository. This has been simplified to only fetch the local working directory from the tool, rather than the full repository info. 3. Finally, after having chosen a tool, we ask it for the repository info structure. Testing Done: - Ran unit tests. - Smoke tested tools that use the repository.	27c20a7913520016edf2a9fc9c0b3323e7c56567

Diff:

Revision 4 (+870 -500)

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
	4 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

rbtools/clients/__init__.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Can you add Version Added?
```
rbtools/clients/__init__.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Can you add Version Added?
```

rbtools/clients/svn.py (Diff revision 4)

The issue has been resolved. Show all issues

There's a danger with this line (and any like it). One of the warts of Python.


>>> d = {}
>>> print(d and 'key' in d)
{}
>>> n = None
>>> print(n and 'key' in n)
None


Due to the old-style ternary-ish statements Python allowed, we have to be very careful to explicitly compare each part of this if we're going to return it as a boolean. So, info is not None or bool(info) or whatever makes sense here.
I know we've done this wrong in places, and while it normally isn't an issue, I have fixed an obscure bug or two in the past due to this sort of quirk.
This might apply to other parts of the code. I haven't gone through carefully, just noticed this here.

rbtools/clients/svn.py (Diff revision 4)

The issue has been resolved. Show all issues

This could be:

local_path = info.get('Working  Copy Root Path')

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

The issue has been dropped. Show all issues

Can this use the new deprecated class?

david March 21, 2021, 9:40 p.m.

My change to add the deprecated class comes after this one (sorry for not setting depends-on correctly). That change fixes up this location.

Commits:

	Summary	ID
	Update and streamline local repository type detection. The way that the local repository type was detected overly complicated and inefficient. We would go through and assemble a complete `RepositoryInfo` for each possible repository, potentially doing excess computation that would just be thrown away. In addition, there were some situtations where a repository would be operating in a remote-only mode (for example, using SVN with the `--repository-url` option) where we were scanning everything locally before finally using the remote. This change reworks the local repository detection in RBTools to work in three phases: 1. First we ask each tool if it's operating in "remote only" mode. At the moment, SVN is the only such tool, but it's easy to imagine other server-heavy services gaining this ability. 2. Second, we scan through each tool to see if it thinks there's an active repository. This has been simplified to only fetch the local working directory from the tool, rather than the full repository info. 3. Finally, after having chosen a tool, we ask it for the repository info structure. Testing Done: - Ran unit tests. - Smoke tested tools that use the repository.	27c20a7913520016edf2a9fc9c0b3323e7c56567
	Update and streamline local repository type detection. The way that the local repository type was detected overly complicated and inefficient. We would go through and assemble a complete `RepositoryInfo` for each possible repository, potentially doing excess computation that would just be thrown away. In addition, there were some situtations where a repository would be operating in a remote-only mode (for example, using SVN with the `--repository-url` option) where we were scanning everything locally before finally using the remote. This change reworks the local repository detection in RBTools to work in three phases: 1. First we ask each tool if it's operating in "remote only" mode. At the moment, SVN is the only such tool, but it's easy to imagine other server-heavy services gaining this ability. 2. Second, we scan through each tool to see if it thinks there's an active repository. This has been simplified to only fetch the local working directory from the tool, rather than the full repository info. 3. Finally, after having chosen a tool, we ask it for the repository info structure. Testing Done: - Ran unit tests. - Smoke tested tools that use the repository.	a4ce2a41da89265ae7b025fd02327baeb3453186

Diff:

Revision 5 (+876 -500)

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
	4 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

rbtools/clients/__init__.py (Diff revision 5)
The issue has been resolved. Show all issues
```
These still have to go before Returns, in order to render correctly.
```
rbtools/clients/__init__.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Same here.
```

Status:: Completed
Change Summary:: Pushed to master (53b7208)