git rv sync --continue does not set the last synced hash value in the check_continue method. While we're at it, non-existent globals FINISHED and EXPORT are reference which should instead refer to the class "constants" self.FINISHED and self.EXPORT.

The seeds of a fix:

diff --git a/sync.py b/sync.py
index 819db1f..09ee048 100644
--- a/sync.py
+++ b/sync.py
@@ -139,9 +139,13 @@ class SyncAction(object):
             commits = utils.get_commits(self.__last_commit, 'HEAD')
             if len(commits) == 0:
                 print 'Please make a commit after resolving the merge conflict.'
-                self.state = FINISHED
+                self.state = self.FINISHED
             elif len(commits) == 1:
-                self.state = EXPORT
+                remote = self.__rietveld_info.remote_info.remote
+                remote_branch = self.__rietveld_info.remote_info.branch
+                remote_branch_ref = '%s/%s' % (remote, remote_branch)
+                self.__last_synced = utils.get_head_commit(remote_branch_ref)
+                self.state = self.EXPORT
             else:
                 template_args = {'commit': commits[-1]}
                 print TOO_MANY_COMMITS_AFTER_CONTINUE % template_args

/cc @tmatsuo

From the discussion in the initial review:

It was not obvious without reading from_branch implementation that it was
a get_or_create pattern.

Maybe there is a way to make it more explicit.
s/from_branch/get_from_branch/?

When implementing it, I previously was using a get_or_create pattern and there were places where None as the return in the case it didn't exist was helpful and others
where having the new instance was helpful.

I'm still torn on which I prefer or which makes sense given the naming.

/cc @proppy @danholevoet @fredsa

Essentially, args.server is returned from optparse. When it is not the default, it is a singleton list with the server passed as a flag, else it is simply a list.

For example, this current failure is caused by RietveldInfo.server assuming it will always receive a string (which it should):

$ git rv export -s review-site.appspot.com -r [email protected]
Traceback (most recent call last):
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/runpy.py", line 162, in _run_module_as_main
    "__main__", fname, loader, pkg_name)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/runpy.py", line 72, in _run_code
    exec code in run_globals
  File "__main__.py", line 28, in <module>
  File "__main__.py", line 23, in main
  File "export.py", line 159, in callback
  File "export.py", line 66, in __init__
  File "export.py", line 88, in __update_rietveld_info_from_args
  File "utils.py", line 699, in setter
  File "utils.py", line 618, in _string_type_cast
utils.GitRvException: Property must be string. Received ['review-site.appspot.com'].

The following change in ExportAction.__update_rietveld_info_from_args should be sufficient:

class ExportAction(object):
         Args:
             args: An argparse.Namespace object to extract parameters from.
         """
-        self.__rietveld_info.server = args.server
+        server = args.server
+        if isinstance(server, list):
+            assert len(server) == 1
+            server = server[0]
+        self.__rietveld_info.server = server

The state machine approach is currently too verbose and typical YAGNI and the advance() method on each action sometimes makes it unclear which states correspond to which methods.

A better approach needs to be figured out. This thread will be used to try to help resolve that and lead to a sweeping change (or a bunch of individual changes for each git-rv action).

One idea I had was to keep the state machines around, but to make them actual state machines by:

1. Using the method names as the states (modulo upper/lower case)
2. Having a single state machine/action base class which handles advancing
3. Instead of saving the git-rv metadata as a top level object (retrieved by
   git rv getinfo) use a protected key, e.g. final, and instead save the info as
   {'safe': previous_idea_of_rietveld_info}.
4. Use a combination of the command and state names as keys and save the results
   temporarily in the Rietveld info under a key other than final.
5. Add a --recover flag to all commands that allows the state machine to pick up at
   the last saved point. This would require thinking about when keys should be expunged
   from the Rietveld info.
6. Potentially disallow calling commands/actions without --recover if those actions are in a dirty state.
   Or at least do this by default and ask for --force to override. This could easily be done by shared code.
7. In our shared action parent class, create all the logic needed to make --recover
   and temporary saved states work, as well as a final clean_up method which will
   apply all the changes to the final key. Another valuable/necessary thing may be
   a way to remove the previous temporary result when saving the next/subsequent.

/cc @proppy @danholevoet @fredsa

Add an issue flag to export (for a review) so a "Fixes #$ISSUE." can be added to the commit message.

I'm unclear on whether or not this is "commit message bloat" and also on how we might specify it with the current commit messages produced.

/cc @fredsa @danholevoet

{{{
$ git rv submit
Checking out origin/review-9830044 at 9904b47aeb78defc1b9561064f048061a444bd32.
Adding reviewed commits.
Adding commit:
update part5-templates branch to match documentation https://developers.google.com/appengine/docs/p…

Reviewed in https://codereview.appspot.com/9830044/
Replacing review branch 'part5-templates' with newly committed content.
Your browser has been opened to visit:

https://codereview.appspot.com/get-access-token?port=8001

If your browser is on a different machine then exit and re-run
upload.py with the command-line parameter

--no_oauth2_webbrowser

Created new window in existing browser session.
Traceback (most recent call last):
File "/usr/lib/python2.7/runpy.py", line 162, in _run_module_as_main
"main", fname, loader, pkg_name)
File "/usr/lib/python2.7/runpy.py", line 72, in _run_code
exec code in run_globals
File "main.py", line 28, in
File "main.py", line 23, in main
File "submit.py", line 123, in callback
File "submit.py", line 99, in init
File "submit.py", line 411, in advance
File "submit.py", line 140, in verify_approval
File "submit.py", line 413, in advance
File "submit.py", line 159, in update_from_metadata
File "submit.py", line 415, in advance
File "submit.py", line 174, in create_branch
File "submit.py", line 417, in advance
File "submit.py", line 205, in commit
File "submit.py", line 419, in advance
File "submit.py", line 227, in push_commit
File "submit.py", line 423, in advance
File "submit.py", line 288, in clean_up_local
File "submit.py", line 425, in advance
File "submit.py", line 390, in clean_up_review
File "submit.py", line 340, in __add_commit_link
File "/usr/lib/python2.7/urllib.py", line 1312, in urlencode
v = quote_plus(str(v))
UnicodeEncodeError: 'ascii' codec can't encode character u'\u2026' in position 99: ordinal not in range(128)
}}

The browser URL was:

http://localhost:8001/?access_token=ya29.AHES6ZQwg3I3MFGYW7rXMqlGoUPzHcZ7lEzWUphecdgrwhxVyil4

git rv should explicitly log all git tasks it performs. This may result in verbose output, so having instead a -v flag to ask for verbosity may be better.

This could likely be accomplished by plugging into capture_command or it's eventual successor (hopefully Pygit2).

Suggested changes:

1. Show precisely the commands being executed:

git checkout master
git merge ....

2. Tell the user that we're pushing to origin

git push origin

3. Also show the output of each git command, e.g.

Checking out master.
$ git checkout master
> Switched to branch 'master'

$ git merge somebranch
> Updating cff8a30..97ec29e
> Fast-forward
> .....

$ git push origin
> .....

/cc @fredsa

export should fail while there is a pending sync

If an export occurs of the commit that was supposed to be used for git rv sync --continue, then the check for exactly one commit will fail and sync will be in a permanently busted state.

We currently assume if git rv sync --continue is called that there was a bad sync; from sync.py:

        commits = utils.get_commits(self.__last_commit, 'HEAD')
        if len(commits) == 0:
            print 'Please make a commit after resolving the merge conflict.'
            self.state = FINISHED

Instead we should also check if HEAD is clean and print a different notification expressing that --continue is not need since no sync had been attempted.

/cc @proppy

Document how to use https://github.com/rfk/git-remote-hg with git-rv

I have been using git-rv in congress with git-remote-hg to do code reviews for

• https://code.google.com/p/rietveld/
• https://code.google.com/p/httplib/
• https://code.google.com/p/google-api-python-client/

SIDE-NOTE: We may need to add a "Type:Docs" label. Who has more red?

Currently, git rv export will run even if there are no commits, because it could potentially update the metadata, i.e. a person may want to change the issue subject without sending any changes:

git rv export --message="I didn't do anything. Ha!"

However, this still works before a review has begun, which should not occur.

Example:

• First clone a new repo

$ git clone https://github.com/GoogleCloudPlatform/git-rv
$ cd git-rv

• Do absolutely nothing
• Try to export your nothing changes

$ git rv export -r [email protected]
No output from ['git', 'diff', '--no-color', '--no-ext-diff', '--full-index', '--ignore-submodules', SOMEHASH']

and realize that upload.py refused to upload anything, this ending the command.

• Check to make sure nothing bad happened

$ git rv getinfo
{
  "reviewers": [
    "[email protected]"
  ], 
  "private": false, 
  "remote_info": {
    "url": "https://github.com/GoogleCloudPlatform/git-rv", 
    "last_synced": "SOMEHASH", 
    "remote": "origin", 
    "commit_hash": "SOMEHASH", 
    "branch": "master"
  }, 
  "server": "codereview.appspot.com"
}

Oh no!

/cc @fredsa

git rv sync needs to be able to pass the --no_oauth2_webbrowser flag and other auth related flags to to ExportAction when it calls it.

"Allow 'git rv export' to specify one or more commit hashes. This would cause git rv to create a new clean branch (off of origin/master), then cherry-pick the list of commits onto this new branch, then 'git rv export' the branch."

/cc @fredsa

Rather than fetching upload.py over the network, include Rietveld as a submodule via git-remote-hg.

This will allow us to have simple access to upload.py in make_executable.py.

Specifically, the metadata is retrieved without any tokens:
https://github.com/GoogleCloudPlatform/git-rv/blob/0713c4cb794ae81b113e5abcda5c9e4523dde8bd/utils.py#L1375

/cc @PaulRashidi

Currently the review is uploaded as soon as authenticated, but no preview is shown, so it may include commits you were not expecting.

It'd be good to have a preview and ask for a confirmation before creating the CR and issue, otherwise one needs to delete the issue and get rid of the local CR (as discussed on issue #51).

This is a good re-factor, but the optimal re-factor would be to use Pygit2 and get rid of all system calls.

See discussion from original review.

/cc @proppy

This makes the tool generic enough to use other backends or other libraries in the place of upload.py.

It's likely that using pygit2 will be crucial in such a decoupling.

For example, if

bossylobster:dhermes-projects (some-branch) dhermes$ git merge --squash master
Already up-to-date!
Squash commit -- not updating HEAD
Automatic merge went well; stopped before committing as requested

where the review is actually out of sync (via this snipped rv getinfo):

bossylobster:dhermes-projects (some-branch) dhermes$ git rv getinfo
{
  "remote_info": {
    "last_synced": "79001cd606e449c3514fdff012d4d917e740ac5e", 
  }
}

and the HEAD commit does not match

bossylobster:dhermes-projects (some-branch) dhermes$ git log -2 master --pretty=%H
26ae14fbc54422648a48c7f561a67ebe7f7ed899
79001cd606e449c3514fdff012d4d917e740ac5e

Actually running the code that fails by hand (the stacktrace not so helpful), we get

>>> args = ('git', 'commit', '-m', 
                  'Syncing ...')
>>> proc = subprocess.Popen(args, stdout=subprocess.PIPE,
                            stderr=subprocess.PIPE)
>>> print proc.wait()
1
>>> print proc.stdout.read()
# On branch no-oauth-sync
# Untracked files:
#   (use "git add <file>..." to include in what will be committed)
#
#   HOW.txt
nothing added to commit but untracked files present (use "git add" to track)
>>> print repr(proc.stderr.read())
''

Context: I had not pushed my changes merged from upstream, so my fork repository had some commits that were not intended for the code review. Due to one of them, git-rv export just fails.

Traceback (most recent call last):
  File "/usr/lib/python2.7/runpy.py", line 162, in _run_module_as_main
    "__main__", fname, loader, pkg_name)
  File "/usr/lib/python2.7/runpy.py", line 72, in _run_code
    exec code in run_globals
  File "__main__.py", line 28, in <module>
  File "__main__.py", line 23, in main
  File "export.py", line 218, in callback
  File "export.py", line 113, in __init__
  File "export.py", line 168, in __get_commit_message_parts
  File "utils.py", line 456, in get_user_commit_message_parts
  File "utils.py", line 358, in get_commit_message_parts
utils.GitRvException: Commit message:
'- strings are now assumed to always be Unicode\nThis fixes a bug where this caused a crash:\n\nLANG=C bin/logstash agent -e \'filter { mutate { replace => { "message"\n=> "\xe2\x98\xb9" } } } output { stdout { codec => json } }\'\n\nThe problem was that LANG=C told ruby\'s eval() what the encoding of\nstrings was by default, and since logstash requires UTF-8/unicode\nelswhere, it makes sense to require it for the config file as well.'
does not begin with the subject:
'- strings are now assumed to always be Unicode This fixes a bug where this caused a crash:'.

These are the recent log messages (the one starting with "- strings" seem to have caused the issue)

commit 522d8b029e8416dcbac795297dbc1a841b11407e
Author: Rodrigo De Castro <[email protected]>
Date:   Thu Sep 12 14:15:29 2013 -0700

    Add first version of BQ output plugin.

commit 29de30745138ddcb69a2b45b8ebf3e5a1c39b58a
Author: Jordan Sissel <[email protected]>
Date:   Wed Sep 11 11:02:20 2013 -0700

    .

commit 7ca972e8d11337142fd841e6f175cd3f744b76e5
Author: Jordan Sissel <[email protected]>
Date:   Mon Sep 9 11:01:30 2013 -0700

    - strings are now assumed to always be Unicode
    This fixes a bug where this caused a crash:

    LANG=C bin/logstash agent -e 'filter { mutate { replace => { "message"
    => "☹" } } } output { stdout { codec => json } }'

    The problem was that LANG=C told ruby's eval() what the encoding of
    strings was by default, and since logstash requires UTF-8/unicode
    elswhere, it makes sense to require it for the config file as well.

commit 45ac715e36e6b0074e7233d709eefee28b50d650
Author: Jordan Sissel <[email protected]>
Date:   Mon Sep 9 07:35:42 2013 -0700

    - fix docs

commit f1d6d1d71cef10067180ef2d2fac401d6356483c
Author: Jordan Sissel <[email protected]>
Date:   Sun Sep 8 21:01:47 2013 -0700

    - swear less, and never in documentation.

version command:

Provide the "release" version of the git-rv binary via a command like git rv version

update command:

Update the git-rv binary in place. I know to do this in pure Python would likely require using os.fork to create a process that will download the new version and be ready to replace it after the current version stops running the update command.

After several changes in Rietveld, OAuth 2.0 is fully supported by upload.py:

• https://code.google.com/p/rietveld/source/detail?r=21bfd85693bbee22c5ea318669563074b519ca57
• https://code.google.com/p/rietveld/source/detail?r=c379eb429d33267182473d603bee9edcd3c5e624
• https://code.google.com/p/rietveld/source/detail?r=318844ab4d6d423adc97514caaf1488b00e0c38d
• https://code.google.com/p/rietveld/source/detail?r=8cc30f2401c1a9e247bc0b9ec6c6685304905291
• https://code.google.com/p/rietveld/source/detail?r=d3ad0a8b35764ab0a35058e64a48b33df5b194c6
• https://code.google.com/p/rietveld/source/detail?r=c8ec47581adb274d868d9141fa261bdea4f73e7d

As is done with --send_mail, we should make this flag always True by default and potentially not allow any other kind of auth?

/cc @fredsa @danholevoet @proppy @tmatsuo

From the initial review, we can replace callback with action. For example, instead of

parser_export.set_defaults(callback=ExportAction.callback)

we should simply say

parser_export.set_defaults(action=ExportAction)

and move the extra behavior of the callback into the constructor.

This is simple since in __main__.py we are explicitly assuming we know the signature of each callback.

This is "simple" using webbrowser module in Python, though isn't guaranteed to always work, especially on remote machines.

https://github.com/GoogleCloudPlatform/git-rv/blob/8e2cb4435344d5555137f72fd97b776e2c674070/utils.py#L316

Example stack trace from when trying to fix #1 and using git-rv for the review

Checking out origin/review-9188043 at ee57c6d25917b5e1f43b5a3fc284e8f4d8648636.
Adding reviewed commits.
Traceback (most recent call last):
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/runpy.py", line 162, in _run_module_as_main
    "__main__", fname, loader, pkg_name)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/runpy.py", line 72, in _run_code
    exec code in run_globals
  File "/Users/dhermes/git-rv/__main__.py", line 28, in <module>
    sys.exit(main(sys.argv))
  File "/Users/dhermes/git-rv/__main__.py", line 23, in main
    args.callback(args, remaining)
  File "/Users/dhermes/git-rv/submit.py", line 128, in callback
    return cls(rpc_server_args=rpc_server_args, do_close=args.do_close)
  File "/Users/dhermes/git-rv/submit.py", line 104, in __init__
    self.advance()
  File "/Users/dhermes/git-rv/submit.py", line 423, in advance
    self.verify_approval(*args, **kwargs)
  File "/Users/dhermes/git-rv/submit.py", line 145, in verify_approval
    self.advance()
  File "/Users/dhermes/git-rv/submit.py", line 425, in advance
    self.update_from_metadata(*args, **kwargs)
  File "/Users/dhermes/git-rv/submit.py", line 163, in update_from_metadata
    self.advance()
  File "/Users/dhermes/git-rv/submit.py", line 427, in advance
    self.create_branch(*args, **kwargs)
  File "/Users/dhermes/git-rv/submit.py", line 178, in create_branch
    self.advance()
  File "/Users/dhermes/git-rv/submit.py", line 429, in advance
    self.commit(*args, **kwargs)
  File "/Users/dhermes/git-rv/submit.py", line 198, in commit
    single_line=False)
  File "/Users/dhermes/git-rv/utils.py", line 206, in capture_command
    raise GitRvException('Command %r failed with:\n%s' % (command, stderr))
utils.GitRvException: Command 'git merge --squash add-oauth' failed with:
warning: Cannot merge binary files: git-rv (HEAD vs. add-oauth)

Potential solutions:

1. Find common ancestor between current branch and the {remote}/{branch} the user selects.
2. When --contains failure occurs on HEAD, check the common ancestor and report this in the error.

As in this commit, both GitHub and Google Code Hosting will cut off commits with first line longer than 100 characters and so they should.

We should enforce these values by making sure the subject stays below 100 characters and keeping the rest of the commit in the body of message.

/cc @fredsa

For example, in the middle of a review, if we have

$ git rv getinfo
...
   "remote_info": {
     "url": "hg::https://code.google.com/p/rietveld/",
     "last_synced": "4d1dc510d4ae42637f416e29ac982d53bc9940b1",
     "remote": "origin",
     "commit_hash": "4d1dc510d4ae42637f416e29ac982d53bc9940b1",
     "branch": "master"
   }
...

Then, we can convert the git hashes to the hg hashes with something like the following:

url = 'hg::https://code.google.com/p/rietveld'
assert url.startswith('hg::')
_, url = url.split('hg::', 1)
import urllib
url = urllib.quote_plus(url)
import os
path = os.path.join('.git', 'hgremotes', url, '.hg', 'git-mapfile')
real_path = os.path.join(GIT_ROOT, path)


with open(real_path, 'r') as fh:
  content = fh.read().rstrip()
git_to_hg = dict(row.split(' ', 1) for row in content.split('\n'))


hg_hash = git_to_hg[git_hash]

This will go in utils.py and will added as part of the regexes in GoogleCodehostingRepositoryInfo and GithubRepositoryInfo.

In the rm-branch command, if branch to be deleted has an active review with a Rietveld issue, delete or close that issue in addition to remove all the local review data.

See this review for example, where git-rv linked to:

https://code.google.com/p/cloud-playground.mimic/source/detail?r=53e1762f16ed9cbda566e0ba80c31313273cc3c8

but that URL is wrong. It should be:

https://code.google.com/p/cloud-playground/source/detail?r=53e1762f16ed9cbda566e0ba80c31313273cc3c&repo=mimic

/cc @fredsa

Currently, we are passing (at most)

• choices
• dest
• metavar
• const
• nargs
• default
• type

but the constructor for both optparse and argparse parsers can also take an action, and the list of actions is the same between the two.

I already have a fix ready and will submit it soon.

git rv sync --continue fails if merge conflict ends up not requiring a commit.

Here is the relevant code:
https://github.com/GoogleCloudPlatform/git-rv/blob/8236118f961c1ff4775228c2b9058f45c65a7622/sync.py#L141

I need to do some research on this. After a merge, it seems if there are no needed changes after resolving conflicts that there should be an "empty" commit to change the parent hash, but I haven't been able to verify this behavior.

/cc @fredsa

Previously (see contents from initial review), there existed pack and unpack commands to create a zip and unpack a zip to pass reviews around, but this failed due to a poor understanding of git commit objects and compression used for large git projects.

Instead, we should use the Rietveld info stored along with the patch objects to allow patching in a review "over HTTP".

Potential issues:

1. This may require features to be added to Rietveld
2. We may run into issues relating to diff formats. This is discussed in an issue and in the Rietveld forum. It seems it is specific to Mercurial, but it is worth noting for future reference.

/cc @fredsa @danholevoet

EDIT: Issue title was "git rv submit sometimes hangs trying to update rietveld"

1. Type git rv submit
2. Command line output reads:

Checking out origin/review-9780046 at d0e512fda912a85fa6b10faf223f60325c1bda73.
Adding reviewed commits.
Adding commit:
add missing index.yaml to part5-staticfiles branch

Reviewed in https://codereview.appspot.com/9780046/
Replacing review branch 'part5-templates' with newly committed content.
Your browser has been opened to visit:

    https://codereview.appspot.com/get-access-token?port=8001

If your browser is on a different machine then exit and re-run
upload.py with the command-line parameter

  --no_oauth2_webbrowser

Created new window in existing browser session.

3. After waiting 10 seconds, hit CTRL-C for traceback

^CTraceback (most recent call last):
  File "/usr/lib/python2.7/runpy.py", line 162, in _run_module_as_main
    "__main__", fname, loader, pkg_name)
  File "/usr/lib/python2.7/runpy.py", line 72, in _run_code

    exec code in run_globals
  File "__main__.py", line 28, in <module>
  File "__main__.py", line 23, in main
  File "submit.py", line 123, in callback
  File "submit.py", line 99, in __init__
  File "submit.py", line 411, in advance
  File "submit.py", line 140, in verify_approval
  File "submit.py", line 413, in advance
  File "submit.py", line 159, in update_from_metadata
  File "submit.py", line 415, in advance
  File "submit.py", line 174, in create_branch
  File "submit.py", line 417, in advance
  File "submit.py", line 205, in commit
  File "submit.py", line 419, in advance
  File "submit.py", line 227, in push_commit
  File "submit.py", line 423, in advance
  File "submit.py", line 288, in clean_up_local
  File "submit.py", line 425, in advance
  File "submit.py", line 385, in clean_up_review
  File "submit.py", line 303, in __get_xsrf_server
  File "upload.py", line 467, in _Authenticate
  File "upload.py", line 844, in __call__
  File "upload.py", line 779, in GetAccessToken
  File "upload.py", line 756, in WaitForAccessToken
  File "/usr/lib/python2.7/SocketServer.py", line 265, in handle_request
    fd_sets = select.select([self], [], [], timeout)
KeyboardInterrupt

DISCUSSION: Provide the option to condense each commit message from all the micro-commits in a review into a single commit message.

This is somewhat related to #12 but not quite the same.

With this discussion we should also addres the question of whether or not to allow an option to commit and git push all the micro-commits as individual commits.

/cc @danholevoet

git-rv should (provide the option to) print git status and its output at the end of git rv submit

$ git rv submit
Checking out master.
Adding reviewed commits to master.
Adding commit:
improve the thing

Reviewed in https://codereview.appspot.com/123456/
Loaded authentication cookies from /some/path
Issue 123456 has been closed.

> $ git status
> # On branch master
> nothing to commit, working directory clean

/cc @fredsa

In git-rv sync, don't export if no changes occurred in the files being reviewed. It may not be so difficult to parse this from the git diff output after doing the merge, but I'm not sure.

I'm not sure if this is worth it, so marking as a discussion as well.

If there are multiple commits, one of which may fail the 100 character check, the script fails before the user has selected that commit to use.

This is unnecessary and failure would only be needed if the user explicitly selected the bad commit.

/cc @jsimonweb

Using setuptools, it'd be optimal to package git-rv via setup.py build instead of the current flow in make_executable.py.

Some reference on distribute/eggsecutable-scripts.

/cc @proppy

This would follow the same lookup path as git config, e.g.:

1. Local: $PROJECT_ROOT/.git/config
2. User Global: $HOME/.gitconfig
3. Global: /etc/gitconfig

I'm marking this as a discussion because we should figure out what would belong in there and at what places it would be relevant to interject this config into git-rv commands.

I think both git aliases and hg plugins should be looked at before making any concrete design decisions.

/cc @fredsa @proppy

Suggest defaulting to 'N'

Example output:

Delete branch 'foo'` and checkout 'master' [y/N]?

Would need to figure out if master or its equivalent is tracking the remote branch associated with the review.

/cc @fredsa

During export, if a commit message exceeds 100 characters, git-rv bails with an error. Assuming the restriction can't be overcome or extended, a cleaner error message (maybe with hints on how to edit the commit message: git rebase --interactive?) would be more appropriate.

Traceback (most recent call last):
File "/usr/lib/python2.7/runpy.py", line 162, in _run_module_as_main
"main", fname, loader, pkg_name)
File "/usr/lib/python2.7/runpy.py", line 72, in _run_code
exec code in run_globals
File "main.py", line 28, in
File "main.py", line 23, in main
File "export.py", line 218, in callback
File "export.py", line 113, in init
File "export.py", line 168, in __get_commit_message_parts
File "utils.py", line 456, in get_user_commit_message_parts
File "utils.py", line 341, in get_commit_message_parts
utils.GitRvException: Commit subject '...' exceeds 100 characters.

i.e. If the remote branch you are tracking in your local branch is called "master" and your current local branch is also called "master", display a message/prompt asking the user if they want to proceed or create a new branch to export the review from.

It is my personal preference to do reviews in "named" branches per feature and leave one branch as "always clean", though this workflow/preference differs by user.

/cc @fredsa

This is used in submit.py when replacing the review branch with a "copy" based on what was sent to the remote.

Essentially, when I originally wrote the method I thought was necessary, but it's possible that

git branch local-branch-name remote-name/remote-branch-name

is sufficient without the --track flag, or with --no-track. It's also possible that we should keep around whether or not the branch is a tracking branch before destroying/replacing it.

Due to issue #50 my export failed. I wanted to try again after tidying up my repository, but did not find an option to cancel my code review. My workaround was to clone the repository again, reapply the changes, and resubmit the code review.

Please allow code reviews to be canceled or, even better, if the export fails for some reason (like issue #50), that a code review is not created.

We can accomplish this by using lines that begin with # and ignoring them when creating the commit in submit.

We can optionally add a prompt when we encounter lines beginning with # that we don't expect asking the user if scrubbing them is OK.

/cc @fredsa

When trying to export with multiple commits since the last export I see

$ git rv export
 You have made more than one commit since HEAD in origin/master.
 Please choose one of the following as your commit message:
 0: allow inline
 1: another content type
 2: send files as 'nosniff' attachments
 Message:

and type

 Message: My custom commit message is written here

in the prompt.

I expect git-rv to use the message that I typed, but instead get a traceback:

Traceback (most recent call last):
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/runpy.py", line 162, in _run_module_as_main
    "__main__", fname, loader, pkg_name)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/runpy.py", line 72, in _run_code
    exec code in run_globals
  File "__main__.py", line 28, in <module>
  File "__main__.py", line 23, in main
  File "export.py", line 159, in callback
  File "export.py", line 76, in __init__
  File "export.py", line 124, in __get_current_message
  File "utils.py", line 372, in get_user_commit_message
  File "utils.py", line 314, in user_choice_from_list
utils.GitRvException: Message choice My custom commit message is written here' is invalid.

Instead of raising an exception, the error message should clearly state the allowed values there. The prompt should say something like "Message #:" and the error message should mention that the user should use "-m" or "--message" to use a custom message.

It may also be worth discussing whether a custom value should be allowed here. The current expected behavior is that the user either types one of the integer values (0, 1, or 2) or exactly one of the full text choices.

/cc @fredsa

Add git rv diff.

This will show the diff from the current state and the last synced commit in a review.