code review 55400044: crypto/tls: Exposing the protocol version used during a...

crypto/tls: Exposing the protocol version used during a handshake via the ConnectionState struct.

Fixes issue 7231

[noj, 2014-01-31 05:08:44]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go/

[noj, 2014-01-31 05:10:34]

On 2014/01/31 05:08:44, noj wrote:
> Hello mailto:golang-codereviews@googlegroups.com,
> 
> I'd like you to review this change to
> https://code.google.com/p/go/

This is in response to the following issue:
https://code.google.com/p/go/issues/detail?id=7231

[dfc, 2014-01-31 05:26:03]

On 2014/01/31 05:10:34, noj wrote:
> On 2014/01/31 05:08:44, noj wrote:
> > Hello mailto:golang-codereviews@googlegroups.com,
> > 
> > I'd like you to review this change to
> > https://code.google.com/p/go/
> 
> This is in response to the following issue:
> https://code.google.com/p/go/issues/detail?id=7231

Looks pretty good. Please add a test.

[noj, 2014-01-31 05:59:49]

On 2014/01/31 05:26:03, dfc wrote:
> On 2014/01/31 05:10:34, noj wrote:
> > On 2014/01/31 05:08:44, noj wrote:
> > > Hello mailto:golang-codereviews@googlegroups.com,
> > > 
> > > I'd like you to review this change to
> > > https://code.google.com/p/go/
> > 
> > This is in response to the following issue:
> > https://code.google.com/p/go/issues/detail?id=7231
> 
> Looks pretty good. Please add a test.

Well, this is embarrassing...

So I added a test and ran...
> hg file 55400044 src/pkg/crypto/tls/handshake_server_test.go
> hg change 55400044

But it doesn't seem to be updating this review with the new changes.  Am I doing
something wrong? Sorry, first time using mercurial and coderview.appspot.com

[dfc, 2014-01-31 06:10:54]

hg change just changes the metadata, you can use hg mail NNN to upload a new
change set and sent a PTAL, or hg upload to avoid the notification. 

> On 31 Jan 2014, at 16:59, manoj.dayaram@moovweb.com wrote:
> 
>> On 2014/01/31 05:26:03, dfc wrote:
>> On 2014/01/31 05:10:34, noj wrote:
>> > On 2014/01/31 05:08:44, noj wrote:
>> > > Hello mailto:golang-codereviews@googlegroups.com,
>> > >
>> > > I'd like you to review this change to
>> > > https://code.google.com/p/go/
>> >
>> > This is in response to the following issue:
>> > https://code.google.com/p/go/issues/detail?id=7231
> 
>> Looks pretty good. Please add a test.
> 
> Well, this is embarrassing...
> 
> So I added a test and ran...
>> hg file 55400044 src/pkg/crypto/tls/handshake_server_test.go
>> hg change 55400044
> 
> But it doesn't seem to be updating this review with the new changes.  Am
> I doing something wrong? Sorry, first time using mercurial and
> coderview.appspot.com
> 
> https://codereview.appspot.com/55400044/

[noj, 2014-01-31 06:24:27]

On 2014/01/31 06:10:54, dfc wrote:
> hg change just changes the metadata, you can use hg mail NNN to upload a new
> change set and sent a PTAL, or hg upload to avoid the notification. 
> 
> > On 31 Jan 2014, at 16:59, mailto:manoj.dayaram@moovweb.com wrote:
> > 
> >> On 2014/01/31 05:26:03, dfc wrote:
> >> On 2014/01/31 05:10:34, noj wrote:
> >> > On 2014/01/31 05:08:44, noj wrote:
> >> > > Hello mailto:golang-codereviews@googlegroups.com,
> >> > >
> >> > > I'd like you to review this change to
> >> > > https://code.google.com/p/go/
> >> >
> >> > This is in response to the following issue:
> >> > https://code.google.com/p/go/issues/detail?id=7231
> > 
> >> Looks pretty good. Please add a test.
> > 
> > Well, this is embarrassing...
> > 
> > So I added a test and ran...
> >> hg file 55400044 src/pkg/crypto/tls/handshake_server_test.go
> >> hg change 55400044
> > 
> > But it doesn't seem to be updating this review with the new changes.  Am
> > I doing something wrong? Sorry, first time using mercurial and
> > http://coderview.appspot.com
> > 
> > https://codereview.appspot.com/55400044/

Ah! Thanks!  Sorry for the newbness.
Test has been added ^.^

[agl1, 2014-01-31 15:12:50]

LGTM.

Have you signed the ICA? http://golang.org/doc/contribute.html#copyright

https://codereview.appspot.com/55400044/diff/40001/src/pkg/crypto/tls/common.go
File src/pkg/crypto/tls/common.go (right):

https://codereview.appspot.com/55400044/diff/40001/src/pkg/crypto/tls/common....
src/pkg/crypto/tls/common.go:19: // Secure protocol versions.
s/Secure p/P/

[noj, 2014-02-01 01:43:07]

I just submitted it via email.  I'm assuming we have to wait for a response
before we can submit this changelist?

[dfc, 2014-02-01 01:58:48]

There is no direct feedback from the cla process. A googler will have to look on
their dashboard and propose a change to the author and contributors file. 
Normally Ian, brad or Andrew takes care of this, the latter are in the eu time
zone so will probably action this over the weekend. 

Cheers

Dave



> On 1 Feb 2014, at 12:43, manoj.dayaram@moovweb.com wrote:
> 
> I just submitted it via email.  I'm assuming we have to wait for a
> response before we can submit this changelist?
> 
> https://codereview.appspot.com/55400044/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
"golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.

[noj, 2014-02-01 03:11:34]

Alrighty, thanks for the help! 

On 2014/02/01 01:58:48, dfc wrote:
> There is no direct feedback from the cla process. A googler will have to look
on
> their dashboard and propose a change to the author and contributors file. 
> Normally Ian, brad or Andrew takes care of this, the latter are in the eu time
> zone so will probably action this over the weekend. 
> 
> Cheers
> 
> Dave
> 
> 
> 
> > On 1 Feb 2014, at 12:43, mailto:manoj.dayaram@moovweb.com wrote:
> > 
> > I just submitted it via email.  I'm assuming we have to wait for a
> > response before we can submit this changelist?
> > 
> > https://codereview.appspot.com/55400044/
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email to mailto:golang-codereviews+unsubscribe@googlegroups.com.
> > For more options, visit https://groups.google.com/groups/opt_out.

[agl1, 2014-02-03 15:59:10]

There's no record of the signature yet.

[noj, 2014-02-03 18:08:34]

On 2014/02/03 15:59:10, agl1 wrote:
> There's no record of the signature yet.

Hmmm, I forwarded you the email I sent to the CLA, perhaps I did something
wrong?

[agl1, 2014-02-04 15:49:56]

LGTM

[noj, 2014-02-04 19:49:36]

On 2014/02/04 15:49:56, agl1 wrote:
> LGTM

Cool! Is this something I should submit myself? Or will you submit it?

[agl1, 2014-02-04 19:55:11]

I'll submit once the CLA is processed. Thanks!

[noj, 2014-02-04 20:52:07]

On 2014/02/04 19:55:11, agl1 wrote:
> I'll submit once the CLA is processed. Thanks!

Cool, thanks ^.^

[gobot, 2014-02-05 22:39:56]

R=agl@golang.org (assigned by dave@cheney.net)

[rsc, 2014-03-05 19:39:19]

71700043 is the CLA

[bradfitz, 2014-03-05 19:49:01]

R=close

Already fixed by revision 8cef243ea572 it seems.

Still worth getting the CLA in, to speed future contributions.



On Wed, Mar 5, 2014 at 11:39 AM, <rsc@golang.org> wrote:

> 71700043 is the CLA
>
>
> https://codereview.appspot.com/55400044/
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>

[agl1, 2014-03-05 19:53:19]

On Wed, Mar 5, 2014 at 2:48 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> R=close
>
> Already fixed by revision 8cef243ea572 it seems.

Yea, the CLA risked missing the 1.3 freeze so I implemented it myself
to ensure that it was included.


Cheers

AGL

[bradfitz, 2014-03-05 19:54:43]

On Wed, Mar 5, 2014 at 11:52 AM, Adam Langley <agl@golang.org> wrote:

> On Wed, Mar 5, 2014 at 2:48 PM, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
> > R=close
> >
> > Already fixed by revision 8cef243ea572 it seems.
>
> Yea, the CLA risked missing the 1.3 freeze so I implemented it myself
> to ensure that it was included.
>

That's fine, but FWIW the freeze only applies to new work, not CLs mailed
prior to the freeze.

[rsc, 2014-03-05 20:28:53]

I tried to submit this but there are some merge conflicts. manoj, could you hg
sync and then re-hg mail?

[rsc, 2014-03-05 20:29:47]

sorry, behind on mail. great, glad this got in another way.

[noj, 2014-03-05 22:04:16]

On 2014/03/05 20:29:47, rsc wrote:
> sorry, behind on mail. great, glad this got in another way.

Aww, so it looks like this changelist never made it through. I guess I'll have
to find something else for my first contribution to the Go codebase :p
Glad the change made it in though! Thanks for getting it through into 1.3!