[WIP] SubGit support

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

brettdh
RBTools
master
rbtools

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[HTML_REMOVED] 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)
     
     
     redefinition of unused 'StringIO' from line 11
    
  3. rbtools/clients/git.py (Diff revision 1)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  4. rbtools/clients/git.py (Diff revision 1)
     
     
     undefined name 'git_dir'
    
  5. rbtools/clients/git.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (82 > 79 characters)
    
  6. rbtools/clients/git.py (Diff revision 1)
     
     
    Col: 25
     E127 continuation line over-indented for visual indent
    
  7. rbtools/clients/git.py (Diff revision 1)
     
     
    Col: 55
     E261 at least two spaces before inline comment
    
  8. rbtools/clients/git.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (84 > 79 characters)
    
  9. rbtools/clients/git.py (Diff revision 1)
     
     
    Col: 38
     E261 at least two spaces before inline comment
    
  10. rbtools/clients/git.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (93 > 79 characters)
    
  11. rbtools/clients/git.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (92 > 79 characters)
    
  12. rbtools/clients/git.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (88 > 79 characters)
    
  13. rbtools/clients/git.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (87 > 79 characters)
    
  14. rbtools/clients/git.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  15. rbtools/clients/git.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  16. rbtools/clients/git.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (86 > 79 characters)
    
  17. rbtools/clients/git.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  18. rbtools/clients/git.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (89 > 79 characters)
    
  19. rbtools/clients/git.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (88 > 79 characters)
    
  20. rbtools/clients/git.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (97 > 79 characters)
    
  21. rbtools/clients/git.py (Diff revision 1)
     
     
    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)
     
     

    Can you undo the changes to this file?

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

    Single quotes.

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

    Needs a docstring.

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

    Needs a docstring.

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

    single quotes

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

    Needs a docstring.

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

    Blank line between these.

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

    Blank line between these.

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

    Blank line before a block.

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

    Single quotes.

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

    Single quotes.

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

    Blank line between these.

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

    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)
     
     

    Needs a docstring.

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

    Needs a docstring.

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

    Single quotes.

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

    Blank line between these.

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

    Single quotes.

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

    Blank line between these.

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

    Blank line between these.

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

    Blank line between these.

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

    Blank line between these. Single quotes.

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

    Single quotes.

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

    Single quotes.

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

    Blank line between these.

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

    Single quotes.

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

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

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

    Needs a docstring.

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

    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)
     
     

    Single quotes.

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

    Single quotes.

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

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

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

    Same here.

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

    And here.

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

    And here.

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

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

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

    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)
     
     
     

    Blank line between these.

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

    Needs a docstring.

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

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

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

    This should use self.git.

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

    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)
     
     
     

    Blank line between these.

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

    Blank line between these.

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

    Blank line between these.

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

    Docstring should be of the format:

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

    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)
     
     
     

    Single quotes.

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

    Blank line between these.

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

    Blank line between these.

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

    Blank line between these.

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

    Blank line between these.

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

    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)
     
     
     

    Blank line between these.

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

    Blank line between these.

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

    Blank line between these.

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

    Blank line between these.

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

    Blank line between these.

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

    Blank line between these.

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

    Blank line between these.

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

    Blank line between these.

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

    Blank line between these.

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

    Needs a docstring.

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

    Blank line between these.

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

    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)
     
     

    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)
     
     
    Col: 80
     E501 line too long (87 > 79 characters)
    
  3. rbtools/clients/git.py (Diff revision 3)
     
     
     undefined name 'svn_url_key'
    
  4. rbtools/clients/git.py (Diff revision 3)
     
     
    Col: 80
     E501 line too long (84 > 79 characters)
    
  5. rbtools/clients/git.py (Diff revision 3)
     
     
    Col: 80
     E501 line too long (90 > 79 characters)
    
  6. rbtools/clients/git.py (Diff revision 3)
     
     
    Col: 80
     E501 line too long (95 > 79 characters)
    
  7. rbtools/clients/git.py (Diff revision 3)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  8. rbtools/clients/git.py (Diff revision 3)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  9. rbtools/clients/tests.py (Diff revision 3)
     
     
    Col: 30
     E128 continuation line under-indented for visual indent
    
  10. rbtools/clients/tests.py (Diff revision 3)
     
     
    Col: 38
     E128 continuation line under-indented for visual indent
    
  11. 
      
david
Review request changed

Status: Discarded

Loading...