david.hendricks@gmail.com has posted comments on this change. ( https://review.coreboot.org/19206 )
Change subject: Convert flashrom to git ......................................................................
Patch Set 1:
(20 comments)
Makefile, util/getrevision.sh, and util/git-hooks/pre-push still need some work and many of your PS1 comments still need to be addressed, but I tried to handle some of the low-hanging fruit.
https://review.coreboot.org/#/c/19206/1//COMMIT_MSG Commit Message:
PS1, Line 28: With this patch the version string includes the last reachable git tag, : number of commits since said tag in the upstream branches (if any), : the name of said upstream branch, number of commits since that branch : (if any), and the shortened git hash. This seems way over-complicated with no visible use case. It also makes the version confusing and difficult to parse programmatically.
PS1, Line 63: 3 commits since last tag in upstream's staging branch and : 4 local commits on top of that, and some local uncommitted changes too Honestly, who cares? If somebody gets on the mailing list and presents this info, what value does it provide? AFAICT the only interesting part is the sha and "-dirty".
I suppose one could make an argument for the branch name, but as you pointed out there's no guarantee that the branches are synchronized so even that info may just be misleading.
PS1, Line 84: Acked-by: Stefan Tauner stefan.tauner@alumni.tuwien.ac.at
I'd drop this since it's not this revision he acked.
Done. While we're at it, should I add your Signed-off-by? By the time we're done you'll have contributed a significant chunk of the rework.
https://review.coreboot.org/#/c/19206/1/util/getrevision.sh File util/getrevision.sh:
PS1, Line 41: upcache_refs
uninitialized
Done
PS1, Line 44: # We need to update our upstream information sometimes so that we can create detailed version information. : # To that end the code below fetches parts of the upstream repository via https. : # This takes about one second or less under normal circumstances.
Do we break lines only at full stops now? This looks like a single
Agreed, and done. Long lines can help readability in some cases, comments are not one of them.
PS1, Line 50: forced_update
This is not about updating in general but updating the "upcache".
Done
PS1, Line 58: update
This is not about updating in general but updating the "upcache".
Done
Line 66: upstream repositories as git remote to rely on that information.
Is here supposed to start a new paragraph? Please separate them
Done
I avoided breaking commands to make copy+paste easier for the user and put the URL on its own line to try and avoid having it expand wider than the other lines.
PS1, Line 69: >&2 not needed?
PS1, Line 73: >&2 not needed?
PS1, Line 77: >&2 not needed?
PS1, Line 86: "$1"
This is an empty string if $1 is not set. It still works, but git
I think the way this is used we'll always have non-empty $1, though it's not very intuitive since we need to trace the argument all the way back to main().
Maybe we should add something to abort and indicate a bug in the script if the "$1" is empty? Same for other functions, too.
PS1, Line 218: previse
precise?
Done, and reformatted as with the above comments.
PS1, Line 224: -n "$up_remote"
Just that there is a upstream remote registered doesn't imply
I'm wondering if this is all too elaborate. I'd rather keep it simple and stick with sha1 + dirty, and add upstream URL while we're at it. Those are easy to obtain and I don't think I've ever cared to know about "commits since the last tag in an unrelated upstream branch". All this extra stuff is interesting git-fu, but all it accomplishes is to make the version string confusing and impossible to parse programmatically.
At the very least, I think we should split most of this hunk into a separate patch for now. LMK what you think.
https://review.coreboot.org/#/c/19206/1/util/git-hooks/pre-push File util/git-hooks/pre-push:
PS1, Line 1: #!/bin/sh
But it looks like a bash script to me.
Since we probably want to avoid bashisms in general for git hooks, I got rid of the array by using a simple space-delimited string instead.
PS1, Line 25: ( stable staging ) changed to "stable staging"
Line 41: else
Why put an else here if we exited anyway?
Good question. Fixed.
PS1, Line 58: "${precious_branches[@]}" updated to iterate thru strings in $precious_branches
Line 60: nonreachable=$(git rev-list $remote_sha ^$local_sha)
Maybe add `| head -1` the list could be long.
Done
PS1, Line 63: pusing
*pushing
Done