Nico Huber has posted comments on this change. ( https://review.coreboot.org/19206 )
Change subject: Convert flashrom to git ......................................................................
Patch Set 6:
(8 comments)
Only some nits and simple things it getrevision.sh.
https://review.coreboot.org/#/c/19206/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/19206/6//COMMIT_MSG@13 PS6, Line 13: - Remove djgpp-dos target (it is not different to other x-compilations). I was just wondering, why I didn't nag earlier, you can keep the change in or remove this line...
https://review.coreboot.org/#/c/19206/6//COMMIT_MSG@15 PS6, Line 15: anything else from the code dump when we export the respective variable. Nit, line is too long.
https://review.coreboot.org/#/c/19206/6//COMMIT_MSG@26 PS6, Line 26: f5dd7ce1 This sha will probably get lost. Linking to the mailing list archive might be better.
https://review.coreboot.org/#/c/19206/6//COMMIT_MSG@28 PS6, Line 28: pushing to the review server. Maybe also mention, that you split out the complex version string.
https://review.coreboot.org/#/c/19206/6/util/getrevision.sh File util/getrevision.sh:
https://review.coreboot.org/#/c/19206/6/util/getrevision.sh@140 PS6, Line 140: local svn_base=$(git log --grep=git-svn-id -1 --format='%h') : if [ "$svn_base" != "" ] ; then : local diff_to_svn=$(git rev-list --count ${svn_base}..${r}) : if [ "$diff_to_svn" -gt 0 ] ; then : r="$r-$diff_to_svn" : fi : fi Looks like we don't have git-svn-id any more. A simple `git describe ${r}` might do? We already have tags in git, for me it shows something like:
v0.9.9-22-gfb2996c1
https://review.coreboot.org/#/c/19206/6/util/getrevision.sh@168 PS6, Line 168: sed 's/.*@/r/;s/[[:blank:]].*//') Same here, maybe just drop the --upstream command?
https://review.coreboot.org/#/c/19206/6/util/git-hooks/pre-push File util/git-hooks/pre-push:
https://review.coreboot.org/#/c/19206/6/util/git-hooks/pre-push@46 PS6, Line 46: else : range="$remote_sha..$local_sha" Nit, actually I don't like the `else` here, it's not directly related to the `if` (i.e. could just move below the `fi`).
Now I also wonder why we need this variable at all (only used in one place).
https://review.coreboot.org/#/c/19206/6/util/git-hooks/pre-push@55 PS6, Line 55: " or "Acked-by" lines, not pushing." >&2 Sounds good.
-
Nico Huber (Code Review)