• 
      

    [WIP] SubGit support

    Review Request #7078 — Created March 18, 2015 and discarded

    Information

    RBTools
    master

    Reviewers

    RBTools currently supports interacting with reviews from a git-svn clone; it jumps through some hoops specific to git-svn to make everything happy from the perspective of the ReviewBoard API and backend SVN repo. However, it has no such support for SubGit, which is (IMHO) a far superior translation tool. This PR aims to provide that support, so that "rbt post" and other operations Just Work™ when the local git repo is a clone of a SubGit mirror.

    Assuming that you have a setup like this:

    [subgit-server] $ subgit configure --svn-url svn+ssh://svn.example.com/path/to/repo ./repo.git
    [subgit-server] $ $EDITOR repo.git/subgit/config repo.git/subgit/authors.txt
    [subgit-server] $ subgit install ./repo.git
    
    [some-client] $ git clone ssh://subgit-server.example.com/repo.git
    

    the following commands should work:

    [some-client] $ cd repo.git
    [some-client] $ gco feature/awesome
    [some-client] $ rbt diff
    [some-client] $ rbt post
    [some-client] $ gco feature/depends_on_awesome
    [some-client] $ rbt post --parent feature/awesome
    

    and possibly others, but that is most of my workflow (besides adding backend-agnostic options to the above commands, such as --diff-only or --change-description, etc).

    This PR is currently mostly for discussion value, as there are some significant known limitations:
    1) No tests (yet).
    - There were also no tests for git-svn when I started
    - I plan to run my new (forthcoming) tests against both kinds of translation

    2) Requires SSH access to the SubGit mirror host.
    - RBTools must inspect the SubGit configuration to determine the SVN URL.
    - There's a git config option you can set to specify it explicitly, but this is not currently documented, and it should probably be a .reviewboardrc option instead.

    3) It's obviously way behind master. I will rebase and re-verify once I have a bunch of tests.

    Also inviting the SubGit folks to participate in this discussion.

    No unit tests yet (see above), but I use it for my day-to-day development and code review. I've been using the current version for a month or two without any problems (especially since the most recent commits in which I fixed a bug with --parent).

    Description From Last Updated

    redefinition of unused 'StringIO' from line 11

    reviewbot reviewbot

    Col: 1 E302 expected 2 blank lines, found 1

    reviewbot reviewbot

    undefined name 'git_dir'

    reviewbot reviewbot

    Col: 80 E501 line too long (82 > 79 characters)

    reviewbot reviewbot

    Col: 25 E127 continuation line over-indented for visual indent

    reviewbot reviewbot

    Col: 55 E261 at least two spaces before inline comment

    reviewbot reviewbot

    Col: 80 E501 line too long (84 > 79 characters)

    reviewbot reviewbot

    Col: 38 E261 at least two spaces before inline comment

    reviewbot reviewbot

    Col: 80 E501 line too long (93 > 79 characters)

    reviewbot reviewbot

    Col: 80 E501 line too long (92 > 79 characters)

    reviewbot reviewbot

    Col: 80 E501 line too long (88 > 79 characters)

    reviewbot reviewbot

    Col: 80 E501 line too long (87 > 79 characters)

    reviewbot reviewbot

    Col: 80 E501 line too long (81 > 79 characters)

    reviewbot reviewbot

    Col: 80 E501 line too long (83 > 79 characters)

    reviewbot reviewbot

    Col: 80 E501 line too long (86 > 79 characters)

    reviewbot reviewbot

    Col: 80 E501 line too long (81 > 79 characters)

    reviewbot reviewbot

    Col: 80 E501 line too long (89 > 79 characters)

    reviewbot reviewbot

    Col: 80 E501 line too long (88 > 79 characters)

    reviewbot reviewbot

    Col: 80 E501 line too long (97 > 79 characters)

    reviewbot reviewbot

    Col: 80 E501 line too long (94 > 79 characters)

    reviewbot reviewbot

    Can you undo the changes to this file?

    brennie brennie

    Single quotes.

    brennie brennie

    Needs a docstring.

    brennie brennie

    Needs a docstring.

    brennie brennie

    single quotes

    brennie brennie

    Needs a docstring.

    brennie brennie

    Blank line between these.

    brennie brennie

    Blank line between these.

    brennie brennie

    Blank line before a block.

    brennie brennie

    Single quotes.

    brennie brennie

    Single quotes.

    brennie brennie

    Blank line between these.

    brennie brennie

    We are moving away from using die in all code. This should raise an exception. Also, single quotes.

    brennie brennie

    Needs a docstring.

    brennie brennie

    Needs a docstring.

    brennie brennie

    Single quotes.

    brennie brennie

    Blank line between these.

    brennie brennie

    Single quotes.

    brennie brennie

    Blank line between these.

    brennie brennie

    Blank line between these.

    brennie brennie

    Blank line between these.

    brennie brennie

    Blank line between these. Single quotes.

    brennie brennie

    Single quotes.

    brennie brennie

    Single quotes.

    brennie brennie

    Blank line between these.

    brennie brennie

    Single quotes.

    brennie brennie

    Instead of using memoize here, we can just cache this in a field on the class.

    brennie brennie

    Needs a docstring.

    brennie brennie

    Why not just config_svn_url = execute(['git', 'config', '--git', 'rbtools.subgit.svn_url'], ignore_errors=True, none_on_ignored_error=True)

    brennie brennie

    Single quotes.

    brennie brennie

    Single quotes.

    brennie brennie

    Our style dicatates a blank line between a statement and a block.

    brennie brennie

    Same here.

    brennie brennie

    And here.

    brennie brennie

    And here.

    brennie brennie

    Style guide dictates a blank line between the end of a block and a statement.

    brennie brennie

    Can we format this like: if server: svn_remote_url = svn_remote_url.replace('file://', 'svn+ssh://%s' % server)

    brennie brennie

    Blank line between these.

    brennie brennie

    Needs a docstring.

    brennie brennie

    Why not just build the list directly? Also, single quotes.

    brennie brennie

    This should use self.git.

    brennie brennie

    Can we avoid the backslash via notes = execute( fetch_cmd, ignore_errors=True, none_on_ignored_error=True) # ... if notes is None: # ...

    brennie brennie

    Blank line between these.

    brennie brennie

    Blank line between these.

    brennie brennie

    Blank line between these.

    brennie brennie

    Docstring should be of the format: """Single line summary. Multi-line description. """

    brennie brennie

    Why are we building this instead of passing tehse arguments directly?

    brennie brennie

    Single quotes.

    brennie brennie

    Blank line between these.

    brennie brennie

    Blank line between these.

    brennie brennie

    Blank line between these.

    brennie brennie

    Blank line between these.

    brennie brennie

    I'm sure we can avoid the backslash here. Are you trying to split on only newlines or all whitespace? If …

    brennie brennie

    Blank line between these.

    brennie brennie

    Blank line between these.

    brennie brennie

    Blank line between these.

    brennie brennie

    Blank line between these.

    brennie brennie

    Blank line between these.

    brennie brennie

    Blank line between these.

    brennie brennie

    Blank line between these.

    brennie brennie

    Blank line between these.

    brennie brennie

    Blank line between these.

    brennie brennie

    Needs a docstring.

    brennie brennie

    Blank line between these.

    brennie brennie

    Can you undo all the moves you did? These are unnecessary.

    brennie brennie

    Test methods should not have a leading underscore. Also, all these methods should be in snake_case. Finally, they all should …

    brennie brennie

    Col: 80 E501 line too long (87 > 79 characters)

    reviewbot reviewbot

    undefined name 'svn_url_key'

    reviewbot reviewbot

    Col: 80 E501 line too long (84 > 79 characters)

    reviewbot reviewbot

    Col: 80 E501 line too long (90 > 79 characters)

    reviewbot reviewbot

    Col: 80 E501 line too long (95 > 79 characters)

    reviewbot reviewbot

    Col: 80 E501 line too long (83 > 79 characters)

    reviewbot reviewbot

    Col: 80 E501 line too long (81 > 79 characters)

    reviewbot reviewbot

    Col: 30 E128 continuation line under-indented for visual indent

    reviewbot reviewbot

    Col: 38 E128 continuation line under-indented for visual indent

    reviewbot reviewbot
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          rbtools/clients/tests.py
          rbtools/api/request.py
          rbtools/clients/git.py
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          rbtools/clients/tests.py
          rbtools/api/request.py
          rbtools/clients/git.py
      
      
    2. rbtools/api/request.py (Diff revision 1)
       
       
      Show all issues
       redefinition of unused 'StringIO' from line 11
      
    3. rbtools/clients/git.py (Diff revision 1)
       
       
      Show all issues
      Col: 1
       E302 expected 2 blank lines, found 1
      
    4. rbtools/clients/git.py (Diff revision 1)
       
       
      Show all issues
       undefined name 'git_dir'
      
    5. rbtools/clients/git.py (Diff revision 1)
       
       
      Show all issues
      Col: 80
       E501 line too long (82 > 79 characters)
      
    6. rbtools/clients/git.py (Diff revision 1)
       
       
      Show all issues
      Col: 25
       E127 continuation line over-indented for visual indent
      
    7. rbtools/clients/git.py (Diff revision 1)
       
       
      Show all issues
      Col: 55
       E261 at least two spaces before inline comment
      
    8. rbtools/clients/git.py (Diff revision 1)
       
       
      Show all issues
      Col: 80
       E501 line too long (84 > 79 characters)
      
    9. rbtools/clients/git.py (Diff revision 1)
       
       
      Show all issues
      Col: 38
       E261 at least two spaces before inline comment
      
    10. rbtools/clients/git.py (Diff revision 1)
       
       
      Show all issues
      Col: 80
       E501 line too long (93 > 79 characters)
      
    11. rbtools/clients/git.py (Diff revision 1)
       
       
      Show all issues
      Col: 80
       E501 line too long (92 > 79 characters)
      
    12. rbtools/clients/git.py (Diff revision 1)
       
       
      Show all issues
      Col: 80
       E501 line too long (88 > 79 characters)
      
    13. rbtools/clients/git.py (Diff revision 1)
       
       
      Show all issues
      Col: 80
       E501 line too long (87 > 79 characters)
      
    14. rbtools/clients/git.py (Diff revision 1)
       
       
      Show all issues
      Col: 80
       E501 line too long (81 > 79 characters)
      
    15. rbtools/clients/git.py (Diff revision 1)
       
       
      Show all issues
      Col: 80
       E501 line too long (83 > 79 characters)
      
    16. rbtools/clients/git.py (Diff revision 1)
       
       
      Show all issues
      Col: 80
       E501 line too long (86 > 79 characters)
      
    17. rbtools/clients/git.py (Diff revision 1)
       
       
      Show all issues
      Col: 80
       E501 line too long (81 > 79 characters)
      
    18. rbtools/clients/git.py (Diff revision 1)
       
       
      Show all issues
      Col: 80
       E501 line too long (89 > 79 characters)
      
    19. rbtools/clients/git.py (Diff revision 1)
       
       
      Show all issues
      Col: 80
       E501 line too long (88 > 79 characters)
      
    20. rbtools/clients/git.py (Diff revision 1)
       
       
      Show all issues
      Col: 80
       E501 line too long (97 > 79 characters)
      
    21. rbtools/clients/git.py (Diff revision 1)
       
       
      Show all issues
      Col: 80
       E501 line too long (94 > 79 characters)
      
    22. 
        
    BR
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          rbtools/clients/tests.py
          rbtools/api/request.py
          rbtools/clients/git.py
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          rbtools/clients/tests.py
          rbtools/api/request.py
          rbtools/clients/git.py
      
      
    2. 
        
    brennie
    1. This revieiw is mostly just style nits.

    2. rbtools/api/request.py (Diff revision 2)
       
       
      Show all issues

      Can you undo the changes to this file?

    3. rbtools/clients/git.py (Diff revision 2)
       
       
      Show all issues

      Single quotes.

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

      Needs a docstring.

    5. rbtools/clients/git.py (Diff revision 2)
       
       
      Show all issues

      Needs a docstring.

    6. rbtools/clients/git.py (Diff revision 2)
       
       
      Show all issues

      single quotes

    7. rbtools/clients/git.py (Diff revision 2)
       
       
      Show all issues

      Needs a docstring.

    8. rbtools/clients/git.py (Diff revision 2)
       
       
       
      Show all issues

      Blank line between these.

    9. rbtools/clients/git.py (Diff revision 2)
       
       
       
      Show all issues

      Blank line between these.

    10. rbtools/clients/git.py (Diff revision 2)
       
       
       
      Show all issues

      Blank line before a block.

    11. rbtools/clients/git.py (Diff revision 2)
       
       
       
      Show all issues

      Single quotes.

    12. rbtools/clients/git.py (Diff revision 2)
       
       
      Show all issues

      Single quotes.

    13. rbtools/clients/git.py (Diff revision 2)
       
       
       
      Show all issues

      Blank line between these.

    14. rbtools/clients/git.py (Diff revision 2)
       
       
       
      Show all issues

      We are moving away from using die in all code. This should raise an exception.

      Also, single quotes.

    15. rbtools/clients/git.py (Diff revision 2)
       
       
      Show all issues

      Needs a docstring.

    16. rbtools/clients/git.py (Diff revision 2)
       
       
      Show all issues

      Needs a docstring.

    17. rbtools/clients/git.py (Diff revision 2)
       
       
      Show all issues

      Single quotes.

    18. rbtools/clients/git.py (Diff revision 2)
       
       
       
      Show all issues

      Blank line between these.

    19. rbtools/clients/git.py (Diff revision 2)
       
       
      Show all issues

      Single quotes.

    20. rbtools/clients/git.py (Diff revision 2)
       
       
       
      Show all issues

      Blank line between these.

    21. rbtools/clients/git.py (Diff revision 2)
       
       
       
      Show all issues

      Blank line between these.

    22. rbtools/clients/git.py (Diff revision 2)
       
       
       
      Show all issues

      Blank line between these.

    23. rbtools/clients/git.py (Diff revision 2)
       
       
       
      Show all issues

      Blank line between these. Single quotes.

    24. rbtools/clients/git.py (Diff revision 2)
       
       
      Show all issues

      Single quotes.

    25. rbtools/clients/git.py (Diff revision 2)
       
       
      Show all issues

      Single quotes.

    26. rbtools/clients/git.py (Diff revision 2)
       
       
       
      Show all issues

      Blank line between these.

    27. rbtools/clients/git.py (Diff revision 2)
       
       
      Show all issues

      Single quotes.

    28. rbtools/clients/git.py (Diff revision 2)
       
       
       
      Show all issues

      Instead of using memoize here, we can just cache this in a field on the class.

    29. rbtools/clients/git.py (Diff revision 2)
       
       
      Show all issues

      Needs a docstring.

    30. rbtools/clients/git.py (Diff revision 2)
       
       
       
       
      Show all issues

      Why not just

      config_svn_url = execute(['git', 'config', '--git', 'rbtools.subgit.svn_url'],
                               ignore_errors=True, none_on_ignored_error=True)
      
    31. rbtools/clients/git.py (Diff revision 2)
       
       
      Show all issues

      Single quotes.

    32. rbtools/clients/git.py (Diff revision 2)
       
       
      Show all issues

      Single quotes.

    33. rbtools/clients/git.py (Diff revision 2)
       
       
       
      Show all issues

      Our style dicatates a blank line between a statement and a block.

    34. rbtools/clients/git.py (Diff revision 2)
       
       
       
      Show all issues

      Same here.

    35. rbtools/clients/git.py (Diff revision 2)
       
       
       
      Show all issues

      And here.

    36. rbtools/clients/git.py (Diff revision 2)
       
       
       
      Show all issues

      And here.

    37. rbtools/clients/git.py (Diff revision 2)
       
       
       
      Show all issues

      Style guide dictates a blank line between the end of a block and a statement.

    38. rbtools/clients/git.py (Diff revision 2)
       
       
       
      Show all issues

      Can we format this like:

              if server:
                  svn_remote_url = svn_remote_url.replace('file://',
                                                          'svn+ssh://%s' % server)
      
    39. rbtools/clients/git.py (Diff revision 2)
       
       
       
      Show all issues

      Blank line between these.

    40. rbtools/clients/git.py (Diff revision 2)
       
       
      Show all issues

      Needs a docstring.

    41. rbtools/clients/git.py (Diff revision 2)
       
       
      Show all issues

      Why not just build the list directly? Also, single quotes.

    42. rbtools/clients/git.py (Diff revision 2)
       
       
      Show all issues

      This should use self.git.

    43. rbtools/clients/git.py (Diff revision 2)
       
       
       
       
       
      Show all issues

      Can we avoid the backslash via

              notes = execute(
                  fetch_cmd,
                  ignore_errors=True,
                  none_on_ignored_error=True)
      
              # ...
              if notes is None:
                  # ...
      
    44. rbtools/clients/git.py (Diff revision 2)
       
       
       
      Show all issues

      Blank line between these.

    45. rbtools/clients/git.py (Diff revision 2)
       
       
       
      Show all issues

      Blank line between these.

    46. rbtools/clients/git.py (Diff revision 2)
       
       
       
      Show all issues

      Blank line between these.

    47. rbtools/clients/git.py (Diff revision 2)
       
       
       
       
       
       
      Show all issues

      Docstring should be of the format:

      """Single line summary.
      
      Multi-line description.
      """
      
    48. rbtools/clients/git.py (Diff revision 2)
       
       
      Show all issues

      Why are we building this instead of passing tehse arguments directly?

      1. So as to avoid repeating them in the four subsequent calls to execute().

    49. rbtools/clients/git.py (Diff revision 2)
       
       
       
      Show all issues

      Single quotes.

    50. rbtools/clients/git.py (Diff revision 2)
       
       
       
      Show all issues

      Blank line between these.

    51. rbtools/clients/git.py (Diff revision 2)
       
       
       
      Show all issues

      Blank line between these.

    52. rbtools/clients/git.py (Diff revision 2)
       
       
       
      Show all issues

      Blank line between these.

    53. rbtools/clients/git.py (Diff revision 2)
       
       
       
      Show all issues

      Blank line between these.

    54. rbtools/clients/git.py (Diff revision 2)
       
       
       
      Show all issues

      I'm sure we can avoid the backslash here.

      Are you trying to split on only newlines or all whitespace? If the former, you can pass split_newline=True to execute.

    55. rbtools/clients/git.py (Diff revision 2)
       
       
       
      Show all issues

      Blank line between these.

    56. rbtools/clients/git.py (Diff revision 2)
       
       
       
      Show all issues

      Blank line between these.

    57. rbtools/clients/git.py (Diff revision 2)
       
       
       
      Show all issues

      Blank line between these.

    58. rbtools/clients/git.py (Diff revision 2)
       
       
       
      Show all issues

      Blank line between these.

    59. rbtools/clients/git.py (Diff revision 2)
       
       
       
      Show all issues

      Blank line between these.

    60. rbtools/clients/git.py (Diff revision 2)
       
       
       
      Show all issues

      Blank line between these.

    61. rbtools/clients/git.py (Diff revision 2)
       
       
       
      Show all issues

      Blank line between these.

    62. rbtools/clients/git.py (Diff revision 2)
       
       
       
      Show all issues

      Blank line between these.

    63. rbtools/clients/git.py (Diff revision 2)
       
       
       
      Show all issues

      Blank line between these.

    64. rbtools/clients/git.py (Diff revision 2)
       
       
      Show all issues

      Needs a docstring.

    65. rbtools/clients/git.py (Diff revision 2)
       
       
       
      Show all issues

      Blank line between these.

    66. rbtools/clients/tests.py (Diff revision 2)
       
       
      Show all issues

      Test methods should not have a leading underscore.

      Also, all these methods should be in snake_case.

      Finally, they all should have a doscring. If the docstring wont fit in a single line, you can wrap it as

      """Foo ...
      Bar ...
      """
      

      This is a special exception for test docstrings.

      1. These are not actually test methods, but methods called by actual test methods in classes that mix in SvnWrapperMixin. This would be clearer if they had docstrings. :-)

    67. 
        
    brennie
    1. 
        
    2. rbtools/clients/tests.py (Diff revision 2)
       
       
      Show all issues

      Can you undo all the moves you did? These are unnecessary.

      1. Confused by this comment. These are all part of pulling out a mixin that can be used for git-svn and hg-svn tests (as I didn't see any git-svn tests when I started working on this).

    3. 
        
    BR
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          rbtools/clients/tests.py
          rbtools/clients/errors.py
          rbtools/clients/git.py
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          rbtools/clients/tests.py
          rbtools/clients/errors.py
          rbtools/clients/git.py
      
      
    2. rbtools/clients/git.py (Diff revision 3)
       
       
      Show all issues
      Col: 80
       E501 line too long (87 > 79 characters)
      
    3. rbtools/clients/git.py (Diff revision 3)
       
       
      Show all issues
       undefined name 'svn_url_key'
      
    4. rbtools/clients/git.py (Diff revision 3)
       
       
      Show all issues
      Col: 80
       E501 line too long (84 > 79 characters)
      
    5. rbtools/clients/git.py (Diff revision 3)
       
       
      Show all issues
      Col: 80
       E501 line too long (90 > 79 characters)
      
    6. rbtools/clients/git.py (Diff revision 3)
       
       
      Show all issues
      Col: 80
       E501 line too long (95 > 79 characters)
      
    7. rbtools/clients/git.py (Diff revision 3)
       
       
      Show all issues
      Col: 80
       E501 line too long (83 > 79 characters)
      
    8. rbtools/clients/git.py (Diff revision 3)
       
       
      Show all issues
      Col: 80
       E501 line too long (81 > 79 characters)
      
    9. rbtools/clients/tests.py (Diff revision 3)
       
       
      Show all issues
      Col: 30
       E128 continuation line under-indented for visual indent
      
    10. rbtools/clients/tests.py (Diff revision 3)
       
       
      Show all issues
      Col: 38
       E128 continuation line under-indented for visual indent
      
    11. 
        
    david
    Review request changed
    Status:
    Discarded