October 2017 - flashrom-gerrit

Change in flashrom[staging]: fixup! Convert flashrom to git
by Nico Huber (Code Review) 02 Oct '17

02 Oct '17

Nico Huber has posted comments on this change. ( https://review.coreboot.org/21828 ) Change subject: fixup! Convert flashrom to git ...................................................................... Patch Set 1: (1 comment) > ... Hence I want to have such rather benign things in > it if possible. Yeah, that's ok, I guess. We are already making exceptions for the Git conversion commit anyway. Generally, I'd prefer only changes that fix semantical bugs in the code to be fixups, i.e. what you really need to make it "stable". > > The comment in question documents intent to make this clear for > reviewers as contributors (while the sh shebang should be a clear > hint I think the comment is still warranted due to the local > restriction and its outspokenness). There was no strict review for > this I guess but a) I always intended it to be that way (but of > course there might be bugs... e.g., I wasn't aware local is not > POSIX before you brought it up), b) you had a look for that > obviously in the first review. Not sure if that was me. At least I don't recall really checking the whole file. Maybe just rephrase (see inline comment), I'd like to avoid compatibility claims if we are not 100% sure. (It's my code-comment allergy. I've seen too many misleading comments over the years. So I prefer to write code that explains itself.) Also, who'd maintain the comment if anybody puts another non- POSIX construct in this file? https://review.coreboot.org/#/c/21828/1/util/getrevision.sh File util/getrevision.sh: https://review.coreboot.org/#/c/21828/1/util/getrevision.sh@2 PS1, Line 2: # NB: POSIX-compatible apart from the usage of 'local' to define variables within the scope of functions. How about "Supposed to be POSIX compatible but at least the usage of 'local' is off."? -- To view, visit https://review.coreboot.org/21828 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: Ia60186f783067ba084439a8ef701dc8f4c0072f0 Gerrit-Change-Number: 21828 Gerrit-PatchSet: 1 Gerrit-Owner: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 02 Oct 2017 13:47:35 +0000 Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: fixup! Convert flashrom to git
by Nico Huber (Code Review) 02 Oct '17

02 Oct '17

Nico Huber has posted comments on this change. ( https://review.coreboot.org/21837 ) Change subject: fixup! Convert flashrom to git ...................................................................... Patch Set 1: Code-Review-2 Scratch that. -U is pretty old, this fixup is completely unrelated to the commit it fixes up? -- To view, visit https://review.coreboot.org/21837 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: I2f84d9db2ec3247f313573c5fcd8b9b758ab9d96 Gerrit-Change-Number: 21837 Gerrit-PatchSet: 1 Gerrit-Owner: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 02 Oct 2017 13:24:35 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: fixup! Convert flashrom to git
by Nico Huber (Code Review) 02 Oct '17

02 Oct '17

Nico Huber has posted comments on this change. ( https://review.coreboot.org/21837 ) Change subject: fixup! Convert flashrom to git ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/21837 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: I2f84d9db2ec3247f313573c5fcd8b9b758ab9d96 Gerrit-Change-Number: 21837 Gerrit-PatchSet: 1 Gerrit-Owner: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 02 Oct 2017 13:22:58 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: fixup! Convert flashrom to git
by Nico Huber (Code Review) 02 Oct '17

02 Oct '17

Nico Huber has posted comments on this change. ( https://review.coreboot.org/21836 ) Change subject: fixup! Convert flashrom to git ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/21836 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: Iac3bf11fbd55cfa034ef8af04ef90fe57182ee2b Gerrit-Change-Number: 21836 Gerrit-PatchSet: 1 Gerrit-Owner: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 02 Oct 2017 13:21:46 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: fixup! Convert flashrom to git
by Nico Huber (Code Review) 02 Oct '17

02 Oct '17

Nico Huber has posted comments on this change. ( https://review.coreboot.org/21834 ) Change subject: fixup! Convert flashrom to git ...................................................................... Patch Set 1: (2 comments) https://review.coreboot.org/#/c/21834/1/util/git-hooks/pre-push File util/git-hooks/pre-push: https://review.coreboot.org/#/c/21834/1/util/git-hooks/pre-push@34 PS1, Line 34: $\([0-9]\+\.$\+$[0-9]\+$\)$ Why the outer-most parentheses? Why not start with ^ ? and didn't we agree that stable branches should be named <major>.<minor>.x ? Might have been an idle wish, though. https://review.coreboot.org/#/c/21834/1/util/git-hooks/pre-push@51 PS1, Line 51: echo "Neither \"Signed-off-by\" nor \"Acked-by\" were found in commit $local_sha in " \ Both Signed-off-by and Acked-by have to be present, don't they? this phrasing doesn't reflect that. The old did, nothing to fix? -- To view, visit https://review.coreboot.org/21834 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: I1d4b4a7ef2673cabee980ec4a7d7d5fbebdcaed1 Gerrit-Change-Number: 21834 Gerrit-PatchSet: 1 Gerrit-Owner: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 02 Oct 2017 13:21:18 +0000 Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: fixup! Convert flashrom to git
by Nico Huber (Code Review) 02 Oct '17

02 Oct '17

Nico Huber has posted comments on this change. ( https://review.coreboot.org/21833 ) Change subject: fixup! Convert flashrom to git ...................................................................... Patch Set 1: (1 comment) Not a fixup. What's the use case for a fixup? somebody pushing commits based on something between v0.9.9 and v1.0-rc1? Not likely if we write the whole history at once :-P https://review.coreboot.org/#/c/21833/1/util/git-hooks/commit-msg File util/git-hooks/commit-msg: https://review.coreboot.org/#/c/21833/1/util/git-hooks/commit-msg@198 PS1, Line 198: [ ] How about [[:space:]] instead? Would be easier to read in the average text editor. -- To view, visit https://review.coreboot.org/21833 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: Ia36147e673cceb6d175884b40d4bdd00015b96dc Gerrit-Change-Number: 21833 Gerrit-PatchSet: 1 Gerrit-Owner: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 02 Oct 2017 12:51:59 +0000 Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: fixup! Convert flashrom to git
by Stefan Tauner (Code Review) 02 Oct '17

02 Oct '17

Stefan Tauner has posted comments on this change. ( https://review.coreboot.org/21830 ) Change subject: fixup! Convert flashrom to git ...................................................................... Patch Set 1: (2 comments) https://review.coreboot.org/#/c/21830/1/Makefile File Makefile: https://review.coreboot.org/#/c/21830/1/Makefile@a544 PS1, Line 544: > Why drop the empty line? anyway the whole block seems to be a dead Dunno why I decided to put it in here TBH. And yes, the offline block will be gone anyway and then it looks fine (but someone told me to slice my changes up into a myriad of pieces ;). I will rearrange that to be included in the patch that removes the offline block when I add the sign-offs. https://review.coreboot.org/#/c/21830/1/Makefile@529 PS1, Line 529: required by the build process when packaging flashrom > This is not true, AFAIU (the old text wasn't very accurate either). Yep, that's better. Thanks. -- To view, visit https://review.coreboot.org/21830 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: I0c66f2508cc754cf9219211a06d6f305a32c422d Gerrit-Change-Number: 21830 Gerrit-PatchSet: 1 Gerrit-Owner: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 02 Oct 2017 12:50:59 +0000 Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: fixup! Convert flashrom to git
by Nico Huber (Code Review) 02 Oct '17

02 Oct '17

Nico Huber has posted comments on this change. ( https://review.coreboot.org/21830 ) Change subject: fixup! Convert flashrom to git ...................................................................... Patch Set 1: (2 comments) https://review.coreboot.org/#/c/21830/1/Makefile File Makefile: https://review.coreboot.org/#/c/21830/1/Makefile@a544 PS1, Line 544: Why drop the empty line? anyway the whole block seems to be a dead artifact. getrevsion.sh never returns "offline", does it? https://review.coreboot.org/#/c/21830/1/Makefile@529 PS1, Line 529: required by the build process when packaging flashrom This is not true, AFAIU (the old text wasn't very accurate either). It is not required "by the build process when packaging flashrom" but "to build a packaged flashrom"? -- To view, visit https://review.coreboot.org/21830 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: I0c66f2508cc754cf9219211a06d6f305a32c422d Gerrit-Change-Number: 21830 Gerrit-PatchSet: 1 Gerrit-Owner: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 02 Oct 2017 12:38:05 +0000 Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: fixup! Convert flashrom to git
by Stefan Tauner (Code Review) 02 Oct '17

02 Oct '17

Stefan Tauner has posted comments on this change. ( https://review.coreboot.org/21828 ) Change subject: fixup! Convert flashrom to git ...................................................................... Patch Set 1: > Is there any automatic tool that checks for compliance? Or did > anybody > really check every line? How is adding this comment a fixup? > > I agree to the full-stop, btw. Line could be broken too, though. > > Is there any automatic tool that checks for compliance? Or did > anybody > really check every line? How is adding this comment a fixup? > > I agree to the full-stop, btw. Line could be broken too, though. This aspect was brought up during the initial review and so for me it naturally belongs to that change. I think we need to write down what fixup patches in the staging branch are supposed to be at some time... :) Independent from the general issue with the staging process I really want to get the git commit "complete" so we have one definitive starting point for all commits to come. Unfortunately this was not possible with staging itself but it should definitely be for any stable release branch (I do intend to have a 0.9.9.1 with the most important fixes to ease getting them in LTS distributions). Hence I want to have such rather benign things in it if possible. The comment in question documents intent to make this clear for reviewers as contributors (while the sh shebang should be a clear hint I think the comment is still warranted due to the local restriction and its outspokenness). There was no strict review for this I guess but a) I always intended it to be that way (but of course there might be bugs... e.g., I wasn't aware local is not POSIX before you brought it up), b) you had a look for that obviously in the first review. -- To view, visit https://review.coreboot.org/21828 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: Ia60186f783067ba084439a8ef701dc8f4c0072f0 Gerrit-Change-Number: 21828 Gerrit-PatchSet: 1 Gerrit-Owner: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 02 Oct 2017 12:36:21 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: fixup! Convert flashrom to git
by Stefan Tauner (Code Review) 02 Oct '17

02 Oct '17

Stefan Tauner has posted comments on this change. ( https://review.coreboot.org/21829 ) Change subject: fixup! Convert flashrom to git ...................................................................... Patch Set 1: (1 comment) https://review.coreboot.org/#/c/21829/1/util/getrevision.sh File util/getrevision.sh: https://review.coreboot.org/#/c/21829/1/util/getrevision.sh@a162 PS1, Line 162: > Why drop that information? I can't see any change in behaviour? That's a mistake, thanks. In my patches the line was even longer due to the elaborate version string... I did not read carefully and apparently removed too much here. -- To view, visit https://review.coreboot.org/21829 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: I6689ac24077b3981b471ed69de7cc3ef79d435b1 Gerrit-Change-Number: 21829 Gerrit-PatchSet: 1 Gerrit-Owner: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 02 Oct 2017 12:26:21 +0000 Gerrit-HasComments: Yes

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit October 2017