Hello David Hendricks, Paul Menzel, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/21832
to look at the new patch set (#2).
Change subject: fixup! Convert flashrom to git
......................................................................
fixup! Convert flashrom to git
- update the commit-msg hook to the latest one provided by Gerrit.
However, disable the (new) code that would avoid adding Change-IDs
to fixup/squash commits as needed on the staging branch
Change-Id: I2f2d7ae58dcd7d3e55959e18fe664df10bc3cc41
Signed-off-by: Stefan Tauner <stefan.tauner(a)gmx.at>
---
M util/git-hooks/commit-msg
1 file changed, 27 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/32/21832/2
--
To view, visit https://review.coreboot.org/21832
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: staging
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2f2d7ae58dcd7d3e55959e18fe664df10bc3cc41
Gerrit-Change-Number: 21832
Gerrit-PatchSet: 2
Gerrit-Owner: Stefan Tauner <stefan.tauner(a)gmx.at>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Hello David Hendricks, Paul Menzel, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/21829
to look at the new patch set (#2).
Change subject: fixup! Convert flashrom to git
......................................................................
fixup! Convert flashrom to git
Rename getrevision's local_revision function to just revision.
All revisions are local in git and we certainly wont go back to
a non-distributed VCS :)
Change-Id: I6689ac24077b3981b471ed69de7cc3ef79d435b1
Signed-off-by: Stefan Tauner <stefan.tauner(a)gmx.at>
---
M Makefile
M util/getrevision.sh
2 files changed, 12 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/29/21829/2
--
To view, visit https://review.coreboot.org/21829
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: staging
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6689ac24077b3981b471ed69de7cc3ef79d435b1
Gerrit-Change-Number: 21829
Gerrit-PatchSet: 2
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>
Hello David Hendricks, Paul Menzel, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/21828
to look at the new patch set (#2).
Change subject: fixup! Convert flashrom to git
......................................................................
fixup! Convert flashrom to git
Note the non-strict POSIX compatibility in getrevision.sh and a add missing full stop*.* ;)
Change-Id: Ia60186f783067ba084439a8ef701dc8f4c0072f0
Signed-off-by: Stefan Tauner <stefan.tauner(a)gmx.at>
---
M util/getrevision.sh
1 file changed, 2 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/28/21828/2
--
To view, visit https://review.coreboot.org/21828
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: staging
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia60186f783067ba084439a8ef701dc8f4c0072f0
Gerrit-Change-Number: 21828
Gerrit-PatchSet: 2
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>
Stefan Tauner has posted comments on this change. ( https://review.coreboot.org/21837 )
Change subject: fixup! Convert flashrom to git
......................................................................
Patch Set 1:
> Scratch that. -U is pretty old, this fixup is completely unrelated
> to
> the commit it fixes up?
Well, not completely completely. In my initial patch that included the version string and upcache it was also changed to -u and I still think it's a good and tiny change.
I don't have *any* reservations regarding backward compatibility of this script's parameters because I bet no one but the makefile is using it yet and it simply does not make sense to have an upper-case parameter in its current form so there is really no reason to NOT change it IMHO even if it seems "unrelated" or useless. I don't see how a -2 would be warranted apart from expressing anger about the extensive bike shedding ;)
--
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: Stefan Tauner <stefan.tauner(a)gmx.at>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Wed, 04 Oct 2017 01:33:16 +0000
Gerrit-HasComments: No
Stefan Tauner 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]\+\)\)$
> Same paragraph mentions ^ is implicit and unclear what happens
Good news everyone, I am not completely senile after all!
But I have documented these two bits in case I will in the future ;)
--
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: Wed, 04 Oct 2017 01:08:07 +0000
Gerrit-HasComments: Yes
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?
There is BTW: http://www.shellcheck.net/ and it does not report any major problems as of now (but we could use more quoting if anybody feels like it).
--
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:44:19 +0000
Gerrit-HasComments: No
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