[PATCH] D60017: [git] Be more specific when looking for llvm-svn

[James Y Knight via Phabricator via llvm-commits]

jyknight added a comment.

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



================
Comment at: llvm/utils/git-svn/git-llvm:496
+      svn_rev = lookup_llvm_svn_id(args.revision)
+      git_hash = args.revision
 
----------------
rupprecht wrote:
> jyknight wrote:
> > 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.
> 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)'
> ```
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).


================
Comment at: llvm/utils/git-svn/git-llvm:456
+    possible_hashes = git(
+        'log', '--format=%h', '--grep', '^llvm-svn: %d$' % svn_rev,
+        'HEAD~10000...HEAD').split('\n')
----------------
Probably better to retrieve/emit the full hash (that is, from %H), not an abbreviated hash (%h).



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60017