[PATCH] D60017: [git] Be more specific when looking for llvm-svn
James Y Knight via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed May 22 14:39:36 PDT 2019
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
More information about the llvm-commits
mailing list