• 
      

    [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

    reviewbotreviewbot

    Col: 1 E302 expected 2 blank lines, found 1

    reviewbotreviewbot

    undefined name 'git_dir'

    reviewbotreviewbot

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

    reviewbotreviewbot

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

    reviewbotreviewbot

    Col: 55 E261 at least two spaces before inline comment

    reviewbotreviewbot

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

    reviewbotreviewbot

    Col: 38 E261 at least two spaces before inline comment

    reviewbotreviewbot

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

    reviewbotreviewbot

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

    reviewbotreviewbot

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

    reviewbotreviewbot

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

    reviewbotreviewbot

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

    reviewbotreviewbot

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

    reviewbotreviewbot

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

    reviewbotreviewbot

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

    reviewbotreviewbot

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

    reviewbotreviewbot

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

    reviewbotreviewbot

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

    reviewbotreviewbot

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

    reviewbotreviewbot

    Can you undo the changes to this file?

    brenniebrennie

    Single quotes.

    brenniebrennie

    Needs a docstring.

    brenniebrennie

    Needs a docstring.

    brenniebrennie

    single quotes

    brenniebrennie

    Needs a docstring.

    brenniebrennie

    Blank line between these.

    brenniebrennie

    Blank line between these.

    brenniebrennie

    Blank line before a block.

    brenniebrennie

    Single quotes.

    brenniebrennie

    Single quotes.

    brenniebrennie

    Blank line between these.

    brenniebrennie

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

    brenniebrennie

    Needs a docstring.

    brenniebrennie

    Needs a docstring.

    brenniebrennie

    Single quotes.

    brenniebrennie

    Blank line between these.

    brenniebrennie

    Single quotes.

    brenniebrennie

    Blank line between these.

    brenniebrennie

    Blank line between these.

    brenniebrennie

    Blank line between these.

    brenniebrennie

    Blank line between these. Single quotes.

    brenniebrennie

    Single quotes.

    brenniebrennie

    Single quotes.

    brenniebrennie

    Blank line between these.

    brenniebrennie

    Single quotes.

    brenniebrennie

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

    brenniebrennie

    Needs a docstring.

    brenniebrennie

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

    brenniebrennie

    Single quotes.

    brenniebrennie

    Single quotes.

    brenniebrennie

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

    brenniebrennie

    Same here.

    brenniebrennie

    And here.

    brenniebrennie

    And here.

    brenniebrennie

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

    brenniebrennie

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

    brenniebrennie

    Blank line between these.

    brenniebrennie

    Needs a docstring.

    brenniebrennie

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

    brenniebrennie

    This should use self.git.

    brenniebrennie

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

    brenniebrennie

    Blank line between these.

    brenniebrennie

    Blank line between these.

    brenniebrennie

    Blank line between these.

    brenniebrennie

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

    brenniebrennie

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

    brenniebrennie

    Single quotes.

    brenniebrennie

    Blank line between these.

    brenniebrennie

    Blank line between these.

    brenniebrennie

    Blank line between these.

    brenniebrennie

    Blank line between these.

    brenniebrennie

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

    brenniebrennie

    Blank line between these.

    brenniebrennie

    Blank line between these.

    brenniebrennie

    Blank line between these.

    brenniebrennie

    Blank line between these.

    brenniebrennie

    Blank line between these.

    brenniebrennie

    Blank line between these.

    brenniebrennie

    Blank line between these.

    brenniebrennie

    Blank line between these.

    brenniebrennie

    Blank line between these.

    brenniebrennie

    Needs a docstring.

    brenniebrennie

    Blank line between these.

    brenniebrennie

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

    brenniebrennie

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

    brenniebrennie

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

    reviewbotreviewbot

    undefined name 'svn_url_key'

    reviewbotreviewbot

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

    reviewbotreviewbot

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

    reviewbotreviewbot

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

    reviewbotreviewbot

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

    reviewbotreviewbot

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

    reviewbotreviewbot

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

    reviewbotreviewbot

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

    reviewbotreviewbot
    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