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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added a comment.

Looks great! Thanks.



================
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:
> rupprecht wrote:
> > 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.
> > 
> > (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__`?
`git llvm svn-lookup`? It would be great to have it, even if `git llvm revert 123456` works as-is


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59837