[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