code review 5339041: httputil: move dump and chunking functions out of http

httputil: move dump and chunking functions out of http

This moves DumpRequest, DumpResponse, NewChunkedReader,
and NewChunkedWriter out of http, as part of the continued
http diet plan.

Also, adds DumpRequestOut (for dumping outbound requests),
since DumpRequest's ambiguity (the "wire representation" in
what direction?) was often a source of confusion and bug
reports.

[bradfitz, 2011-11-04 00:15:24]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[rsc, 2011-11-04 00:26:04]

I'm confused.  How can the outgoing wire representation
be different from the incoming wire representation?
That's not a very good wire.

Russ

[bradfitz, 2011-11-04 00:28:23]

On Thu, Nov 3, 2011 at 5:26 PM, Russ Cox <rsc@golang.org> wrote:

> I'm confused.  How can the outgoing wire representation
> be different from the incoming wire representation?
> That's not a very good wire.


arrggh.  I hate these Dump functions so much.

You may not remember
http://code.google.com/p/go/source/detail?r=8d5b60fed6b9 , but I do.

[bradfitz, 2011-11-04 00:29:56]

The difference is the wire as we get it (as a Server) vs the wire as we
send it (for Transport, adding our User-Agent and HTTP/1.1, etc)

Also, I need to update the docs on Request so say it's used for two
purposes.  The current docs say that Request is only for incoming stuff.


On Thu, Nov 3, 2011 at 5:28 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> On Thu, Nov 3, 2011 at 5:26 PM, Russ Cox <rsc@golang.org> wrote:
>
>> I'm confused.  How can the outgoing wire representation
>> be different from the incoming wire representation?
>> That's not a very good wire.
>
>
> arrggh.  I hate these Dump functions so much.
>
> You may not remember
> http://code.google.com/p/go/source/detail?r=8d5b60fed6b9 , but I do.
>
>
>

[rsc, 2011-11-04 00:30:00]

Can we delete them?

[bradfitz, 2011-11-04 00:31:30]

Well, I hate them less now, now that we have both halves, they're
documented, and they're out of http.

People obviously use them if they're filing bugs on them.  I've used them
too when debugging stuff, so they're _occasionally_ useful.


On Thu, Nov 3, 2011 at 5:29 PM, Russ Cox <rsc@golang.org> wrote:

> Can we delete them?
>

[rsc, 2011-11-04 00:34:09]

http://codereview.appspot.com/5339041/diff/8001/src/pkg/net/http/httputil/dum...
File src/pkg/net/http/httputil/dump.go (right):

http://codereview.appspot.com/5339041/diff/8001/src/pkg/net/http/httputil/dum...
src/pkg/net/http/httputil/dump.go:46: // DumpRequestOut returns the outbound
wire representation of req,
Delete entire paragraph (mostly duplicated with DumpRequest).

// DumpRequestOut is like DumpRequest but includes
// headers that the standard http.Transport adds,
// such as X-Whatever.

[rsc, 2011-11-04 00:34:52]

LGTM

Should the tests move too?

[bradfitz, 2011-11-04 00:36:43]

On Thu, Nov 3, 2011 at 5:34 PM, <rsc@golang.org> wrote:

> LGTM
>
> Should the tests move too?
>

Which?

I got DumpRequest's.

Chunked could use some [more?] tests.  Didn't see any.  I'll add some
before submitting.

[rsc, 2011-11-04 00:38:48]

Missing a file in the CL?
There are no httputil/*_test.go files.

[bradfitz, 2011-11-04 00:40:05]

On Thu, Nov 3, 2011 at 5:38 PM, Russ Cox <rsc@golang.org> wrote:

> Missing a file in the CL?
> There are no httputil/*_test.go files.
>

Oh, indeed.

"hg p" normally tells me that, but isn't now.  hg status does:

$ hg p
5339041:
httputil: move dump and chunking functions out of http
 DumpRequest, DumpResponse, NewChunkedReader, NewChunkedWriter
 Also, adds DumpRequestOut (for dumping outbound requests),
since DumpRequest's ambiguity (the "wire representation" in
what direction?) was often a source of confusion and bug
reports.

Reviewer: rsc@golang.org
rsc: LGTM
CC: golang-dev@googlegroups.com
Files:
src/pkg/net/http/Makefile
src/pkg/net/http/chunked.go
src/pkg/net/http/dump.go
src/pkg/net/http/httputil/Makefile
src/pkg/net/http/httputil/chunked.go
src/pkg/net/http/httputil/dump.go
src/pkg/net/http/request.go
src/pkg/net/http/requestwrite_test.go
src/pkg/net/http/transfer.go

$ hg st
M src/pkg/net/http/Makefile
M src/pkg/net/http/chunked.go
M src/pkg/net/http/httputil/Makefile
M src/pkg/net/http/request.go
M src/pkg/net/http/requestwrite_test.go
M src/pkg/net/http/transfer.go
A src/pkg/net/http/httputil/chunked.go
A src/pkg/net/http/httputil/dump.go
R src/pkg/net/http/dump.go
? src/pkg/net/http/httputil/#dump.go#
? src/pkg/net/http/httputil/dump_test.go

[bradfitz, 2011-11-04 00:46:51]

Hello rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2011-11-04 01:02:28]

Hello rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2011-11-04 01:03:28]

Now with chunking tests. And the old dump tests actually in the CL.

On Thu, Nov 3, 2011 at 6:02 PM, <bradfitz@golang.org> wrote:

> Hello rsc@golang.org (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
>
http://codereview.appspot.com/**5339041/<http://codereview.appspot.com/5339041/>
>

[adg, 2011-11-04 01:06:59]

LGTM

http://codereview.appspot.com/5339041/diff/15002/src/pkg/net/http/httputil/ch...
File src/pkg/net/http/httputil/chunked_test.go (right):

http://codereview.appspot.com/5339041/diff/15002/src/pkg/net/http/httputil/ch...
src/pkg/net/http/httputil/chunked_test.go:21: if g, e := b.String(),
"7\r\nhello, \r\n17\r\nworld! 0123456789abcdef\r\n0\r\n"; g != e {
can you programmatically generate the expected string?

const msg1 = "hello, "
const msg2 = "world! sdafhdshfdsa"
w.Write([]byte(msg1))
w.Write([]byte(msg2))

exp1 := fmt.Sprintf("%d\r\n%s\r\n%d\r\n%s\r\n0\r\n", len(msg1), msg1, len(msg2),
msg2)
exp2 := msg1 + msg2

[bradfitz, 2011-11-04 01:09:59]

Done, to some degree.  I don't want to replicate the code in the test,
though, otherwise what am I testing?  I think it's fine to have some golden
data in tests.

But I did kill some of the stutter.


On Thu, Nov 3, 2011 at 6:06 PM, <adg@golang.org> wrote:

> LGTM
>
>
> http://codereview.appspot.com/**5339041/diff/15002/src/pkg/**
>
net/http/httputil/chunked_**test.go<http://codereview.appspot.com/5339041/diff/15002/src/pkg/net/http/httputil/chunked_test.go>
> File src/pkg/net/http/httputil/**chunked_test.go (right):
>
> http://codereview.appspot.com/**5339041/diff/15002/src/pkg/**
>
net/http/httputil/chunked_**test.go#newcode21<http://codereview.appspot.com/5339041/diff/15002/src/pkg/net/http/httputil/chunked_test.go#newcode21>
> src/pkg/net/http/httputil/**chunked_test.go:21: if g, e := b.String(),
> "7\r\nhello, \r\n17\r\nworld! 0123456789abcdef\r\n0\r\n"; g != e {
> can you programmatically generate the expected string?
>
> const msg1 = "hello, "
> const msg2 = "world! sdafhdshfdsa"
> w.Write([]byte(msg1))
> w.Write([]byte(msg2))
>
> exp1 := fmt.Sprintf("%d\r\n%s\r\n%d\r\**n%s\r\n0\r\n", len(msg1), msg1,
> len(msg2), msg2)
> exp2 := msg1 + msg2
>
>
http://codereview.appspot.com/**5339041/<http://codereview.appspot.com/5339041/>
>

[rsc, 2011-11-04 01:11:08]

hg p only tells you if you have hg add'ed the file

[bradfitz, 2011-11-04 01:12:55]

*** Submitted as http://code.google.com/p/go/source/detail?r=37533e1cfe43 ***

httputil: move dump and chunking functions out of http

This moves DumpRequest, DumpResponse, NewChunkedReader,
and NewChunkedWriter out of http, as part of the continued
http diet plan.

Also, adds DumpRequestOut (for dumping outbound requests),
since DumpRequest's ambiguity (the "wire representation" in
what direction?) was often a source of confusion and bug
reports.

R=rsc, adg
CC=golang-dev
http://codereview.appspot.com/5339041