Nico Huber has posted comments on this change. ( https://review.coreboot.org/19206 )
Change subject: Convert flashrom to git ......................................................................
Patch Set 4:
(10 comments)
Looks pretty good overall, except for the complex getrevision.sh changes. Splitting it out into a follow-up would get the other changes in (with a minor update, see inline comments), I suppose.
https://review.coreboot.org/#/c/19206/4/Makefile File Makefile:
https://review.coreboot.org/#/c/19206/4/Makefile@a1374 PS4, Line 1374: Why didn't I bikeshed this for being totally unrelated?
https://review.coreboot.org/#/c/19206/4/Makefile@1396 PS4, Line 1396: @tar -cz --format=ustar -f $(EXPORTDIR)/flashrom-$(RELEASENAME).tar.gz -C $(EXPORTDIR)/ \ : $(TAROPTIONS) flashrom-$(RELEASENAME)/ : # Delete the exported directory again because it is most likely what's expected by the user. : @rm -rf $(EXPORTDIR)/flashrom-$(RELEASENAME) Missing quotes around everything with $(RELEASENAME).
https://review.coreboot.org/#/c/19206/1/util/getrevision.sh File util/getrevision.sh:
https://review.coreboot.org/#/c/19206/1/util/getrevision.sh@66 PS1, Line 66: "to the commit history from an upstream repository. If no git remote"\
Done
Looks good, but did you test it? Using echo this way there shouldn't be any line breaks in the output at all.
https://review.coreboot.org/#/c/19206/1/util/getrevision.sh@69 PS1, Line 69:
not needed?
Why not? The message should draw the user's attention and isn't usual program output (i.e. make progress).
https://review.coreboot.org/#/c/19206/1/util/getrevision.sh@224 PS1, Line 224: urn
I'm wondering if this is all too elaborate. I'd rather keep it simple and s
Sounds good, I was all for splitting from the beginning ;) We could just update getrevision.sh to use git for a start. Add the upcache + complex version string (we should start a discussion on the ML probably) later.
https://review.coreboot.org/#/c/19206/1/util/getrevision.sh@224 PS1, Line 224: return : fi : done : } : : revision() { : local sha=$(git_last_commit "$1" 2>/dev/null) : #
What we'd need here instead is
What I meant here is roughly: In the `-n "$up_remote"` case, we only construct $up_refs based on registered remotes and $upstream_branches. If the user just registered the upstream remote without ever fetching it, $up_refs will contain refs unknown to git.
What we need to do is kind of force_update_upcache but for the detected already present remotes. We could spare us all that hassle and some of the already written code if we'd always use an upcache.
https://review.coreboot.org/#/c/19206/1/util/getrevision.sh@257 PS1, Line 257: update_upcache || { echo "offline" ; return ; } : up_refs=$upcache_refs : fi : : # Find nearest commit contained in this branch that is also in any of the up_refs, i.e. the branch point : # of the current branch. This might be the latest commit if it's in any of the upstream branches. : loca
TBH, I'm not sure what to do about that here :-/
Don't worry. It's only a cosmetic issue. And I don't remember it accurately.
https://review.coreboot.org/#/c/19206/1/util/getrevision.sh@269 PS1, Line 269:
Yep, now it is!
Then we don't need the `if`.
https://review.coreboot.org/#/c/19206/4/util/getrevision.sh File util/getrevision.sh:
https://review.coreboot.org/#/c/19206/4/util/getrevision.sh@72 PS4, Line 72: "$upstream_url"\ Maybe indent it a little.
https://review.coreboot.org/#/c/19206/1/util/git-hooks/pre-push File util/git-hooks/pre-push:
https://review.coreboot.org/#/c/19206/1/util/git-hooks/pre-push@53 PS1, Line 53: "$commit" ]; then
If we're going that route, "Neither Signoff nor Ack were found..." would so
Sounds better but is not what I meant: Both have to be present.