Add a revert script.

tools/revert.py

+import optparse

[Siggi Cherem (dart-lang), 2012/05/08 01:16:07]

+ #!/usr/bin/env python
+ # Copyright ...

[Emily Fortuna, 2012/05/08 17:33:27]

Done.

+import os
+import platform
+import subprocess
+import sys
+
+"""A script used to revert one or a sequence of consecutive CLs, for svn and
+git-svn users."""

[Siggi Cherem (dart-lang), 2012/05/08 01:16:07]

wrap """ to next line (not sure if this is in the styleguide, but I tend to do
this for multi-line documentation)

[Emily Fortuna, 2012/05/08 17:33:27]

Done.

+
+def parse_args():
+  parser = optparse.OptionParser()
+  parser.add_option('--range', '-r', dest='rev_range', action='store',

[Emily Fortuna, 2012/05/08 00:40:01]

Yes, this could/should have been a positional argument, but I wanted to provide
a better -h message to explain how to use this script.

[Siggi Cherem (dart-lang), 2012/05/08 01:16:07]

maybe rename it to --revisions instead of range? 

[Emily Fortuna, 2012/05/08 17:33:27]

Done.

+                    default=None, help='The revision number(s) of the commits '
+                    'you wish to undo. An individual number, or a range 8-10.')

[Siggi Cherem (dart-lang), 2012/05/08 01:16:07]

in git it's common to specify a range as A..B, maybe we can use that syntax?

[Emily Fortuna, 2012/05/08 17:33:27]

Done.

+  args, _ = parser.parse_args()
+  revision_range = args.rev_range
+  if revision_range == None:
+    fail('You must specify at least one revision number to revert.')
+  if revision_range.find('-') > -1 or revision_range.find(':') > -1:

[Siggi Cherem (dart-lang), 2012/05/08 01:16:07]

oh - I guess I'm suggesting a third syntax :).

Either way is ok, but if we have more than one, we should include an example in
the help message. (e.g. 8:10, or 8..10)

[Emily Fortuna, 2012/05/08 17:33:27]

Done. Yes, the colon version is svn-like, so I'll add the git version, too. :-)

+    # We have a range of commits to revert.
+    split = revision_range.split('-')
+    if len(revision_range.split('-')) == 1:

[Siggi Cherem (dart-lang), 2012/05/08 01:16:07]

len(revision_range.split('-')) => len(split)

[Emily Fortuna, 2012/05/08 17:33:27]

Done.

+      split = revision_range.split(':')
+    start = int(split[0])
+    end = int(split[1])
+    if start > end:
+      temp = start
+      start = end
+      end = temp
+    fail('Warning: are you sure you want to revert a range of revisions? If '

[Siggi Cherem (dart-lang), 2012/05/08 01:16:07]

maybe check if start != end (if they are equal, we can skip this error)

[Emily Fortuna, 2012/05/08 17:33:27]

Done, though it will be strange if someone specifies a range from 100..100, but
I should definitely deal with this case.

+         'you just want to revert one CL, only specify one revision number.',
+         user_input=True)
+  else:
+    start = end = int(revision_range)
+  return start, end
+
+def fail(error_msg, user_input=False):

[Siggi Cherem (dart-lang), 2012/05/08 01:16:07]

fail => maybe_fail? or prompt_user?

[Emily Fortuna, 2012/05/08 17:33:27]

Done.

[Emily Fortuna, 2012/05/08 17:33:27]

Done.

+  """Determine if we have encountered a condition upon which our script cannot
+  continue, and abort if so.
+  Args:
+    - error_msg: The error message to print.
+    - user_input: True if we require user confirmation to continue. We assume

[Siggi Cherem (dart-lang), 2012/05/08 01:16:07]

in this case, the message is not necessarily an error. Maybe rename error_msg to
simply 'msg'

[Emily Fortuna, 2012/05/08 17:33:27]

Done.

+                  that the user must enter y to proceed.
+  """
+  if user_input:
+    force = raw_input(error_msg + ' (y/N)')
+    if force != 'y':
+      sys.exit(0)
+  else:
+    print error_msg
+    sys.exit(1)
+
+def has_new_code(is_git):
+  """Tests if there are any newer versions of files on the server.
+  Args:
+    - is_git: True if we are working in a git repository.
+  """
+  os.chdir(os.path.join(os.path.dirname(os.path.dirname(os.path.abspath(

[Siggi Cherem (dart-lang), 2012/05/08 01:16:07]

join seems unnecessary?

[Emily Fortuna, 2012/05/08 17:33:27]

Oops, that was a refactoring gone wrong that I missed.
Done.

+      __file__)))))
+  if not is_git:
+    results = run_cmd(['svn', 'st'])
+  else:
+    results = run_cmd(['git', 'status'])
+  for line in results.split('\n'):
+    if not is_git and (not line.strip().startswith('?') and line != ''):
+      return True
+    elif is_git and ('Changes to be committed' in line or 
+        'Changes not staged for commit:' in line):
+      return True
+  if is_git:
+    p = subprocess.Popen(['git', 'log'], stdout=subprocess.PIPE)

[Siggi Cherem (dart-lang), 2012/05/08 01:16:07]

should we add here '-1' to this git log command? It seems that find_git_info
will find a revision number in older commits, and we'll return False here, even
when there are local commits.

[Emily Fortuna, 2012/05/08 17:33:27]

When there are local commits, a commit name is listed, but a commit_id is not,
so find_git_info returns None. But you're right -1 is only what's really needed
to determine that.

+    output, _ = p.communicate()
+    if find_git_info(output) == None:
+      return True
+  return False
+
+def run_cmd(cmd_list):
+  """Run the specified command and print out any output to stdout."""
+  print ' '.join(cmd_list)
+  out = subprocess.PIPE
+  p = subprocess.Popen(cmd_list, stdout=out, stderr=subprocess.PIPE,
+                       stdin=subprocess.PIPE,
+                       shell=(platform.system()=='Windows'))
+  output, stderr = p.communicate()
+  if output:
+    print output
+  if stderr:
+    print stderr
+  return output
+
+def runs_git():
+  """Returns True if we're standing in an svn-git repository."""
+  p = subprocess.Popen(['svn', 'info'], stdout=subprocess.PIPE,
+                       stderr=subprocess.PIPE)
+  output, err = p.communicate()
+  if err != None and 'is not a working copy' in err:
+    p = subprocess.Popen(['git', 'status'], stdout=subprocess.PIPE)
+    output, _ = p.communicate()
+    if 'fatal: Not a git repository' in output:
+      fail('Error: not running git or svn.')
+    else:
+      return True
+  return False
+
+def find_git_info(git_log, rev_num=None):
+  """Determine the latest svn revision number if rev_num = None, or find the 
+  git commit_id that corresponds to a particular svn revision number."""

[Siggi Cherem (dart-lang), 2012/05/08 01:16:07]

wrap """ to next line

[Emily Fortuna, 2012/05/08 17:33:27]

Done.

+  for line in git_log.split('\n'):
+    tokens = line.split()
+    if len(tokens) == 2 and tokens[0] == 'commit':
+      current_commit_id = tokens[1]
+    elif len(tokens) > 0 and tokens[0] == 'git-svn-id:':
+      revision_number = int(tokens[1].split('@')[1])
+      if revision_number == rev_num:
+        return current_commit_id
+      if rev_num == None:
+        return revision_number
+
+def revert(start, end, is_git):
+  """Revert the sequence of CLs.
+  Args:
+    - start: The first CL to revert.
+    - end: The last CL to revert.
+    - is_git: True if we are in a git-svn checkout.
+  """
+  if not is_git:
+    run_cmd(['svn', 'merge', '-r', '%d:%d' % (end, start-1), '.'])
+  else:
+    # If we're running git, we have to use the log feature to find the commit
+    # id(s) that correspond to the particular revision number(s).
+    output = run_cmd(['git', 'log', '-1'])
+    current_revision = find_git_info(output)
+    distance = (current_revision-start) + 1
+    reverts = [start]
+    commit_msg = '"Reverting %d"' % start
+    if end != start:
+      reverts = range(start, end + 1)
+      reverts.reverse()

[Emily Fortuna, 2012/05/08 00:40:01]

these two lines (137-138) could certainly be done more efficiently, but I
decided to write it this way to clearly show what was going on.

+      commit_msg = '%s-%d"' % (commit_msg[:-1], end)
+    for revert in reverts:
+      git_commit_id = find_git_info(output, revert)
+      run_cmd(['git', 'revert', '-n', git_commit_id])
+    run_cmd(['git', 'commit', '-m', commit_msg])

[Siggi Cherem (dart-lang), 2012/05/08 01:16:07]

this command shouldn't be necessary (there shouldn't be any open files).

[Emily Fortuna, 2012/05/08 17:33:27]

This is done because when I run git revert, I pass the -n (no commit) flag,
because I want all of the reverts (potentially multiple in a sequence) to be one
big commit together. Is this a bad strategy?

[Siggi Cherem (dart-lang), 2012/05/08 17:48:23]

Ah - got it. It might be better to do it without the -n, I'm not sure how the -n
deals with subsequent changes in the same file. We can test it out.

+
+def main():
+  revisions = parse_args()
+  git_user = runs_git()
+  if has_new_code(git_user):
+    fail('WARNING: This checkout has local modifications!! This could result '
+         'in a CL that is not just a revert and/or you could lose your local '
+         'changes! Are you **SURE** you want to continue? ', user_input=True)

[Siggi Cherem (dart-lang), 2012/05/08 01:16:07]

In the case of git, I feel that we should just not let people continue and fail
(since all local changes will be part of the revert CL that will get submitted).

+  if git_user:
+    run_cmd(['git', 'cl', 'rebase'])
+  run_cmd(['gclient', 'sync'])
+  revert(revisions[0], revisions[1], git_user)

[Siggi Cherem (dart-lang), 2012/05/08 01:16:07]

after this, should we print a message saying: now create a CL and submit it!?

[Emily Fortuna, 2012/05/08 17:33:27]

Done.

[Emily Fortuna, 2012/05/08 17:33:27]

Done.

+
+if __name__ == '__main__':
+  main()