[git] Be more specific when looking for llvm-svn

[rupprecht]

Summary

A commit may, for some reason, have llvm-svn: in it multiple times. It may even take up the whole line and look identical to what gets added automatically when svn commits land in github.

To workaround this, make changes to both lookups:

1. When doing the git -> svn lookup, make sure to go through the whole message, and: a) Only look for llvm-svn starting at the beginning of the line (excluding the whitespace that git log adds). b) Take the last one (at the end of the commit message), if there are multiple matches.

2. When doing the svn -> git lookup, look through a sizeable but still reasonably small number of git commits (10k, about 4-5 months right now), and: a) Only consider commits with the '^llvm-svn: NNNNNN' we expect, and b) Only consider those that also follow the same git -> svn matching above. (Error if it's not exactly one commit).

[jyknight]

llvm/utils/git-svn/git-llvm

417–418

If you use git log -1 --format=%b then you will get just the raw log message, without extraneous whitespace.

496

The argument might be a git ref name (e.g. HEAD~1), so we shouldn't pass the user's specification directly into the output.

I think lookup_llvm_svn_id can return both the git hash and svn id, since it's already looked up both.

[rupprecht]

• Use %b format to avoid whitespace handling
• Handle ref names

[rupprecht]

llvm/utils/git-svn/git-llvm

496

Good point, I hadn't considered git refs.

Although it's a bit simpler to just run an extra "git log -1 --format=%h" than it is to make lookup_llvm_svn_id run w/ "--format=%h%b" and have to parse both things, so I didn't quite make the change you suggested.

With the latest patch, it seems to work:

$ git llvm revert -n HEAD~6
Would have run the following commands, if this weren't a dry run:
1) git revert --no-commit b9b35fd12d4
2) git commit -m 'Revert Fixed error message printing in write_cmake_config.py' -m 'This reverts r358557 (git commit b9b35fd12d4)'

[rupprecht]

ping

[jyknight]

Sorry for dropping this, a couple more comments, then I think it's good.

llvm/utils/git-svn/git-llvm

454

Probably better to retrieve/emit the full hash (that is, from %H), not an abbreviated hash (%h).

496

I think the better command to parse a rev specifier is 'git rev-parse'. According to the manpage, this seems correct:
git_hash = git('git', 'rev-parse', '--verify', args.revision+'^{commit}')

Also probably best to do that command first, then pass the resulting git_hash from that into lookup_llvm_svn_id, just to make sure the user-input parsing is only done once (and thus consistently).

[rupprecht]

• %h->%H
• Use git rev-parse instead of git log to get the commit

[rupprecht]

In D60017#1512747, @jyknight wrote:

Sorry for dropping this, a couple more comments, then I think it's good.

No worries, I also dropped it in between vacation and build/release cop things... thanks for all the comments so far!

llvm/utils/git-svn/git-llvm

for r in revs:
         clean_svn(svn_root)
         svn_push_one_rev(svn_root, r, dry_run)
 
 
 def lookup_llvm_svn_id(git_commit_hash):
     commit_msg = git('log', '-1', git_commit_hash, ignore_errors=True)
     if len(commit_msg) == 0:
       die("Can't find git commit " + git_commit_hash)
-    svn_match = re.search('llvm-svn: (\d{5,7})$', commit_msg)
+    # If a commit has multiple "llvm-svn:" lines (e.g. if the commit is
+    # reverting/quoting a previous commit), choose the last one, which should
+    # be the authoritative one.
+    # Also, we need to check for leading/trailing whitespace to deal with
+    # git log formatting.
+    svn_match_iter = re.finditer('^\s*llvm-svn: (\d{5,7})\s*$', commit_msg, re.MULTILINE)
+    svn_match = None
+    for m in svn_match_iter:
+      svn_match = m.group(1)
     if svn_match:
-      return int(svn_match.group(1))
+      return int(svn_match)
     die("Can't find svn revision in git commit " + git_commit_hash)
 
 
 def cmd_svn_lookup(args):
     '''Find the SVN revision id for a given git commit hash.
 
     This is identified by 'llvm-svn: NNNNNN' in the git commit message.'''
     # 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)
 
     log('r' + str(lookup_llvm_svn_id(args.git_commit_hash)))
 
 
+def git_hash_by_svn_rev(svn_rev):
+    '''Find the git hash for a given svn revision.
+
+    This check is paranoid: 'llvm-svn: NNNNNN' could exist on its own line
+    somewhere else in the commit message. Look in the full log message to see
+    if it's actually on the last line.
+
+    Since this check is expensive (we're searching every single commit), limit
+    to the past 10k commits (about 5 months).
+    '''
+    possible_hashes = git(
+        'log', '--format=%h', '--grep', '^llvm-svn: %d$' % svn_rev,
+        'HEAD~10000...HEAD').split('\n')
+    matching_hashes = [h for h in possible_hashes
+                       if lookup_llvm_svn_id(h) == svn_rev]
+    if len(matching_hashes) > 1:
+      die("svn revision r%d has ambiguous commits: %s" % (
+          svn_rev, ', '.join(matching_hashes)))
+    elif len(matching_hashes) < 1:
+      die("svn revision r%d matches no commits" % svn_rev)
+    return matching_hashes[0]
+
 def cmd_revert(args):
     '''Revert a commit by either SVN id (rNNNNNN) or git hash. This also
     populates the git commit message with both the SVN revision and git hash of
     the change being reverted.'''
 
     # 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)
 
     # If the revision looks like rNNNNNN, use that. Otherwise, look for it in
     # the git commit.
     svn_match = re.match('^r(\d{5,7})$', args.revision)
     if svn_match:
-      svn_rev = svn_match.group(1)
+      # If the revision looks like rNNNNNN, use that as the svn revision, and
+      # grep through git commits to find which one corresponds to that svn
+      # revision.
+      svn_rev = int(svn_match.group(1))
+      git_hash = git_hash_by_svn_rev(svn_rev)
     else:
-      svn_rev = str(lookup_llvm_svn_id(args.revision))
+      # Otherwise, this looks like a git hash, so we just need to grab the svn
+      # revision from the end of the commit message.
+      svn_rev = lookup_llvm_svn_id(args.revision)
+      git_hash = args.revision
 
-    oneline = git('log', '--all',  '-1', '--format=%H %s', '--grep',
-                  'llvm-svn: ' + svn_rev)
-    if len(oneline) == 0:
-      die("Can't find svn revision r" + svn_rev)
-    (git_hash, msg) = oneline.split(' ', 1)
+    msg = git('log', '-1', '--format=%s', git_hash)
 
-    log_verbose('Ready to revert r%s/%s: "%s"' % (svn_rev, git_hash, msg))
+    log_verbose('Ready to revert r%d (%s): "%s"' % (svn_rev, git_hash, msg))
 
     revert_args = ['revert', '--no-commit', git_hash]
     # TODO: Running --edit doesn't seem to work, with errors that stdin is not
     # a tty.
     commit_args = [
         'commit', '-m', 'Revert ' + msg,
-        '-m', 'This reverts r%s (git commit %s)' % (svn_rev, git_hash)]
+        '-m', 'This reverts r%d (git commit %s)' % (svn_rev, git_hash)]
     if args.dry_run:
       log("Would have run the following commands, if this weren't a dry run:\n"
           '1) git %s\n2) git %s' % (
               ' '.join(quote(arg) for arg in revert_args),
               ' '.join(quote(arg) for arg in commit_args)))
       return
 
     git(*revert_args)
     commit_log = git(*commit_args)
 
-    log('Created revert of r%s: %s' % (svn_rev, commit_log))
+    log('Created revert of r%d: %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:]