Make repository select the deepest repository in the directory tree

Review Request #9620 — Created Feb. 11, 2018 and updated

maram
RBTools
release-0.7.x
7a16851...
rbtools, students

Without the presence of a .reviewboardrc file which defines
REPOSITORY_TYPE, RBTools will try to auto-detect the repository type by running
various commands like "svn info", "hg root", etc. This will run through each
repository type until one is found that works.

Unfortunately, if people have nested repositories, this means
RBTools can select the wrong one. For example, some people may have
/home/user/ as a git directory, and /home/user/src/project/ is a subversion
checkout, running "rbt" within their "project" directory may detect it as git
instead of svn.

This change auto-detects all repository types, and then selects the
deepest one. In the case of multiple matching repositories, we output a
warning suggesting that people define REPOSITORY_TYPE in .reviewboardrc in
order to speed up detection.

  • Tested combinations of nested svn, git, hg repos.
  • All unit tests pass.
  • 4
  • 0
  • 25
  • 83
  • 112
Description From Last Updated
Hmmm. We should probably just make sure that get_repository_info doesn't change things. Needing a special case here is ugly. david david
Also not necessary if we fix up GitClient. david david
More fallout from GitClient being dumb. david david
And here. david david
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

Review request changed

Change Summary:

Check if git changes the CWD, roll back until other clients find their repos, and then find deepest repo. Cannot stop gitClient from changing CWD as that ensures diffs on server are not messed up. Check if git changes the CWD, roll back until other clients find their repos, and then find deepest. Cannot stop gitClient from changing CWD as that ensures diffs on server are not messed up.

Diff:

Revision 2 (+186 -72)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Review request changed

Description:

   

Without the presence of a .reviewboardrc file which defines REPOSITORY_TYPE, RBTools will try to auto-detect the repository type by running various commands like "svn info", "hg root", etc. This will run through each repository type until one is found that works.

   
~  

Unfortunately, if people have nested repositories, this means RBTools can select the wrong one. For example, some people may have /home/user/ as a git directory, and /home/user/src/project/ is a subversion checkout, running "rbt" within their "project" directory may detect it as git instead of svn. We'd like to auto-detect all repository types, and then select the deepest one.

  ~

Unfortunately, if people have nested repositories, this means RBTools can select the wrong one. For example, some people may have /home/user/ as a git directory, and /home/user/src/project/ is a subversion checkout, running "rbt" within their "project" directory may detect it as git instead of svn. This change auto-detects all repository types, and then selects the deepest one.

   
~  

In the case of multiple matching repositories, we may want to output a warning suggesting that people define REPOSITORY_TYPE in .reviewboardrc in order to speed up detection.

  ~

In the case of multiple matching repositories, we output a warning suggesting that people define REPOSITORY_TYPE in .reviewboardrc in order to speed up detection.

Commit:

-1b9c14f1524fb0f4d12834f8fdfef956bddc54e1
+33dde16cf2afda0204792f8e01a38f75631e8470

Diff:

Revision 3 (+247 -117)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Review request changed

Change Summary:

Rebased the branch on release-0.7.x to avoid rbt post error after new changes on master were pulled.

Testing Done:

~  
  • Tested combinations of nested svn and git repos, both were unaffected by this issue.
  ~
  • Tested combinations of nested svn, git, hg repos
-  
  • Tested mercurial repo nested in a git repo.

Branch:

-deepestrepo
+release-0.7.x

Commit:

-33dde16cf2afda0204792f8e01a38f75631e8470
+6996a3346f42ac105c9025ef413ea7646f9a7e71

Diff:

Revision 4 (+102 -46)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Review request changed

Testing Done:

~  
  • Tested combinations of nested svn, git, hg repos
  ~
  • Tested combinations of nested svn, git, hg repos.

Diff:

Revision 6 (+103 -46)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Review request changed

Summary:

-[WIP] Make repository select the deepest repository in the directory tree
+Make repository select the deepest repository in the directory tree

Testing Done:

   
  • Tested combinations of nested svn, git, hg repos.
  +
  • All unit tests pass.

Diff:

Revision 7 (+104 -46)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
  1. 
      
  2. Can you wrap your description to 70 columns?

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

    These should be in alphabetical order.

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

    Should be "self.path". Even though it's starting a sentence, self refers to a symbol.

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

    There's an extra space at the beginning of this comment line.

  6. rbtools/clients/__init__.py (Diff revision 8)
     
     
     

    Add a blank line between these (we like to have a bit of space before and after blocks)

  7. rbtools/clients/__init__.py (Diff revision 8)
     
     
     

    Add a blank line between these.

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

    Hmmm. We should probably just make sure that get_repository_info doesn't change things. Needing a special case here is ugly.

    1. Should I create a separate function to handle chdir for GitClient and have all calls to GitCleint.get_repository_info() call that function as well?

      (GitClient.get_repository_info() has been doing the directory change for quite some time (https://reviews.reviewboard.org/r/2960/diff/1/#))

  9. rbtools/clients/__init__.py (Diff revision 8)
     
     
     

    Add a blank line between these.

  10. rbtools/clients/__init__.py (Diff revision 8)
     
     
     

    We generally prefer to have the joining space at the end of the previous line instead of the beginning of the next.

  11. rbtools/clients/__init__.py (Diff revision 8)
     
     
     

    Blank line between these.

  12. rbtools/clients/__init__.py (Diff revision 8)
     
     
     

    Blank line between these.

  13. rbtools/clients/__init__.py (Diff revision 8)
     
     
     

    Blank line between these.

  14. rbtools/clients/__init__.py (Diff revision 8)
     
     
     

    Also not necessary if we fix up GitClient.

  15. rbtools/clients/__init__.py (Diff revision 8)
     
     

    This looks like leftover debug output. Can we get rid of it?

    1. I thought it would be helpful to have that as part of the output when -debug is specified... Unneccessary? Better phrasing maybe?

  16. rbtools/clients/__init__.py (Diff revision 8)
     
     

    More fallout from GitClient being dumb.

  17. rbtools/clients/__init__.py (Diff revision 8)
     
     
     
     
     

    And here.

  18. rbtools/clients/cvs.py (Diff revision 8)
     
     
     

    Blank line between these.

  19. rbtools/clients/git.py (Diff revision 8)
     
     
     

    Too many blank lines here now.

  20. rbtools/clients/git.py (Diff revision 8)
     
     
     

    Not enough blank lines here now.

  21. rbtools/clients/git.py (Diff revision 8)
     
     
     

    Blank line here.

  22. rbtools/clients/git.py (Diff revision 8)
     
     
     

    Blank line here.

  23. rbtools/clients/perforce.py (Diff revision 8)
     
     
     

    Blank line here.

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

    Blank line here.

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

    This should fit all on one line.

  26. 
      
  1. 
      
  2. rbtools/clients/git.py (Diff revision 8)
     
     

    I think this function might be a bit too long. Might be a good idea to break it up, for example some of the if/branches can be separate functions.

  3. rbtools/clients/git.py (Diff revision 8)
     
     

    Would it make sense to move all of these Regexes out into one place and a) have them organized by version ranges (e.x. git may change these in some version) and b) compile them

  4. 
      
jshephard
  1. 
      
  2. rbtools/clients/__init__.py (Diff revision 9)
     
     
     

    From my understanding, the idea here is to find the deepest repo in terms of directory depth right? Using the length of the path to determine this might result in selecting a repo that is in a directory with a large name over another repo that is deeper.

    I'm not sure if this is an issue here though, as your description makes it sound like you're just looking at parent directories of the user's current working directory. In that case this is a non-issue.

    1. Good insticts asking this question, but your caveat is correct--the selected directories will always be parents of the cwd.

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

    Does the >>>>>>>> mean anything or is it a part of the reviewboard UI?

    1. This is Review Board's diff viewer showing that the code was indented but otherwise unchanged.

  3. rbtools/clients/git.py (Diff revision 9)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Same comment as above. I'm not sure if the chevrons are appearing as part of the UI or not.

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

    This would wrap more nicely as:

    if (repo.local_root and
        len(os.path.normpath(repo.local_root)) > deepest_repo):
    
  3. rbtools/clients/__init__.py (Diff revision 9)
     
     

    Please use single quotes instead of double, and add a blank line above this.

  4. 
      
  1. 
      
  2. rbtools/clients/__init__.py (Diff revision 9)
     
     

    Oh I see! That makes a lot more sense. Thank you!

  3. 
      
Review request changed
Loading...