[WIP] SubGit support
Review Request #7078 — Created March 18, 2015 and discarded
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 translation2) 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 | |
Col: 1 E302 expected 2 blank lines, found 1 |
reviewbot | |
undefined name 'git_dir' |
reviewbot | |
Col: 80 E501 line too long (82 > 79 characters) |
reviewbot | |
Col: 25 E127 continuation line over-indented for visual indent |
reviewbot | |
Col: 55 E261 at least two spaces before inline comment |
reviewbot | |
Col: 80 E501 line too long (84 > 79 characters) |
reviewbot | |
Col: 38 E261 at least two spaces before inline comment |
reviewbot | |
Col: 80 E501 line too long (93 > 79 characters) |
reviewbot | |
Col: 80 E501 line too long (92 > 79 characters) |
reviewbot | |
Col: 80 E501 line too long (88 > 79 characters) |
reviewbot | |
Col: 80 E501 line too long (87 > 79 characters) |
reviewbot | |
Col: 80 E501 line too long (81 > 79 characters) |
reviewbot | |
Col: 80 E501 line too long (83 > 79 characters) |
reviewbot | |
Col: 80 E501 line too long (86 > 79 characters) |
reviewbot | |
Col: 80 E501 line too long (81 > 79 characters) |
reviewbot | |
Col: 80 E501 line too long (89 > 79 characters) |
reviewbot | |
Col: 80 E501 line too long (88 > 79 characters) |
reviewbot | |
Col: 80 E501 line too long (97 > 79 characters) |
reviewbot | |
Col: 80 E501 line too long (94 > 79 characters) |
reviewbot | |
Can you undo the changes to this file? |
brennie | |
Single quotes. |
brennie | |
Needs a docstring. |
brennie | |
Needs a docstring. |
brennie | |
single quotes |
brennie | |
Needs a docstring. |
brennie | |
Blank line between these. |
brennie | |
Blank line between these. |
brennie | |
Blank line before a block. |
brennie | |
Single quotes. |
brennie | |
Single quotes. |
brennie | |
Blank line between these. |
brennie | |
We are moving away from using die in all code. This should raise an exception. Also, single quotes. |
brennie | |
Needs a docstring. |
brennie | |
Needs a docstring. |
brennie | |
Single quotes. |
brennie | |
Blank line between these. |
brennie | |
Single quotes. |
brennie | |
Blank line between these. |
brennie | |
Blank line between these. |
brennie | |
Blank line between these. |
brennie | |
Blank line between these. Single quotes. |
brennie | |
Single quotes. |
brennie | |
Single quotes. |
brennie | |
Blank line between these. |
brennie | |
Single quotes. |
brennie | |
Instead of using memoize here, we can just cache this in a field on the class. |
brennie | |
Needs a docstring. |
brennie | |
Why not just config_svn_url = execute(['git', 'config', '--git', 'rbtools.subgit.svn_url'], ignore_errors=True, none_on_ignored_error=True) |
brennie | |
Single quotes. |
brennie | |
Single quotes. |
brennie | |
Our style dicatates a blank line between a statement and a block. |
brennie | |
Same here. |
brennie | |
And here. |
brennie | |
And here. |
brennie | |
Style guide dictates a blank line between the end of a block and a statement. |
brennie | |
Can we format this like: if server: svn_remote_url = svn_remote_url.replace('file://', 'svn+ssh://%s' % server) |
brennie | |
Blank line between these. |
brennie | |
Needs a docstring. |
brennie | |
Why not just build the list directly? Also, single quotes. |
brennie | |
This should use self.git. |
brennie | |
Can we avoid the backslash via notes = execute( fetch_cmd, ignore_errors=True, none_on_ignored_error=True) # ... if notes is None: # ... |
brennie | |
Blank line between these. |
brennie | |
Blank line between these. |
brennie | |
Blank line between these. |
brennie | |
Docstring should be of the format: """Single line summary. Multi-line description. """ |
brennie | |
Why are we building this instead of passing tehse arguments directly? |
brennie | |
Single quotes. |
brennie | |
Blank line between these. |
brennie | |
Blank line between these. |
brennie | |
Blank line between these. |
brennie | |
Blank line between these. |
brennie | |
I'm sure we can avoid the backslash here. Are you trying to split on only newlines or all whitespace? If … |
brennie | |
Blank line between these. |
brennie | |
Blank line between these. |
brennie | |
Blank line between these. |
brennie | |
Blank line between these. |
brennie | |
Blank line between these. |
brennie | |
Blank line between these. |
brennie | |
Blank line between these. |
brennie | |
Blank line between these. |
brennie | |
Blank line between these. |
brennie | |
Needs a docstring. |
brennie | |
Blank line between these. |
brennie | |
Can you undo all the moves you did? These are unnecessary. |
brennie | |
Test methods should not have a leading underscore. Also, all these methods should be in snake_case. Finally, they all should … |
brennie | |
Col: 80 E501 line too long (87 > 79 characters) |
reviewbot | |
undefined name 'svn_url_key' |
reviewbot | |
Col: 80 E501 line too long (84 > 79 characters) |
reviewbot | |
Col: 80 E501 line too long (90 > 79 characters) |
reviewbot | |
Col: 80 E501 line too long (95 > 79 characters) |
reviewbot | |
Col: 80 E501 line too long (83 > 79 characters) |
reviewbot | |
Col: 80 E501 line too long (81 > 79 characters) |
reviewbot | |
Col: 30 E128 continuation line under-indented for visual indent |
reviewbot | |
Col: 38 E128 continuation line under-indented for visual indent |
reviewbot |
- Change Summary:
-
PEP8/pyflakes fixes, plus some added none_on_ignored_error kwargs.
- Commit:
-
85a599f5b73d702c4af2b3789266645c2247af5803f5af4b18ad27eafb75eb82f1044ef65c164605
-
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.
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
Why not just
config_svn_url = execute(['git', 'config', '--git', 'rbtools.subgit.svn_url'], ignore_errors=True, none_on_ignored_error=True)
-
-
-
-
-
-
-
-
Can we format this like:
if server: svn_remote_url = svn_remote_url.replace('file://', 'svn+ssh://%s' % server)
-
-
-
-
-
Can we avoid the backslash via
notes = execute( fetch_cmd, ignore_errors=True, none_on_ignored_error=True) # ... if notes is None: # ...
-
-
-
-
-
-
-
-
-
-
-
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
toexecute
. -
-
-
-
-
-
-
-
-
-
-
-
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.
- 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:
-
03f5af4b18ad27eafb75eb82f1044ef65c164605de5151620b08038f939770174a4a7be8830a2f33