Stefan Tauner 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".
Instead of stable or "random" we might better call it subjectively_perfect or something ;) Staging was meant to iron out the most obvious problems and I would then work on the minor details before merging them to stable. The current process (including the lag of stable) is not as it was envisaged... we will see how the next weeks will work out, then I'll write something down/propose a more strictly ruled process.
> > 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?
Adding the comment was meant to exactly reduce the chance of somebody adding non-POSIX code without himself or a reviewer noticing it.
The point is that we want the script to work on all possible build systems of course... and there we get away with little violations. We just need to keep it in mind.
I like your suggestion of damping down the authority of the comment very much (see next PS).
--
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: Wed, 04 Oct 2017 00:37:04 +0000
Gerrit-HasComments: No
Stefan Tauner has posted comments on this change. ( https://review.coreboot.org/21827 )
Change subject: fixup! Convert flashrom to git
......................................................................
Patch Set 1:
> (1 comment)
>
> Don't see that the hidden behavioural change deserves a fixup. Let
> me know if you have strong feelings about that.
<the usual initial git commit argument blabla>
The commit message tries to give some motivations on why these are good ideas. Less interdependence between (the code in) the makefile, the actual vcs used, and getrevision.sh; and it's less but more readable code too :)
--
To view, visit https://review.coreboot.org/21827
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: staging
Gerrit-MessageType: comment
Gerrit-Change-Id: Iedc9df4c033a70447b8b1b65c83764c769b02c3f
Gerrit-Change-Number: 21827
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: Wed, 04 Oct 2017 00:25:14 +0000
Gerrit-HasComments: No
Stefan Tauner has posted comments on this change. ( https://review.coreboot.org/21825 )
Change subject: fixup! Convert flashrom to git
......................................................................
Patch Set 1:
> I have a strong preference, indeed. But I can't force anybody as
> there
> is no written rule. My general impression is: When somebody makes
> the
> visible width (end minus indentation) of a text block wider than 80
> chars, he doesn't care if somebody will read it. If he makes it
> wider
> than 100 chars, he doesn't want it to be read.
>
> The typesetting rule is to break around 60~70 chars (going back to
> typesetting research done some centuries ago, iirc).
I don't really disagree with you on the readability of shorter lines in general... I write almost everything I publish for others to read in Latex-based PDFs for a reason after all ;)
However, the cited 60-70 cpl are for continuous justified text printed out on paper. For source code where fixed-point fonts are used in very short paragraphs these results are not necessarily true and there are studies that disprove the theory that e.g. 100 cpl is less legible than shorter lines on average when reading from screens as well.
That said, I would not mind a shorter maximum length of comment lines if we agree on some rule (e.g. 75 cpl hard limit) although it would be a bit of a nuisance to comply with that (I know of no editor that provides such an dynamic guide easily). If you want such a rule please post a mail on the mailing list with your proposition so that others can chime in and we have something to refer to in the future if a consensus is reached. Until then I will continue using the space to the maximum column limit of 112.
With the help of git hooks we might check for these things automatically though... if someone wants to hack on that ;)
--
To view, visit https://review.coreboot.org/21825
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: staging
Gerrit-MessageType: comment
Gerrit-Change-Id: Iff4021be49a9fab208b619c555b9f9e81f671ab8
Gerrit-Change-Number: 21825
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: Wed, 04 Oct 2017 00:05:21 +0000
Gerrit-HasComments: No
Nico Huber has posted comments on this change. ( https://review.coreboot.org/21834 )
Change subject: fixup! Convert flashrom to git
......................................................................
Patch Set 1:
(1 comment)
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]\+\)\)$
> Outer parentheses: because that's needed when using the matching expression
Same paragraph mentions ^ is implicit and unclear what happens
if you use it. So better leave it out.
Shorter alternative:
\(\([0-9]\+\.\)\{2,\}x\)$
--
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: Stefan Tauner <stefan.tauner(a)gmx.at>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Tue, 03 Oct 2017 13:08:11 +0000
Gerrit-HasComments: Yes
David Hendricks has submitted this change and it was merged. ( https://review.coreboot.org/21702 )
Change subject: fixup! nicintel_eeprom: Support for I210 emulated EEprom
......................................................................
fixup! nicintel_eeprom: Support for I210 emulated EEprom
A couple of C99-style variable declarations within loops are causing
compilation failures on some systems (gcc 4.9.2-10 on Raspbian). This
moves them to make gcc happy.
Change-Id: Ib7ad5a69244e462f84eae93df9e841716e089b31
Signed-off-by: David Hendricks <david.hendricks(a)gmail.com>
Reviewed-on: https://review.coreboot.org/21702
Reviewed-by: Nico Huber <nico.h(a)gmx.de>
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
---
M nicintel_eeprom.c
1 file changed, 4 insertions(+), 2 deletions(-)
Approvals:
Nico Huber: Looks good to me, approved
build bot (Jenkins): Verified
diff --git a/nicintel_eeprom.c b/nicintel_eeprom.c
index e2fb584..e4a91ef 100644
--- a/nicintel_eeprom.c
+++ b/nicintel_eeprom.c
@@ -211,7 +211,8 @@
pci_mmio_writel(eewr, nicintel_eebar + EEWR);
programmer_delay(5);
- for (int i = 0; i < MAX_ATTEMPTS; i++)
+ int i;
+ for (i = 0; i < MAX_ATTEMPTS; i++)
if (pci_mmio_readl(nicintel_eebar + EEWR) & BIT(EEWR_DONE))
return 0;
return -1;
@@ -419,7 +420,8 @@
flup |= BIT(EE_FLUPD);
pci_mmio_writel(flup, nicintel_eebar + EEC);
- for (int i = 0; i < MAX_ATTEMPTS; i++)
+ int i;
+ for (i = 0; i < MAX_ATTEMPTS; i++)
if (pci_mmio_readl(nicintel_eebar + EEC) & BIT(EE_FLUDONE))
return 0;
--
To view, visit https://review.coreboot.org/21702
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: staging
Gerrit-MessageType: merged
Gerrit-Change-Id: Ib7ad5a69244e462f84eae93df9e841716e089b31
Gerrit-Change-Number: 21702
Gerrit-PatchSet: 3
Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com>
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>
Stefan Tauner 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
Outer parentheses: because that's needed when using the matching expression of expr: http://pubs.opengroup.org/onlinepubs/9699919799/utilities/expr.html#tag_20_…
^ is a good idea, no idea why it's not already there.
Not sure about the last question... that 'x' is a literal one i presume. Works for me... so this would be '\(^\([0-9]\+\.\)\+\([0-9]\+\)\.x\)$'
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
Right. Or to be more precise: this is a wrong intermediate wording from an earlier patch set of David. I should have left his last version as is, sorry.
--
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: Stefan Tauner <stefan.tauner(a)gmx.at>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Mon, 02 Oct 2017 23:59:15 +0000
Gerrit-HasComments: Yes