Summary

[WIP] SubGit support

Review Request #7078 — Created March 18, 2015 and discarded Jan. 29, 2019, 1:40 p.m.

Information

Owner

brettdh

Repository

RBTools

Branch

master

Bugs

Depends On

Reviewers

Groups

rbtools

People

Description

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.

Testing Done

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).

Issues

Description	From	Last Updated
redefinition of unused 'StringIO' from line 11	reviewbot	May 28, 2015, 4:59 a.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	May 28, 2015, 5:02 a.m.
undefined name 'git_dir'	reviewbot	May 28, 2015, 5:24 a.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	May 28, 2015, 5:24 a.m.
Col: 25 E127 continuation line over-indented for visual indent	reviewbot	May 28, 2015, 5:24 a.m.
Col: 55 E261 at least two spaces before inline comment	reviewbot	May 28, 2015, 5:24 a.m.
Col: 80 E501 line too long (84 > 79 characters)	reviewbot	May 28, 2015, 5:24 a.m.
Col: 38 E261 at least two spaces before inline comment	reviewbot	May 28, 2015, 5:24 a.m.
Col: 80 E501 line too long (93 > 79 characters)	reviewbot	May 28, 2015, 5:24 a.m.
Col: 80 E501 line too long (92 > 79 characters)	reviewbot	May 28, 2015, 5:24 a.m.
Col: 80 E501 line too long (88 > 79 characters)	reviewbot	May 28, 2015, 5:24 a.m.
Col: 80 E501 line too long (87 > 79 characters)	reviewbot	May 28, 2015, 5:24 a.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	May 28, 2015, 5:24 a.m.
Col: 80 E501 line too long (83 > 79 characters)	reviewbot	May 28, 2015, 5:24 a.m.
Col: 80 E501 line too long (86 > 79 characters)	reviewbot	May 28, 2015, 5:24 a.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	May 28, 2015, 5:24 a.m.
Col: 80 E501 line too long (89 > 79 characters)	reviewbot	May 28, 2015, 5:24 a.m.
Col: 80 E501 line too long (88 > 79 characters)	reviewbot	May 28, 2015, 5:24 a.m.
Col: 80 E501 line too long (97 > 79 characters)	reviewbot	May 28, 2015, 5:24 a.m.
Col: 80 E501 line too long (94 > 79 characters)	reviewbot	May 28, 2015, 5:24 a.m.
Can you undo the changes to this file?	brennie	June 2, 2015, 12:29 p.m.
Single quotes.	brennie	June 2, 2015, 12:30 p.m.
Needs a docstring.	brennie	June 2, 2015, 1:26 p.m.
Needs a docstring.	brennie	June 2, 2015, 1:23 p.m.
single quotes	brennie	June 2, 2015, 12:31 p.m.
Needs a docstring.	brennie	June 2, 2015, 1:26 p.m.
Blank line between these.	brennie	June 2, 2015, 12:49 p.m.
Blank line between these.	brennie	June 2, 2015, 12:49 p.m.
Blank line before a block.	brennie	June 2, 2015, 12:49 p.m.
Single quotes.	brennie	June 2, 2015, 12:32 p.m.
Single quotes.	brennie	June 2, 2015, 12:32 p.m.
Blank line between these.	brennie	June 2, 2015, 1:07 p.m.
We are moving away from using die in all code. This should raise an exception. Also, single quotes.	brennie	June 2, 2015, 12:59 p.m.
Needs a docstring.	brennie	June 2, 2015, 1:33 p.m.
Needs a docstring.	brennie	June 2, 2015, 1:33 p.m.
Single quotes.	brennie	June 2, 2015, 12:35 p.m.
Blank line between these.	brennie	June 2, 2015, 1 p.m.
Single quotes.	brennie	June 2, 2015, 12:35 p.m.
Blank line between these.	brennie	June 2, 2015, 1 p.m.
Blank line between these.	brennie	June 2, 2015, 1 p.m.
Blank line between these.	brennie	June 2, 2015, 1 p.m.
Blank line between these. Single quotes.	brennie	June 2, 2015, 12:37 p.m.
Single quotes.	brennie	June 2, 2015, 12:37 p.m.
Single quotes.	brennie	June 2, 2015, 12:37 p.m.
Blank line between these.	brennie	June 2, 2015, 12:37 p.m.
Single quotes.	brennie	June 2, 2015, 12:37 p.m.
Instead of using memoize here, we can just cache this in a field on the class.	brennie	June 2, 2015, 1:43 p.m.
Needs a docstring.	brennie	June 2, 2015, 1:33 p.m.
Why not just config_svn_url = execute(['git', 'config', '--git', 'rbtools.subgit.svn_url'], ignore_errors=True, none_on_ignored_error=True)	brennie	June 2, 2015, 12:40 p.m.
Single quotes.	brennie	June 2, 2015, 12:41 p.m.
Single quotes.	brennie	June 2, 2015, 12:41 p.m.
Our style dicatates a blank line between a statement and a block.	brennie	June 2, 2015, 12:41 p.m.
Same here.	brennie	June 2, 2015, 12:42 p.m.
And here.	brennie	June 2, 2015, 12:42 p.m.
And here.	brennie	June 2, 2015, 12:42 p.m.
Style guide dictates a blank line between the end of a block and a statement.	brennie	June 2, 2015, 12:42 p.m.
Can we format this like: if server: svn_remote_url = svn_remote_url.replace('file://', 'svn+ssh://%s' % server)	brennie	June 2, 2015, 1:01 p.m.
Blank line between these.	brennie	June 2, 2015, 1:01 p.m.
Needs a docstring.	brennie	June 2, 2015, 1:36 p.m.
Why not just build the list directly? Also, single quotes.	brennie	June 2, 2015, 12:43 p.m.
This should use self.git.	brennie	June 2, 2015, 12:44 p.m.
Can we avoid the backslash via notes = execute( fetch_cmd, ignore_errors=True, none_on_ignored_error=True) # ... if notes is None: # ...	brennie	June 2, 2015, 12:45 p.m.
Blank line between these.	brennie	June 2, 2015, 12:46 p.m.
Blank line between these.	brennie	June 2, 2015, 12:47 p.m.
Blank line between these.	brennie	June 2, 2015, 12:47 p.m.
Docstring should be of the format: """Single line summary. Multi-line description. """	brennie	June 2, 2015, 1:36 p.m.
Why are we building this instead of passing tehse arguments directly?	brennie	June 2, 2015, 1:43 p.m.
Single quotes.	brennie	June 2, 2015, 12:48 p.m.
Blank line between these.	brennie	June 2, 2015, 1:02 p.m.
Blank line between these.	brennie	June 2, 2015, 1:02 p.m.
Blank line between these.	brennie	June 2, 2015, 1:02 p.m.
Blank line between these.	brennie	June 2, 2015, 1:03 p.m.
I'm sure we can avoid the backslash here. Are you trying to split on only newlines or all whitespace? If …	brennie	June 2, 2015, 1:04 p.m.
Blank line between these.	brennie	June 2, 2015, 1:04 p.m.
Blank line between these.	brennie	June 2, 2015, 1:04 p.m.
Blank line between these.	brennie	June 2, 2015, 1:05 p.m.
Blank line between these.	brennie	June 2, 2015, 1:05 p.m.
Blank line between these.	brennie	June 2, 2015, 1:05 p.m.
Blank line between these.	brennie	June 2, 2015, 1:05 p.m.
Blank line between these.	brennie	June 2, 2015, 1:05 p.m.
Blank line between these.	brennie	June 2, 2015, 1:05 p.m.
Blank line between these.	brennie	June 2, 2015, 1:05 p.m.
Needs a docstring.	brennie	June 2, 2015, 1:39 p.m.
Blank line between these.	brennie	June 2, 2015, 1:06 p.m.
Can you undo all the moves you did? These are unnecessary.	brennie	June 2, 2015, 12:22 p.m.
Test methods should not have a leading underscore. Also, all these methods should be in snake_case. Finally, they all should …	brennie	June 2, 2015, 1:55 p.m.
Col: 80 E501 line too long (87 > 79 characters)	reviewbot	June 2, 2015, 1:59 p.m.
undefined name 'svn_url_key'	reviewbot	June 2, 2015, 1:59 p.m.
Col: 80 E501 line too long (84 > 79 characters)	reviewbot	June 2, 2015, 1:59 p.m.
Col: 80 E501 line too long (90 > 79 characters)	reviewbot	June 2, 2015, 1:59 p.m.
Col: 80 E501 line too long (95 > 79 characters)	reviewbot	June 2, 2015, 1:59 p.m.
Col: 80 E501 line too long (83 > 79 characters)	reviewbot	June 2, 2015, 1:59 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	June 2, 2015, 1:59 p.m.
Col: 30 E128 continuation line under-indented for visual indent	reviewbot	June 2, 2015, 1:59 p.m.
Col: 38 E128 continuation line under-indented for visual indent	reviewbot	June 2, 2015, 1:59 p.m.

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

rbtools/api/request.py (Diff revision 1)

Show all issues

 redefinition of unused 'StringIO' from line 11

rbtools/clients/git.py (Diff revision 1)

Show all issues

Col: 1
 E302 expected 2 blank lines, found 1

rbtools/clients/git.py (Diff revision 1)
Show all issues
```
 undefined name 'git_dir'
```

rbtools/clients/git.py (Diff revision 1)

Show all issues

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

rbtools/clients/git.py (Diff revision 1)

Show all issues

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

rbtools/clients/git.py (Diff revision 1)

Show all issues

Col: 55
 E261 at least two spaces before inline comment

rbtools/clients/git.py (Diff revision 1)

Show all issues

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

rbtools/clients/git.py (Diff revision 1)

Show all issues

Col: 38
 E261 at least two spaces before inline comment

rbtools/clients/git.py (Diff revision 1)

Show all issues

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

rbtools/clients/git.py (Diff revision 1)

Show all issues

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

rbtools/clients/git.py (Diff revision 1)

Show all issues

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

rbtools/clients/git.py (Diff revision 1)

Show all issues

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

rbtools/clients/git.py (Diff revision 1)

Show all issues

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

rbtools/clients/git.py (Diff revision 1)

Show all issues

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

rbtools/clients/git.py (Diff revision 1)

Show all issues

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

rbtools/clients/git.py (Diff revision 1)

Show all issues

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

rbtools/clients/git.py (Diff revision 1)

Show all issues

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

rbtools/clients/git.py (Diff revision 1)

Show all issues

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

rbtools/clients/git.py (Diff revision 1)

Show all issues

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

rbtools/clients/git.py (Diff revision 1)

Show all issues

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

Change Summary:

PEP8/pyflakes fixes, plus some added none_on_ignored_error kwargs.

Commit:

-	85a599f5b73d702c4af2b3789266645c2247af58
+	03f5af4b18ad27eafb75eb82f1044ef65c164605

Diff:

Revision 2 (+486 -179)

Show changes

	rbtools/api/request.py
	rbtools/clients/git.py
	rbtools/clients/tests.py

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

```
This revieiw is mostly just style nits.
```
rbtools/api/request.py (Diff revision 2)
Show all issues
```
Can you undo the changes to this file?
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Single quotes.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Needs a docstring.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Needs a docstring.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
single quotes
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Needs a docstring.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Blank line between these.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Blank line between these.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Blank line before a block.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Single quotes.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Single quotes.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Blank line between these.
```

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.

rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Needs a docstring.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Needs a docstring.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Single quotes.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Blank line between these.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Single quotes.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Blank line between these.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Blank line between these.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Blank line between these.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Blank line between these. Single quotes.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Single quotes.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Single quotes.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Blank line between these.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Single quotes.
```

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.

rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Needs a docstring.
```

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)

rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Single quotes.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Single quotes.
```

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

Show all issues

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

rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Same here.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
And here.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
And here.
```

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.

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)

rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Blank line between these.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Needs a docstring.
```

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

Show all issues

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

rbtools/clients/git.py (Diff revision 2)
Show all issues
```
This should use self.git.
```

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:
            # ...

rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Blank line between these.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Blank line between these.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Blank line between these.
```

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

Show all issues

Docstring should be of the format:

"""Single line summary.

Multi-line description.
"""

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

Show all issues

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

brettdh June 2, 2015, 12:25 p.m.

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

rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Single quotes.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Blank line between these.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Blank line between these.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Blank line between these.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Blank line between these.
```

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.

rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Blank line between these.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Blank line between these.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Blank line between these.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Blank line between these.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Blank line between these.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Blank line between these.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Blank line between these.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Blank line between these.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Blank line between these.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Needs a docstring.
```
rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Blank line between these.
```

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.

brettdh June 2, 2015, 12:25 p.m.

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. :-)

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

Show all issues

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

brettdh June 2, 2015, 12:17 p.m.

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).

Change Summary:

Lots of style fixups. Hits all review comments, I think, except the two I dropped (see my comments there).

The style feedback is great (and hopefully we are converging), but I would also love to get feedback on the design and testing changes.

Commit:

-	03f5af4b18ad27eafb75eb82f1044ef65c164605
+	de5151620b08038f939770174a4a7be8830a2f33

Diff:

Revision 3 (+585 -177)

Show changes

	rbtools/clients/errors.py
	rbtools/clients/git.py
	rbtools/clients/tests.py

Fix it!

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

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

Show all issues

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

rbtools/clients/git.py (Diff revision 3)
Show all issues
```
 undefined name 'svn_url_key'
```

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

Show all issues

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

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

Show all issues

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

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

Show all issues

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

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

Show all issues

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

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

Show all issues

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

rbtools/clients/tests.py (Diff revision 3)

Show all issues

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

rbtools/clients/tests.py (Diff revision 3)

Show all issues

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

Review Board 6.0.3 alpha 0 (dev)

[WIP] SubGit support

Information

Reviewers

Change Summary:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Status: Discarded