Re: [coreboot] [coreboot-gerrit] New patch to review for coreboot: 7d39656 lenovo/x60/romstage.c: Collect timestamps in romstage

[Peter Stuge]

ron minnich wrote:
> > commit 7d396566be210dfd5f86c8d9038ae0fdacefa02f
> > Author: Paul Menzel 
> > Date:   Tue Jul 2 09:54:17 2013 +0200
> > lenovo/x60/romstage.c: Collect timestamps in romstage
> >
> > Collect early timestamps in Lenovo X60’s romstage.
>
> This is fine.

Not really, those two lines are completely redundant.
> > Thanks to Nico Huber’s work setting this up for the ICH7 and implementing
> > it for the T60, all what was needed to do, was to do the equivalent
> > changes for the X60 as for the T60 in commit 44c392f8 [1].
> >
> >     lenovo/t60: Collect timestamps in romstage
> >
> > [1] http://review.coreboot.org/3499
>
> remove all this text. It's not needed.

The text is not needed, the link to gerrit is redundant, but the
commit reference is nice to have. The commit message probably
fits in about three lines.

//Peter

[coreboot-gerrit] New patch to review for coreboot: 7d39656 lenovo/x60/romstage.c: Collect timestamps in romstage

[ron minnich]

The message is much better, though. The reference to
review.coreboot.org is a problem; it's a reference in a git message to
something outside the repo. What if in 2 years gerrit is replaced by
something else? We've got another bad URL, that's what. So let's stop
dropping time bombs into commit messages. It's just a headache in the
making.

So, best not to reference another commit at all, since the change is
trivial anyway, but if you must, use the hash. No need to footnote,
just put it inline, as every other project I'm involved in that uses
git seems to do. Then we've got a hash in a log message, and it's all
nicely self-contained.

But, again, no real need to reference another commit. This is a trivial change.

If we further remove the instructions for using timestamps and dumping
them, which do not belong in the log, but on the wiki, we can reduce
to the 3 lines Peter says it can be done in :-)

lenovo/x60/romstage.c: Collect timestamps in romstage

Collect early timestamps in Lenovo X60’s romstage.

Although I only see the need for one line, myself.

thanks

ron

On Thu, Jul 4, 2013 at 2:07 AM, Peter Stuge  wrote:
> ron minnich wrote:
> > > commit 7d396566be210dfd5f86c8d9038ae0fdacefa02f
> > > Author: Paul Menzel 
> > > Date:   Tue Jul 2 09:54:17 2013 +0200
> > > lenovo/x60/romstage.c: Collect timestamps in romstage
> > >
> > > Collect early timestamps in Lenovo X60’s romstage.
> >
> > This is fine.
>
> Not really, those two lines are completely redundant.
> > > Thanks to Nico Huber’s work setting this up for the ICH7 and implementing
> > > it for the T60, all what was needed to do, was to do the equivalent
> > > changes for the X60 as for the T60 in commit 44c392f8 [1].
> > >
> > >     lenovo/t60: Collect timestamps in romstage
> > >
> > > [1] http://review.coreboot.org/3499
> >
> > remove all this text. It's not needed.
>
> The text is not needed, the link to gerrit is redundant, but the
> commit reference is nice to have. The commit message probably
> fits in about three lines.
>
> //Peter