Nico Huber has posted comments on this change. ( https://review.coreboot.org/19206 )
Change subject: Convert flashrom to git ......................................................................
Patch Set 1:
(5 comments)
Answered some inline comments. Though, I really think we should move any further discussion to changes based on the current staging branch. We'd have to sync staging with the solution for stable anyway and reviewing/discussing will be much easier on simple changes than a everything-at-once commit.
Stefan, please chunk a diff of your current version against staging into smaller commits and push them for review to staging.
https://review.coreboot.org/#/c/19206/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/19206/1//COMMIT_MSG@28 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.
Apart from the additional complexity of the get_revision script I don't really see any downsides.
Yeah, it's doable ofc. But you already proved that it's pretty hard to put it together into a sound script. Just come up with something that makes it through a review if you want to spend more time on it.
https://review.coreboot.org/#/c/19206/1/Makefile File Makefile:
https://review.coreboot.org/#/c/19206/1/Makefile@1377 PS1, Line 1377: x;n;n;s/^set$$//;t;x;p
One question: why the '{}' around 'b'?
Doesn't seem to be needed. I wasn't sure if something like //p;p means //{p;};p or //{p;p;} (semantics of ; are not really defined, but POSIX implicitly specifies the former). Though I'd argue that with the braces you can see more clearly that the ;x;p at the end is separate from the address. Wouldn't be necessary if you write the program in multiple lines, e.g.:
x n n /^set$/b x p
With that written down, how about 'x;n;n;{/^set$$/b;};x;p;'? ;)
I've just learned that there should be a semicolon before the closing brace:
The <right-brace> shall be preceded by a <newline> or <semicolon> (before any optional <blank> characters preceding the <right-brace>).
GNU sed doesn't care...
https://review.coreboot.org/#/c/19206/1/util/getrevision.sh File util/getrevision.sh:
https://review.coreboot.org/#/c/19206/1/util/getrevision.sh@44 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.
No, we break at the latest at a 112 character limit because that's what was
I didn't take part in that discussion, can look it up, though, if necessary. I'd argue that the 112 column limit makes much more sense for code (that usually is indented and has a shorter effec- tive line length) than it does for running text that (maybe) some- body is supposed to read (it's really much harder to read with increasing line length).
https://review.coreboot.org/#/c/19206/1/util/getrevision.sh@269 PS1, Line 269: [ -n "$upstream_branch" ]
No, there might be upstream branches that are neither stable nor staging (e
I was talking about the code flow: `upstream_branch` is only set together with `cnt_tag2upstream_branch` above. So the former can't be unset if the latter is set.
https://review.coreboot.org/#/c/19206/1/util/git-hooks/install.sh File util/git-hooks/install.sh:
https://review.coreboot.org/#/c/19206/1/util/git-hooks/install.sh@17 PS1, Line 17: ../../
No, IIRC, the idea is to make these relative links because else they get in
Yeah, and IIRC, we didn't change that. `git rev-parse --show-cdup` is supposed to output a relative path.
-
Nico Huber (Code Review)