code review 5696084: cmd/fix: add rules for net/http -> net/http/httputil renames

cmd/fix: add rules for net/http -> net/http/httputil renames

And merge the httputil fix into go1rename.

[bradfitz, 2012-02-27 01:48:57]

Hello golang-dev@googlegroups.com,

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

[r, 2012-02-27 01:54:08]

LGTM

[bradfitz, 2012-02-27 02:04:21]

David is confused why this CL works, yet it does.

Clues welcome.

go1pkgrename should run before go1rename, and go1pkgrename should change
"http" -> "net/http".  So why is this CL, which converts "http" ->
"net/http/httputil" still working?  (I tested it by hand with an r60 x.go
file and "go tool fix x.go")


On Mon, Feb 27, 2012 at 12:54 PM, <r@golang.org> wrote:

> LGTM
>
>
>
http://codereview.appspot.com/**5696084/<http://codereview.appspot.com/5696084/>
>

[bradfitz, 2012-02-27 02:13:04]

Oh, and we also have the "httputil" fix, which is largely redundant?

Should we just ditch the httputil fix, since it just looks like a
pre-easy-renames manual fix?


On Mon, Feb 27, 2012 at 1:04 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> David is confused why this CL works, yet it does.
>
> Clues welcome.
>
> go1pkgrename should run before go1rename, and go1pkgrename should change
> "http" -> "net/http".  So why is this CL, which converts "http" ->
> "net/http/httputil" still working?  (I tested it by hand with an r60 x.go
> file and "go tool fix x.go")
>
>
> On Mon, Feb 27, 2012 at 12:54 PM, <r@golang.org> wrote:
>
>> LGTM
>>
>>
>>
http://codereview.appspot.com/**5696084/<http://codereview.appspot.com/5696084/>
>>
>
>

[dsymonds, 2012-02-27 02:14:18]

http://codereview.appspot.com/5696084/diff/1002/src/cmd/fix/go1rename.go
File src/cmd/fix/go1rename.go (right):

http://codereview.appspot.com/5696084/diff/1002/src/cmd/fix/go1rename.go#newc...
src/cmd/fix/go1rename.go:72: OldImport: "http",
All these old imports should be net/http; there's already a gofix that renames
http to net/http.

But I'm baffled why this worked on your file you showed me. I understand why the
test file in this CL works (the test only runs this fix), but the go1pkgrename
should run first, rename "http" to "net/http", and then this code should not
match that modified AST.

[dsymonds, 2012-02-27 02:15:16]

There's a potential bug in the httputil gofix. Proposed fix in
http://codereview.appspot.com/5700077.

[bradfitz, 2012-02-27 02:17:02]

On Mon, Feb 27, 2012 at 1:15 PM, David Symonds <dsymonds@golang.org> wrote:

> There's a potential bug in the httputil gofix. Proposed fix in
> http://codereview.appspot.com/5700077.
>

no test

feels like more guesswork without

[dsymonds, 2012-02-27 02:21:09]

On Mon, Feb 27, 2012 at 1:17 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:

> On Mon, Feb 27, 2012 at 1:15 PM, David Symonds <dsymonds@golang.org> wrote:
>>
>> There's a potential bug in the httputil gofix. Proposed fix in
>> http://codereview.appspot.com/5700077.
>
>
> no test
>
> feels like more guesswork without

test added.

[bradfitz, 2012-02-27 23:18:58]

Okay, I'd like to get Russ's opinion before either/both of these httputil
CLs go in.

I feel like we could almost ditch most the exiting httputil gofix code and
change it to a rename one.


On Mon, Feb 27, 2012 at 1:21 PM, David Symonds <dsymonds@golang.org> wrote:

> On Mon, Feb 27, 2012 at 1:17 PM, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
>
> > On Mon, Feb 27, 2012 at 1:15 PM, David Symonds <dsymonds@golang.org>
> wrote:
> >>
> >> There's a potential bug in the httputil gofix. Proposed fix in
> >> http://codereview.appspot.com/5700077.
> >
> >
> > no test
> >
> > feels like more guesswork without
>
> test added.
>

[dsymonds, 2012-02-28 00:05:34]

On Tue, Feb 28, 2012 at 10:18 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote:

> Okay, I'd like to get Russ's opinion before either/both of these httputil
> CLs go in.
>
> I feel like we could almost ditch most the exiting httputil gofix code and
> change it to a rename one.

Agreed. I don't care deeply about my CL, but it'd be good to clarify
the expected semantics of individual gofixes. My understanding was
that individually they were meant to be as narrow as possible such
that they would correctly update intermediate code. That's the purpose
of the date stamp on the fixes too: to make sure they get run in the
correct order. I could be wrong on this.


Dave.

[r2, 2012-02-28 00:08:40]

On Feb 28, 2012, at 11:05 AM, David Symonds wrote:

> On Tue, Feb 28, 2012 at 10:18 AM, Brad Fitzpatrick <bradfitz@golang.org>
wrote:
> 
>> Okay, I'd like to get Russ's opinion before either/both of these httputil
>> CLs go in.
>> 
>> I feel like we could almost ditch most the exiting httputil gofix code and
>> change it to a rename one.
> 
> Agreed. I don't care deeply about my CL, but it'd be good to clarify
> the expected semantics of individual gofixes. My understanding was
> that individually they were meant to be as narrow as possible such
> that they would correctly update intermediate code. That's the purpose
> of the date stamp on the fixes too: to make sure they get run in the
> correct order. I could be wrong on this.

not to disagree with your sentiment - it would be good to understand what's
going on - but the current fixes should soon be relics. i hope at some point
after go 1 we freeze it, flush the fixes, and start a new set.

-rob

[dsymonds, 2012-02-28 00:21:46]

On Tue, Feb 28, 2012 at 11:08 AM, Rob 'Commander' Pike <r@google.com> wrote:

> i hope at some point after go 1 we freeze it, flush the fixes, and start a new
set.

Or not? We shouldn't need any fixes if we are preserving API
compatibility, right?

[r2, 2012-02-28 00:23:22]

On Feb 28, 2012, at 11:21 AM, David Symonds wrote:

> On Tue, Feb 28, 2012 at 11:08 AM, Rob 'Commander' Pike <r@google.com> wrote:
> 
>> i hope at some point after go 1 we freeze it, flush the fixes, and start a
new set.
> 
> Or not? We shouldn't need any fixes if we are preserving API
> compatibility, right?

exactly. further evolution of go fix should be about versions to follow.

-rob

[rsc, 2012-03-12 18:36:32]

On Sun, Feb 26, 2012 at 21:04, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> David is confused why this CL works, yet it does.

Not for me.

$ cat /tmp/x.go
package main

import "http"

var (
	_ = http.ErrPersistEOF
	_ = http.ClientConn
)
$ go tool fix -diff /tmp/x.go
/tmp/x.go: fixed go1rename
diff /tmp/x.go fixed//tmp/x.go
--- /var/folders/00/05_b8000h01000cxqpysvccm000n9d/T/go-fix484560617	2012-03-12
14:34:48.000000000 -0400
+++ /var/folders/00/05_b8000h01000cxqpysvccm000n9d/T/go-fix776465716	2012-03-12
14:34:48.000000000 -0400
@@ -1,6 +1,6 @@
 package main

-import "http"
+import "net/http"

 var (
 	_ = http.ErrPersistEOF
$

Note that the http. references were not rewritten.
If I change your OldImport: "http" lines to
OldImport: "net/http", then it does work.

I suggest to delete the now-redundant httputil fix
and update this CL.

Russ

[bradfitz, 2012-03-12 20:12:03]

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

Please take another look.

[bradfitz, 2012-03-12 20:12:28]

On Mon, Mar 12, 2012 at 11:36 AM, Russ Cox <rsc@golang.org> wrote:

> On Sun, Feb 26, 2012 at 21:04, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
> > David is confused why this CL works, yet it does.
>
> Not for me.
>
> $ cat /tmp/x.go
> package main
>
> import "http"
>
> var (
>        _ = http.ErrPersistEOF
>        _ = http.ClientConn
> )
> $ go tool fix -diff /tmp/x.go
> /tmp/x.go: fixed go1rename
> diff /tmp/x.go fixed//tmp/x.go
> --- /var/folders/00/05_b8000h01000cxqpysvccm000n9d/T/go-fix484560617
>  2012-03-12
> 14:34:48.000000000 -0400
> +++ /var/folders/00/05_b8000h01000cxqpysvccm000n9d/T/go-fix776465716
>  2012-03-12
> 14:34:48.000000000 -0400
> @@ -1,6 +1,6 @@
>  package main
>
> -import "http"
> +import "net/http"
>
>  var (
>        _ = http.ErrPersistEOF
> $
>
> Note that the http. references were not rewritten.
> If I change your OldImport: "http" lines to
> OldImport: "net/http", then it does work.
>
> I suggest to delete the now-redundant httputil fix
> and update this CL.


Done.

[rsc, 2012-03-12 20:22:09]

LGTM

[bradfitz, 2012-03-12 20:25:50]

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

cmd/fix: add rules for net/http -> net/http/httputil renames

And merge the httputil fix into go1rename.

R=golang-dev, r, dsymonds, r, rsc
CC=golang-dev
http://codereview.appspot.com/5696084