[PATCH] D59837: Add "git llvm revert" subcommand

[Justin Lebar via Phabricator via llvm-commits]

jlebar added a comment.

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



================
Comment at: llvm/utils/git-svn/git-llvm:410
 
+def lookup_llvm_svn_id(revision, include_r=True):
+    # Checks if it's already an svn id if it looks like rNNNNNN or NNNNNN,
----------------
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.


================
Comment at: llvm/utils/git-svn/git-llvm:438
+
+    log(lookup_llvm_svn_id(args.revision))
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59837/new/

https://reviews.llvm.org/D59837