Page MenuHomePhabricator

Add "git llvm revert" subcommand
ClosedPublic

Authored by rupprecht on Mar 26 2019, 12:47 PM.

Details

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)

Diff Detail

Repository
rL LLVM

Event Timeline

rupprecht created this revision.Mar 26 2019, 12:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 26 2019, 12:47 PM

Thank you for the patch!

llvm/utils/git-svn/git-llvm
115 ↗(On Diff #192330)

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

429 ↗(On Diff #192330)

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 updated this revision to Diff 192367.Mar 26 2019, 2:47 PM
rupprecht marked 2 inline comments as done.
  • Use shlex/pipes for better quoting
rupprecht added inline comments.Mar 26 2019, 2:48 PM
llvm/utils/git-svn/git-llvm
429 ↗(On Diff #192330)

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 added inline comments.Mar 26 2019, 3:19 PM
llvm/utils/git-svn/git-llvm
429 ↗(On Diff #192330)

(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__?

Looks great! Thanks.

llvm/utils/git-svn/git-llvm
429 ↗(On Diff #192330)

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

rupprecht updated this revision to Diff 192516.Mar 27 2019, 1:44 PM
  • Extract some changes to git llvm svn-lookup command
  • Add dry_run arg for revert
  • Add docs about the message format

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

llvm/utils/git-svn/git-llvm
410 ↗(On Diff #192516)

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 ↗(On Diff #192516)

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 updated this revision to Diff 192546.Mar 27 2019, 4:58 PM
rupprecht marked 3 inline comments as done.
  • 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 ↗(On Diff #192516)

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.

jlebar accepted this revision.Mar 27 2019, 5:05 PM
This revision is now accepted and ready to land.Mar 27 2019, 5:05 PM
This revision was automatically updated to reflect the committed changes.
jyknight added inline comments.Mar 28 2019, 1:59 PM
llvm/trunk/utils/git-svn/git-llvm
414

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

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