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

Tue Mar 26 14:50:03 PDT 2019

rupprecht added inline comments.

================
Comment at: llvm/utils/git-svn/git-llvm:429
+    else:
+      log_verbose('Searching by git hash for commit ' + args.revision)
+      # Ignore errors so we can print a friendlier message. Also, use a format
----------------
jlebar wrote:
> 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?
> 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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59837