code review 5795069: net/http: ensure triv.go compiles and runs

net/http: ensure triv.go compiles and runs

[rh, 2012-03-13 02:47:05]

Hello golang-dev@googlegroups.com,

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

[bradfitz, 2012-03-13 02:48:27]

LGTM

man, this file is always rotting.


On Mon, Mar 12, 2012 at 7:47 PM, <robert.hencke@gmail.com> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> net/http: ensure triv.go compiles and runs
>
> Please review this at
http://codereview.appspot.com/**5795069/<http://codereview.appspot.com/5795069/>
>
> Affected files:
>  M src/pkg/net/http/triv.go
>
>
> Index: src/pkg/net/http/triv.go
> ==============================**==============================**=======
> --- a/src/pkg/net/http/triv.go
> +++ b/src/pkg/net/http/triv.go
> @@ -109,19 +109,19 @@
>                return
>        }
>        io.Copy(rw, r)
> -       wait, err := p.Wait(0)
> +       wait, err := p.Wait()
>        if err != nil {
>                fmt.Fprintf(rw, "wait: %s\n", err)
>                return
>        }
> -       if !wait.Exited() || wait.ExitStatus() != 0 {
> +       if !wait.Exited() || !wait.Success() {
>                fmt.Fprintf(rw, "date: %v\n", wait)
>                return
>        }
>  }
>
>  func Logger(w http.ResponseWriter, req *http.Request) {
> -       log.Print(req.URL.Raw)
> +       log.Print(req.URL)
>        w.WriteHeader(404)
>        w.Write([]byte("oops"))
>  }
>
>
>

[dsymonds, 2012-03-13 02:48:51]

Can we add a test or something to make sure it stays compiling?

[dave_cheney.net, 2012-03-13 02:57:59]

Could this possibly converted an example_test ?

On 13/03/2012, at 13:48, David Symonds <dsymonds@golang.org> wrote:

> Can we add a test or something to make sure it stays compiling?

[rh, 2012-03-13 03:06:33]

On 2012/03/13 02:48:51, dsymonds wrote:
> Can we add a test or something to make sure it stays compiling?

Would adding (roughly) 'go build triv.go' to make.bash be acceptable to you?

[rh, 2012-03-13 03:10:45]

On 2012/03/13 02:57:59, dfc wrote:
> Could this possibly converted an example_test ?

I am not sure how to translate it to example form, since it serves its examples
as a web server.  I am open to suggestions, though.

[r2, 2012-03-13 03:17:02]

An example only needs to compile. It won't be run if there is no expected
output. I agree we should do something to guarantee it continues to compile,
although ironically it is much less likely to rot given Go 1.

-rob

[rh, 2012-03-13 04:01:05]

Thank you all for the reviews.  PTAL.

Compiling it in run.bash seemed the least disruptive, but I'm open to trying
again if that's not palatable.

[bradfitz, 2012-03-13 04:03:17]

Seems arbitrary there, and it's not also in run.bat.

I'd just submit the fix, without the run.bash part.

Like Rob said, Go 1.  It's all stable now!


On Mon, Mar 12, 2012 at 9:01 PM, <robert.hencke@gmail.com> wrote:

> Thank you all for the reviews.  PTAL.
>
> Compiling it in run.bash seemed the least disruptive, but I'm open to
> trying again if that's not palatable.
>
>
http://codereview.appspot.com/**5795069/<http://codereview.appspot.com/5795069/>
>

[rh, 2012-03-13 04:06:31]

On 2012/03/13 04:03:17, bradfitz wrote:
> Seems arbitrary there, and it's not also in run.bat.
> 
> I'd just submit the fix, without the run.bash part.
> 
> Like Rob said, Go 1.  It's all stable now!

Thanks for the review - done.  PTAL.

[bradfitz, 2012-03-13 04:13:36]

LGTM

On Mon, Mar 12, 2012 at 9:06 PM, <robert.hencke@gmail.com> wrote:

> On 2012/03/13 04:03:17, bradfitz wrote:
>
>> Seems arbitrary there, and it's not also in run.bat.
>>
>
>  I'd just submit the fix, without the run.bash part.
>>
>
>  Like Rob said, Go 1.  It's all stable now!
>>
>
> Thanks for the review - done.  PTAL.
>
>
http://codereview.appspot.com/**5795069/<http://codereview.appspot.com/5795069/>
>

[bradfitz, 2012-03-13 13:15:30]

Actually, NOT LGTM.  I just looked at what this program is actually trying
to do and it's very ancient.

Can you update the DateServer to use os/exec and Output instead?  Something
like:

// exec a program, redirecting output
func DateServer(rw http.ResponseWriter, req *http.Request) {

rw.Header().Set("Content-Type", "text/plain; charset=utf-8")
out, err := exec.Command("date").Output()
if err != nil {
   http.Error(...)
   return
}
fmt.Fprintf(rw, "date: %s", out)
}



On Mon, Mar 12, 2012 at 9:13 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> LGTM
>
>
> On Mon, Mar 12, 2012 at 9:06 PM, <robert.hencke@gmail.com> wrote:
>
>> On 2012/03/13 04:03:17, bradfitz wrote:
>>
>>> Seems arbitrary there, and it's not also in run.bat.
>>>
>>
>>  I'd just submit the fix, without the run.bash part.
>>>
>>
>>  Like Rob said, Go 1.  It's all stable now!
>>>
>>
>> Thanks for the review - done.  PTAL.
>>
>>
http://codereview.appspot.com/**5795069/<http://codereview.appspot.com/5795069/>
>>
>
>

[rh, 2012-03-15 02:47:06]

I'm sorry this took so long.  PTAL.

[bradfitz, 2012-03-15 03:25:49]

LGTM

There's other wrong/old stuff in here too, but this is much better, thanks.


On Wed, Mar 14, 2012 at 7:47 PM, <robert.hencke@gmail.com> wrote:

> I'm sorry this took so long.  PTAL.
>
>
http://codereview.appspot.com/**5795069/<http://codereview.appspot.com/5795069/>
>

[bradfitz, 2012-03-15 03:26:02]

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

net/http: ensure triv.go compiles and runs

R=golang-dev, bradfitz, dsymonds, dave, r
CC=golang-dev
http://codereview.appspot.com/5795069

Committer: Brad Fitzpatrick <bradfitz@golang.org>