code review 4430054: gofmt: add -diff

gofmt: add -diff

Some code duplication with gofix.

[crawshaw2, 2011-04-17 21:12:43]

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-04-18 18:04:44]

LGTM

I have been meaning to add this to gofmt
since writing the code for gofix

Leaving for gri to review + submit.

[bradfitz, 2011-04-18 18:06:22]

Which systems have a diff command available, at least by default?  Would be
nice at some point to have this done natively.</pony>

On Mon, Apr 18, 2011 at 11:04 AM, <rsc@golang.org> wrote:

> LGTM
>
> I have been meaning to add this to gofmt
> since writing the code for gofix
>
> Leaving for gri to review + submit.
>
>
>
>
> http://codereview.appspot.com/4430054/
>

[r2, 2011-04-18 18:20:18]

On Apr 18, 2011, at 11:06 AM, Brad Fitzpatrick wrote:

> Which systems have a diff command available, at least by default?  Would be
nice at some point to have this done natively.</pony>

that's not a pony, that's a team of clydesdales.

-rob

[gri, 2011-04-18 23:25:32]

LGTM.

Please address the suggestions below.

Also, have you filled out the CLA form? (
http://code.google.com/legal/individual-cla-v1.0.html )

- gri

http://codereview.appspot.com/4430054/diff/4001/src/cmd/gofmt/gofmt.go
File src/cmd/gofmt/gofmt.go (right):

http://codereview.appspot.com/4430054/diff/4001/src/cmd/gofmt/gofmt.go#newcode32
src/cmd/gofmt/gofmt.go:32: doDiff      = flag.Bool("diff", false, "display diffs
instead of rewriting files")
Can we just call the flag "d" - all main operation modes have one-letter flags.

http://codereview.appspot.com/4430054/diff/4001/src/cmd/gofmt/gofmt.go#newcod...
src/cmd/gofmt/gofmt.go:243: 
The style in this file is to have two empty lines between functions.
(at some point I'll have gofmt take care of such details).

[rsc, 2011-04-18 23:42:31]

> The style in this file is to have two empty lines between functions.
> (at some point I'll have gofmt take care of such details).

It would be easier to have gofmt cut everything to one blank line.  :-)

[r2, 2011-04-18 23:45:45]

On Apr 18, 2011, at 4:42 PM, Russ Cox wrote:

>> The style in this file is to have two empty lines between functions.
>> (at some point I'll have gofmt take care of such details).
> 
> It would be easier to have gofmt cut everything to one blank line.  :-)

maybe we should compromise on three blank lines.

-rob

[adg, 2011-04-18 23:52:40]

On 19 April 2011 09:45, Rob 'Commander' Pike <r@google.com> wrote:
>
> On Apr 18, 2011, at 4:42 PM, Russ Cox wrote:
>
>>> The style in this file is to have two empty lines between functions.
>>> (at some point I'll have gofmt take care of such details).
>>
>> It would be easier to have gofmt cut everything to one blank line.  :-)
>
> maybe we should compromise on three blank lines.

I think it would be cleaner to put something like this between each declaration:

/****************************************************************************/

I'm flexible on whether to use an asterisk or some other character. #
would look just as nice, probably.

Andrew

[crawshaw2, 2011-04-19 05:19:44]

On 2011/04/18 23:25:32, gri wrote:
> Also, have you filled out the CLA form? (
> http://code.google.com/legal/individual-cla-v1.0.html )

Done, though google own what I do anyway (crawshaw@).

[crawshaw2, 2011-04-19 05:20:07]

http://codereview.appspot.com/4430054/diff/4001/src/cmd/gofmt/gofmt.go
File src/cmd/gofmt/gofmt.go (right):

http://codereview.appspot.com/4430054/diff/4001/src/cmd/gofmt/gofmt.go#newcode32
src/cmd/gofmt/gofmt.go:32: doDiff      = flag.Bool("diff", false, "display diffs
instead of rewriting files")
On 2011/04/18 23:25:32, gri wrote:
> Can we just call the flag "d" - all main operation modes have one-letter
flags.

Done.

http://codereview.appspot.com/4430054/diff/4001/src/cmd/gofmt/gofmt.go#newcod...
src/cmd/gofmt/gofmt.go:243: 
On 2011/04/18 23:25:32, gri wrote:
> The style in this file is to have two empty lines between functions.
> (at some point I'll have gofmt take care of such details).

Done.

To remove any confusion, I've added a function header:

/**
 * diff: it diffs
 * @param b1 []byte some bytes
 * @param b2 []byte some other bytes
 * @precondition you must first create the universe
 */

[peterGo, 2011-04-19 12:50:09]

Brad,

On Apr 18, 2:06 pm, Brad Fitzpatrick <bradf...@golang.org> wrote:
> Which systems have a diff command available, at least by default?

Windows doesn't have a diff command installed. Plus, the diff programs aren't
necessarily named diff.

Also, I don't see where this dependency on a diff command is documented for the
gofmt command.

Peter

[rsc, 2011-04-19 13:23:24]

>> Also, have you filled out the CLA form? (
>> http://code.google.com/legal/individual-cla-v1.0.html )
>
> Done, though google own what I do anyway (crawshaw@).

http://codereview.appspot.com/4425054

[peterGo, 2011-04-19 17:24:50]

Not surprisingly, even on a Windows machine with diff, the CL fails.

Exception: STATUS_ACCESS_VIOLATION at eip=60804550

eax=00000000 ebx=004329DA ecx=00000000 edx=60E900D4 esi=60E92414 edi=00000000

ebp=0028FF18 esp=0028FED0 program=us

cs=0023 ds=002B es=002B fs=0053 gs=002B ss=002B

Stack trace:

Frame     Function  Args

0028FF18  60804550  (6089C7A0, FFFFFFFE, 00000050, 6089C6A4)

0028FF58  60804B65  (00000000, 00000000, 00000000, 00000000)

0028FF78  0042033F  (00403360, 037F0000, 0028FF94, 767D33CA)

0028FF88  0040103E  (7EFDE000, 0028FFD4, 77839ED2, 7EFDE000)

0028FF94  767D33CA  (7EFDE000, 77A2AB8D, 00000000, 00000000)

0028FFD4  77839ED2  (00401000, 7EFDE000, 00000000, 00000000)

0028FFEC  77839EA5  (00401000, 7EFDE000, 00000000, 78746341)

Peter

[r, 2011-04-19 17:36:08]

Please let's not check this in until we have the Windows issue resolved.

[brainman, 2011-04-20 01:30:33]

On 2011/04/19 17:24:50, peterGo wrote:
> Not surprisingly, even on a Windows machine with diff, the CL fails.
> 

See my explanation of why it is happening here
http://code.google.com/p/go/issues/detail?id=1719#c3.

The problem could be overcome by a trick similar to what I did for gotest:

21d20
< 	"runtime"
264,269c263
< 	args := []string{"diff", "-u", f1.Name(), f2.Name()}
< 	if runtime.GOOS == "windows" {
< 		args = append([]string{"cmd", "/c"}, args...)
< 	}
< 
< 	diffcmd, err := exec.LookPath(args[0])
---
> 	diffcmd, err := exec.LookPath("diff")
274c268,269
< 	c, err := exec.Run(diffcmd, args, nil, "", exec.DevNull, exec.Pipe,
exec.MergeWithStdout)
---
> 	c, err := exec.Run(diffcmd, []string{"diff", "-u", f1.Name(), f2.Name()},
> 		nil, "", exec.DevNull, exec.Pipe, exec.MergeWithStdout)

If diff.exe is not present (mingw or whatever is not installed), there is no
solution. It'll just fail like this:

$ /g/src/cmd/gofmt/gofmt -d /c/tmp/a.go
diff c:/tmp/a.go fixed/c:/tmp/a.go
'diff' is not recognized as an internal or external command,
operable program or batch file.

$

Alex

[brainman, 2011-04-20 01:37:22]

On 2011/04/19 17:36:08, r wrote:
> Please let's not check this in until we have the Windows issue resolved.

Please see my reply to Peter.

The big issue I see here is user computer doesn't have diff.exe program. There
is no "standard" Windows diff.exe program. One comes with mingw, but if you
don't have mingw installed, ...

Assuming we ignore presence of diff.exe issue, I think it is OK to submit as is.
I'm happy to send my windows specific patch separately later (It seems, gofix
has similar problem, and I need to fix it anyway).

Alex

[gri, 2011-04-20 16:52:32]

I am going to submit this now. It sounds like the Windows problem may
be fixed soon. But even if it is not, the same issue arises with
gofix. Furthermore, as long as diff is not invoked, things are fine.
When it is invoked, in the worst case we get an error message.

Eventually we should replace this with a go-native diff implementation.

Thanks.
- gri

On Wed, Apr 20, 2011 at 5:55 AM, brainman <alex.brainman@gmail.com> wrote:
> I think I've found why mingw programs crash when invoked by go
> code: http://codereview.appspot.com/4435059/. If we apply the fix, we won't
> need any special windows treatment. So, please, ignore my original diff.
> Alex

[gri, 2011-04-20 17:08:04]

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

gofmt: add -diff

Some code duplication with gofix.

R=rsc, gri, bradfitzgo, r2, adg, peterGo, r, brainman
CC=golang-dev
http://codereview.appspot.com/4430054

Committer: Robert Griesemer <gri@golang.org>