Add "git llvm revert" subcommand

[rupprecht]

Summary

The current git-svnrevert script only works with git-svn repos (e.g. using "git svn find-rev" to find the commit to revert). This adds a similar implementation that works with the llvm git command handler.

Usage:

// Revert by svn id
$ git llvm revert r123456
// Forgetting the 'r' is fine
$ git llvm revert 123456
// Git commit hash also fine
$ git llvm revert abc123456
// Push revert upstream (drop the -n)
$ git llvm push -n

Regardless of how the command is invoked (with a svn revision or git hash), the message is:

Revert [LibFoo] Change Foo implementation

This reverts r123456 (git commit abc123)

[jlebar]

Thank you for the patch!

llvm/utils/git-svn/git-llvm

122

Nit, perhaps we should use the idiom from https://stackoverflow.com/a/1070756

436

Is being able to do git svn revert <git hash> really that useful? It looks like it's basically the same as git revert <git hash>?

In fact I wonder if what we really want is git svn lookup that looks up the hash of an svn rev. Then you can do git revert $(git svn lookup r12345). That's more unix-y / more composible.

WDYT?

[rupprecht]

• Use shlex/pipes for better quoting

[rupprecht]

llvm/utils/git-svn/git-llvm

436

Is being able to do git svn revert <git hash> really that useful?

<shrug> I always use the svn revision, so it isn't useful for me. It may be more useful once we switch to github as a source of truth and git commit ids become a more common way of referencing commits. (I have no idea what the plan is for that -- will we even have something like svn ids then?). So I'm slightly leaning towards leaving it in.

(btw, I think you mean "git llvm revert"?)

It looks like it's basically the same as git revert <git hash>?

Basically, but with a prepopulated commit message that includes the svn revision, not the git commit hash. A few recent upstream reverts (including my own :( ) have included the git commit id, not the svn revision, which is confusing to some.

In fact I wonder if what we really want is git svn lookup that looks up the hash of an svn rev. Then you can do git revert $(git svn lookup r12345). That's more unix-y / more composible.

"git svn lookup" - I think that would only work in a git-svn repo? If someone already has a git-svn repo, they don't really need this, they can use llvm/utils/git-svn/git-svnrevert. This script is meant to work in a pure git repo.

[jlebar]

llvm/utils/git-svn/git-llvm

436

(btw, I think you mean "git llvm revert"?)

Yes, sorry. And I also meant git llvm lookup, although that's not a great name.

Once we switch to a monorepo, hopefully we'll be able to do plain git revert without any external commands, so I don't think we need to design around that now.

If the raison d'etre for this command is to provide good commit messages, then I see the value in allowing git llvm revert <git hash>, so I'm happy with this functionality as-is.

Can we document the fact that this is primarily about commit messages in cmd_revert.__doc__?

[mehdi_amini]

Looks great! Thanks.

llvm/utils/git-svn/git-llvm

436

git llvm svn-lookup? It would be great to have it, even if git llvm revert 123456 works as-is

[rupprecht]

• Extract some changes to git llvm svn-lookup command
• Add dry_run arg for revert
• Add docs about the message format

[jlebar]

Looks great, one nit and one unfortunately substantial comment, sorry I didn't think about it earlier.

llvm/utils/git-svn/git-llvm

410

Could we just return an int here and leave the 'r' + str(lookup_llvm_svn_id(rev)) to the caller (i.e. remove the include_r argument)? This seems like a better separation of concerns.

438

Upon reflection: The chance that a 5-digit git hex hash contains only decimal digits is (10/16)^5 = 9%. So I don't think it's going to work here or in cmd_revert to say that a five-decimal-digit number is always an svn revision.

Instead maybe we say, it's an svn rev if it starts with r, otherwise it's a git rev?

Once we do that then I'd say that lookup_llvm_svn_id should only operate on git hashes, because right now we have the strange behavior that cmd_svn_lookup will accept an SVN revision, but that's just the identity function.

[rupprecht]

• lookup_llvm_svn_id should return svn id without the r
• svn-lookup should only be used for git commit hashes

llvm/utils/git-svn/git-llvm

438

It's not *quite* an identity function. Given r123456 it returns 123456 so we can grep for llvm-svn: 123456. (For just 123456, it is the identity).

But that's not a good enough reason to keep it there.

[jyknight]

llvm/trunk/utils/git-svn/git-llvm

414 ↗

(On Diff #192653)

This should search for "^llvm-svn: ...", in re.MULTILINE mode, so it's not confused by "fake" revisions.

See for example, 13c4bc567150f98adb0209aab16cc6d9ba369c7f. The first "> llvm-svn: 355168" came from the user's description committed to svn, since they copied it from the previous attempt. Thus it had a "> " inserted before it by the conversion script.

462 ↗

(On Diff #192653)

Same here, should be '^llvm-svn: ' +

llvm/utils/git-svn/git-llvm

except AttributeError:
     # Python 3
     def iteritems(d):
         return iter(d.items())
 else:
     # Python 2
     def iteritems(d):
         return d.iteritems()
 
+try:
+    # Python 3
+    from shlex import quote
+except ImportError:
+    # Python 2
+    from pipes import quote
+
 # It's *almost* a straightforward mapping from the monorepo to svn...
 GIT_TO_SVN_DIR = {
     d: (d + '/trunk')
     for d in [
         'clang-tools-extra',
         'compiler-rt',
         'debuginfo-tests',
         'dragonegg',
def get_dev_null():
     global dev_null_fd
     if dev_null_fd is None:
         dev_null_fd = open(os.devnull, 'w')
     return dev_null_fd
 
 
 def shell(cmd, strip=True, cwd=None, stdin=None, die_on_failure=True,
           ignore_errors=False, text=True):
-    log_verbose('Running in %s: %s' % (cwd, ' '.join(cmd)))
+    # Escape args when logging for easy repro.
+    quoted_cmd = [quote(arg) for arg in cmd]
+    log_verbose('Running in %s: %s' % (cwd, ' '.join(quoted_cmd)))
 
     err_pipe = subprocess.PIPE
     if ignore_errors:
         # Silence errors if requested.
         err_pipe = get_dev_null()
 
     start = time.time()
     p = subprocess.Popen(cmd, cwd=cwd, stdout=subprocess.PIPE, stderr=err_pipe,
                          stdin=subprocess.PIPE,
                          universal_newlines=text)
     stdout, stderr = p.communicate(input=stdin)
     elapsed = time.time() - start
 
     log_verbose('Command took %0.1fs' % elapsed)
 
     if p.returncode == 0 or ignore_errors:
         if stderr and not ignore_errors:
-            eprint('`%s` printed to stderr:' % ' '.join(cmd))
+            eprint('`%s` printed to stderr:' % ' '.join(quoted_cmd))
             eprint(stderr.rstrip())
         if strip:
             if text:
                 stdout = stdout.rstrip('\r\n')
             else:
                 stdout = stdout.rstrip(b'\r\n')
         if VERBOSE:
             for l in stdout.splitlines():
                 log_verbose("STDOUT: %s" % l)
         return stdout
-    err_msg = '`%s` returned %s' % (' '.join(cmd), p.returncode)
+    err_msg = '`%s` returned %s' % (' '.join(quoted_cmd), p.returncode)
     eprint(err_msg)
     if stderr:
         eprint(stderr.rstrip())
     if die_on_failure:
         sys.exit(2)
     raise RuntimeError(err_msg)
 
 
log('Pushing %d commit%s:\n%s' %
         (len(revs), 's' if len(revs) != 1
          else '', '\n'.join('  ' + git('show', '--oneline', '--quiet', c)
                             for c in revs)))
     for r in revs:
         clean_svn(svn_root)
         svn_push_one_rev(svn_root, r, dry_run)
 
 
+def cmd_revert(args):
+    '''Revert a commit by either SVN id (rNNNNNN) or git hash.'''
+    # Get the git root
+    git_root = git('rev-parse', '--show-toplevel')
+    if not os.path.isdir(git_root):
+        die("Can't find git root dir")
+
+    # Run commands from the root
+    os.chdir(git_root)
+
+    # Check for a client branch first.
+    open_files = git('status', '-uno', '-s', '--porcelain')
+    if len(open_files) > 0:
+      die("Found open files. Please stash and then revert.\n" + open_files)
+
+    # Search by svn id if it looks like rNNNNNN or NNNNNN, otherwise search by
+    # git commit.
+    svn_match = re.match('^r?(\d{5,7})$', args.revision)
+    if svn_match:
+      svn_rev = svn_match.group(1)
+      log_verbose('Searching by svn id for revision r' + svn_rev)
+      oneline = git('log', '--all',  '-1', '--grep', 'llvm-svn: ' + svn_rev,
+                    '--oneline')
+      if len(oneline) == 0:
+        die("Can't find svn revision r" + svn_rev)
+    else:
+      log_verbose('Searching by git hash for commit ' + args.revision)
+      # Ignore errors so we can print a friendlier message. Also, use a format
+      # that looks line --oneline for the first line so we can share parsing
+      # logic.
+      full_msg = git('log', '-1', '--format=%h %s%n%b', args.revision,
+                     ignore_errors=True)
+      if len(full_msg) == 0:
+        die("Can't find git commit " + args.revision)
+      (oneline, rest) = full_msg.split('\n', 1)
+      svn_match = re.search('llvm-svn: (\d{5,7})$', rest)
+      if svn_match:
+        svn_rev = svn_match.group(1)
+      else:
+        die("Can't find svn revision in git commit " + args.revision)
+    (git_hash, msg) = oneline.split(' ', 1)
+
+    log_verbose('Ready to revert r%s/%s: "%s"' % (svn_rev, git_hash, msg))
+    git('revert', '--no-commit', git_hash)
+    # TODO: Running --edit doesn't seem to work, with errors that stdin is not
+    # a tty.
+    commit_log = git(
+        'commit', '-m', 'Revert ' + msg,
+        '-m', 'This reverts r%s (git commit %s)' % (svn_rev, git_hash))
+    log('Created revert of r%s: %s' % (svn_rev, commit_log))
+    log("Run 'git llvm push -n' to inspect your changes and "
+        "run 'git llvm push' when ready")
+
 if __name__ == '__main__':
     if not program_exists('svn'):
         die('error: git-llvm needs svn command, but svn is not installed.')
 
     argv = sys.argv[1:]
     p = argparse.ArgumentParser(
         prog='git llvm', formatter_class=argparse.RawDescriptionHelpFormatter,
         description=__doc__)
parser_push.add_argument(
         'rev_range',
         metavar='GIT_REVS',
         type=str,
         nargs='?',
         help="revs to push (default: everything not in the branch's "
         'upstream, or not in origin/master if the branch lacks '
         'an explicit upstream)')
     parser_push.set_defaults(func=cmd_push)
+
+    parser_revert = subcommands.add_parser(
+        'revert', description=cmd_revert.__doc__,
+        help='Revert a commit locally.')
+    parser_revert.add_argument(
+        'revision',
+        help='Revision to revert. Can either be an SVN revision number '
+        "(rNNNNNN or NNNNNN) or a git commit hash (anything that doesn't look "
+        'like an SVN revision number).')
+    parser_revert.set_defaults(func=cmd_revert)
+
     args = p.parse_args(argv)
     VERBOSE = args.verbose
     QUIET = args.quiet
 
     # Dispatch to the right subcommand
     args.func(args)