Update and streamline local repository type detection.

Review Request #11503 — Created March 4, 2021 and submitted

Information

RBTools
master

Reviewers

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.
  • Ran unit tests.
  • Smoke tested tools that use the repository.
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
Description From Last Updated

E501 line too long (83 > 79 characters)

reviewbotreviewbot

Realizing it's kind of weird that we've been doing this based on path name length, and not number of directory …

chipx86chipx86

While here, can we change these to logging.warning?

chipx86chipx86

"Return ..."

chipx86chipx86

We can just return None at the end. Could even merge in the client_root check above with the client_root.lower() check, …

chipx86chipx86

We can skip the else: here, just return None.

chipx86chipx86

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

chipx86chipx86

No need for parens.

chipx86chipx86

We can skip the else: and just return None.

chipx86chipx86

Totally your call here, but since we're almost certainly going to hvae a result from this call, if info is …

chipx86chipx86

Is this here so that Git and Mercurial can get access to it? Given how similar the name is to …

chipx86chipx86

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

chipx86chipx86

No need for the else:.

chipx86chipx86

We have a few versions of the docstring for this function. Let's copy/paste one summary for all of these, at …

chipx86chipx86

No need for the else:.

chipx86chipx86

No need for the else:.

chipx86chipx86

No need for the else:.

chipx86chipx86

No need for the else:.

chipx86chipx86

Why the direct comparison? Is None a valid option?

chipx86chipx86

Can you add Version Added?

chipx86chipx86

Can you add Version Added?

chipx86chipx86

There's a danger with this line (and any like it). One of the warts of Python. >>> d = {} …

chipx86chipx86

This could be: local_path = info.get('Working Copy Root Path')

chipx86chipx86

Can this use the new deprecated class?

chipx86chipx86

These still have to go before Returns, in order to render correctly.

chipx86chipx86

Same here.

chipx86chipx86
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

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

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

    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
    
    1. 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.

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

    While here, can we change these to logging.warning?

  4. rbtools/clients/git.py (Diff revision 2)
     
     

    "Return ..."

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

    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.

  6. rbtools/clients/plastic.py (Diff revision 2)
     
     
     

    We can skip the else: here, just return None.

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

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

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

    No need for parens.

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

    We can skip the else: and just return None.

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

    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
    
  11. rbtools/clients/svn.py (Diff revision 2)
     
     

    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.

    1. 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.

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

    Can you add a blank line between these?

    Any reason we can't import this at the top of the file?

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

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

    No need for the else:.

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

    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.

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

    No need for the else:.

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

    No need for the else:.

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

    No need for the else:.

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

    No need for the else:.

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

    Why the direct comparison? Is None a valid option?

  20. 
      
david
david
chipx86
  1. 
      
  2. rbtools/clients/__init__.py (Diff revision 4)
     
     
     
     
     
     

    Can you add Version Added?

  3. rbtools/clients/__init__.py (Diff revision 4)
     
     
     
     
     

    Can you add Version Added?

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

    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.

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

    This could be:

    local_path = info.get('Working  Copy Root Path')
    
  6. rbtools/commands/__init__.py (Diff revision 4)
     
     
     
     
     

    Can this use the new deprecated class?

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

  7. 
      
david
chipx86
  1. 
      
  2. rbtools/clients/__init__.py (Diff revision 5)
     
     
     

    These still have to go before Returns, in order to render correctly.

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

    Same here.

  4. 
      
david
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (53b7208)
Loading...