This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by rupprecht on Mar 29 2019, 3:17 PM.

Details

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.
  1. 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).

Event Timeline

rupprecht created this revision.Mar 29 2019, 3:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 29 2019, 3:17 PM
jyknight added inline comments.Apr 8 2019, 3:11 AM
llvm/utils/git-svn/git-llvm
420–421

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

498

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 updated this revision to Diff 195524.Apr 17 2019, 2:18 AM
rupprecht marked 3 inline comments as done.
  • Use %b format to avoid whitespace handling
  • Handle ref names
rupprecht added inline comments.Apr 17 2019, 2:20 AM
llvm/utils/git-svn/git-llvm
498

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)'

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

llvm/utils/git-svn/git-llvm
456

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

498

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 updated this revision to Diff 200832.May 22 2019, 3:52 PM
rupprecht marked 2 inline comments as done.
  • %h->%H
  • Use git rev-parse instead of git log to get the commit

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!

jyknight accepted this revision.May 23 2019, 7:05 AM
This revision is now accepted and ready to land.May 23 2019, 7:05 AM
This revision was automatically updated to reflect the committed changes.