code review 4276059: misc/emacs: gofmt: don't clobber the current buffer on ...

misc/emacs: gofmt: don't clobber the current buffer on failure

Change M-x gofmt to display errors in a new buffer instead of
clobbering the current buffer.

Add gofmt-before-save, which runs gofmt when in go-mode.  This
can be used with before-save-hook.  Add to your .emacs:
  (add-hook 'before-save-hook 'gofmt-before-save)

[Sameer (personal), 2011-03-18 20:08:04]

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

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

[rsc, 2011-03-18 21:29:10]

Fine with me.  I'd like some emacs user to sign off on this though.

[Sameer (personal), 2011-03-19 00:42:22]

Sure, I'm open to any suggested reviewers.  I just grabbed you two from the
filelog.

My only thought about this CL is that it would be simpler if we changed the
existing emacs gofmt function to behave like the new one I added.  Then we
could avoid having two functions, since gofmt would be a valid before-save
hook itself.
On Mar 18, 2011 5:29 PM, "Russ Cox" <rsc@golang.org> wrote:

[Sameer (personal), 2011-03-19 02:01:28]

http://codereview.appspot.com/4276059/diff/8003/misc/emacs/go-mode.el
File misc/emacs/go-mode.el (right):

http://codereview.appspot.com/4276059/diff/8003/misc/emacs/go-mode.el#newcode540
misc/emacs/go-mode.el:540: (when (eq major-mode 'go-mode)
This when guard is the only bit that's specific to the hook behavior (since
before-save-hook runs in all modes, and we only want to run the rest of this on
Go files).  So we could just change all of this to:
  (defun gofmt ...the code below...)
  (defun gofmt-before-save (when (eq major-mode 'go-mode) gofmt))
Provided we're willing to change how M-x gofmt works for people now.

If we want to retain the current behavior of M-x gofmt, another option is to do:
  (defun gofmt-noclobber ...the code below...)
  (defun gofmt-before-save (when (eq major-mode 'go-mode) gofmt-noclobber))

[rsc, 2011-03-19 03:07:20]

On Fri, Mar 18, 2011 at 20:42, Sameer Ajmani <ajmani@gmail.com> wrote:
> Sure, I'm open to any suggested reviewers.  I just grabbed you two from the
> filelog.

It's a good choice.  I'm just inviting any other emacs
users to speak up if they want to review it, since
Austin is no doubt busy with other things.

Russ

[Sameer (personal), 2011-03-21 01:44:53]

I've simplified this CL per my earlier comments on it: gofmt now replaces the
current buffer only on success; on failure, it displays the errors in a new
buffer containing the errors.  gofmt-before-save is a simple wrapper that calls
gofmt when in go-mode, appropriate for use as a before-save-hook.


On 2011/03/19 03:07:20, rsc wrote:
> On Fri, Mar 18, 2011 at 20:42, Sameer Ajmani <mailto:ajmani@gmail.com> wrote:
> > Sure, I'm open to any suggested reviewers.&nbsp; I just grabbed you two from
the
> > filelog.
> 
> It's a good choice.  I'm just inviting any other emacs
> users to speak up if they want to review it, since
> Austin is no doubt busy with other things.
> 
> Russ

[amdragon_mit.edu, 2011-03-21 08:38:35]

On Fri, Mar 18, 2011 at 11:07 PM, Russ Cox <rsc@golang.org> wrote:
> On Fri, Mar 18, 2011 at 20:42, Sameer Ajmani <ajmani@gmail.com> wrote:
>> Sure, I'm open to any suggested reviewers.  I just grabbed you two from the
>> filelog.
>
> It's a good choice.  I'm just inviting any other emacs
> users to speak up if they want to review it, since
> Austin is no doubt busy with other things.

Of course, but I can always spare a little time for Go!

[aclements, 2011-03-21 08:42:37]

http://codereview.appspot.com/4276059/diff/3/misc/emacs/go-mode.el
File misc/emacs/go-mode.el (left):

http://codereview.appspot.com/4276059/diff/3/misc/emacs/go-mode.el#oldcode502
misc/emacs/go-mode.el:502: 
The (provide 'go-mode) should be the last line in the file both for stylistic
reasons and in case there's an eval-after-load registered for go-mode.  (Yes,
this was a problem with the original gofmt function, but this patch makes it
more so.)

http://codereview.appspot.com/4276059/diff/3/misc/emacs/go-mode.el
File misc/emacs/go-mode.el (right):

http://codereview.appspot.com/4276059/diff/3/misc/emacs/go-mode.el#newcode503
misc/emacs/go-mode.el:503: (defun get-empty-buffer (name)
This should be go-mode-get-empty-buffer.  Or possibly gofmt-get-empty-buffer. 
Or removed entirely (see comment below).

http://codereview.appspot.com/4276059/diff/3/misc/emacs/go-mode.el#newcode508
misc/emacs/go-mode.el:508: (set-buffer buf)
It would be simpler and more idiomatic to use with-current-buffer instead of
save-current-buffer plus set-buffer.

http://codereview.appspot.com/4276059/diff/3/misc/emacs/go-mode.el#newcode523
misc/emacs/go-mode.el:523: (old-point (point))
I believe you no longer have to save mark and point, since
shell-command-on-region is going to a different buffer instead of replacing the
region.

http://codereview.appspot.com/4276059/diff/3/misc/emacs/go-mode.el#newcode525
misc/emacs/go-mode.el:525: (errbuf (get-empty-buffer "Gofmt Errors")))
These buffer names should be enclosed in *'s, since they're non-file buffers. 
In fact, the first should probably be " *Gofmt Output*" (with a space at the
beginning), since it's an ephemeral buffer.  Better yet, use generate-new-buffer
or with-temp-buffer for the output buffer, since you kill the output buffer
unconditionally and this will save you the trouble of wiping it.  Then, since
you'd only be wiping the error buffer, you could remove get-empty-buffer and
reduce this to
  (let (... (errbuf (get-buffer-create "*Gofmt Errors*")))
    (with-current-buffer errbuf (erase-buffer))
    ...)

http://codereview.appspot.com/4276059/diff/3/misc/emacs/go-mode.el#newcode529
misc/emacs/go-mode.el:529: (if (= 0 (shell-command-on-region (point-min)
(point-max) "gofmt"
Use call-process-region instead, since shell-command-on-region is just
interactive baggage on top of that.  Also, call-process-region definitely won't
muck with your point and mark, so you won't have to save and restore them.

http://codereview.appspot.com/4276059/diff/3/misc/emacs/go-mode.el#newcode546
misc/emacs/go-mode.el:546: (kill-buffer outbuf)))
The last two lines should be in the unwind of an unwind-protect around the rest
of the let body.  (Or, use with-temp-buffer for the output buffer, which will
take care of this for you).

http://codereview.appspot.com/4276059/diff/3/misc/emacs/go-mode.el#newcode551
misc/emacs/go-mode.el:551: (add-hook 'before-save-hook 'gofmt-before-save)"
Nit: Use #'gofmt-before-save.

http://codereview.appspot.com/4276059/diff/3/misc/emacs/go-mode.el#newcode554
misc/emacs/go-mode.el:554: (when (eq major-mode 'go-mode) (gofmt)))
Just a thought...  Since this is now mostly a rewrite of gofmt, would it be
better as two separate patches?  Certainly the commit message needs updating, at
least.

[Sameer (personal), 2011-03-22 00:44:56]

http://codereview.appspot.com/4276059/diff/3/misc/emacs/go-mode.el
File misc/emacs/go-mode.el (right):

http://codereview.appspot.com/4276059/diff/3/misc/emacs/go-mode.el#newcode503
misc/emacs/go-mode.el:503: (defun get-empty-buffer (name)
On 2011/03/21 08:42:37, aclements wrote:
> This should be go-mode-get-empty-buffer.  Or possibly gofmt-get-empty-buffer. 
> Or removed entirely (see comment below).

Removed.

http://codereview.appspot.com/4276059/diff/3/misc/emacs/go-mode.el#newcode508
misc/emacs/go-mode.el:508: (set-buffer buf)
On 2011/03/21 08:42:37, aclements wrote:
> It would be simpler and more idiomatic to use with-current-buffer instead of
> save-current-buffer plus set-buffer.

Done.

http://codereview.appspot.com/4276059/diff/3/misc/emacs/go-mode.el#newcode523
misc/emacs/go-mode.el:523: (old-point (point))
On 2011/03/21 08:42:37, aclements wrote:
> I believe you no longer have to save mark and point, since
> shell-command-on-region is going to a different buffer instead of replacing
the
> region.

Yes, but I still need to restore the point and mark when I replace the current
buffer contents (when gofmt succeeds).

http://codereview.appspot.com/4276059/diff/3/misc/emacs/go-mode.el#newcode525
misc/emacs/go-mode.el:525: (errbuf (get-empty-buffer "Gofmt Errors")))
On 2011/03/21 08:42:37, aclements wrote:
> These buffer names should be enclosed in *'s, since they're non-file buffers. 
> In fact, the first should probably be " *Gofmt Output*" (with a space at the
> beginning), since it's an ephemeral buffer.  Better yet, use
generate-new-buffer
> or with-temp-buffer for the output buffer, since you kill the output buffer
> unconditionally and this will save you the trouble of wiping it.  Then, since
> you'd only be wiping the error buffer, you could remove get-empty-buffer and
> reduce this to
>   (let (... (errbuf (get-buffer-create "*Gofmt Errors*")))
>     (with-current-buffer errbuf (erase-buffer))
>     ...)

Done.

http://codereview.appspot.com/4276059/diff/3/misc/emacs/go-mode.el#newcode529
misc/emacs/go-mode.el:529: (if (= 0 (shell-command-on-region (point-min)
(point-max) "gofmt"
On 2011/03/21 08:42:37, aclements wrote:
> Use call-process-region instead, since shell-command-on-region is just
> interactive baggage on top of that.  Also, call-process-region definitely
won't
> muck with your point and mark, so you won't have to save and restore them.

I'm keeping shell-command-on-region, as call-process-region can only send stderr
to a file, and the code in shell-command-on-region to set up a temporary file
and read it back into a buffer is non-trivial.

http://codereview.appspot.com/4276059/diff/3/misc/emacs/go-mode.el#newcode546
misc/emacs/go-mode.el:546: (kill-buffer outbuf)))
On 2011/03/21 08:42:37, aclements wrote:
> The last two lines should be in the unwind of an unwind-protect around the
rest
> of the let body.  (Or, use with-temp-buffer for the output buffer, which will
> take care of this for you).

Done.

http://codereview.appspot.com/4276059/diff/3/misc/emacs/go-mode.el#newcode551
misc/emacs/go-mode.el:551: (add-hook 'before-save-hook 'gofmt-before-save)"
On 2011/03/21 08:42:37, aclements wrote:
> Nit: Use #'gofmt-before-save.

Done.  What's the hash for?

http://codereview.appspot.com/4276059/diff/3/misc/emacs/go-mode.el#newcode554
misc/emacs/go-mode.el:554: (when (eq major-mode 'go-mode) (gofmt)))
On 2011/03/21 08:42:37, aclements wrote:
> Just a thought...  Since this is now mostly a rewrite of gofmt, would it be
> better as two separate patches?  Certainly the commit message needs updating,
at
> least.

gofmt-before-save is trivial enough that I'd like to keep this as one patch.

[aclements, 2011-03-22 01:12:49]

LGTM (modulo updating the commit message).

http://codereview.appspot.com/4276059/diff/3/misc/emacs/go-mode.el
File misc/emacs/go-mode.el (right):

http://codereview.appspot.com/4276059/diff/3/misc/emacs/go-mode.el#newcode523
misc/emacs/go-mode.el:523: (old-point (point))
On 2011/03/22 00:44:57, Sameer Ajmani wrote:
> On 2011/03/21 08:42:37, aclements wrote:
> > I believe you no longer have to save mark and point, since
> > shell-command-on-region is going to a different buffer instead of replacing
> the
> > region.
> 
> Yes, but I still need to restore the point and mark when I replace the current
> buffer contents (when gofmt succeeds).

True.  Good to see you updated the comment, though.

It may be worth considering insert-file-contents to update the buffer (with the
replace argument set to t).  That tries really hard to keep point on the same
text it started on, even if things move around it, but has the obvious
disadvantage that the gofmt output has to go to a temporary file instead of just
a buffer.  Just something to think about.

http://codereview.appspot.com/4276059/diff/3/misc/emacs/go-mode.el#newcode529
misc/emacs/go-mode.el:529: (if (= 0 (shell-command-on-region (point-min)
(point-max) "gofmt"
On 2011/03/22 00:44:57, Sameer Ajmani wrote:
> On 2011/03/21 08:42:37, aclements wrote:
> > Use call-process-region instead, since shell-command-on-region is just
> > interactive baggage on top of that.  Also, call-process-region definitely
> won't
> > muck with your point and mark, so you won't have to save and restore them.
> 
> I'm keeping shell-command-on-region, as call-process-region can only send
stderr
> to a file, and the code in shell-command-on-region to set up a temporary file
> and read it back into a buffer is non-trivial.

'Doh.  Yes, you're right.

http://codereview.appspot.com/4276059/diff/3/misc/emacs/go-mode.el#newcode551
misc/emacs/go-mode.el:551: (add-hook 'before-save-hook 'gofmt-before-save)"
On 2011/03/22 00:44:57, Sameer Ajmani wrote:
> On 2011/03/21 08:42:37, aclements wrote:
> > Nit: Use #'gofmt-before-save.
> 
> Done.  What's the hash for?

It marks it as a function symbol, as opposed to a generic symbol.  It's a reader
macro for (function gofmt-before-save), which is semantically equivalent to
(quote gofmt-before-save), but helps the bytecode generator (and I think
produces better warnings when compiled).  Probably none of this matters for
something in .emacs, but it's still good practice.

http://codereview.appspot.com/4276059/diff/3/misc/emacs/go-mode.el#newcode554
misc/emacs/go-mode.el:554: (when (eq major-mode 'go-mode) (gofmt)))
On 2011/03/22 00:44:57, Sameer Ajmani wrote:
> On 2011/03/21 08:42:37, aclements wrote:
> > Just a thought...  Since this is now mostly a rewrite of gofmt, would it be
> > better as two separate patches?  Certainly the commit message needs
updating,
> at
> > least.
> 
> gofmt-before-save is trivial enough that I'd like to keep this as one patch.

Fair enough.  You should still update the commit message.

[Sameer (personal), 2011-03-22 01:23:54]

http://codereview.appspot.com/4276059/diff/3/misc/emacs/go-mode.el
File misc/emacs/go-mode.el (right):

http://codereview.appspot.com/4276059/diff/3/misc/emacs/go-mode.el#newcode523
misc/emacs/go-mode.el:523: (old-point (point))
On 2011/03/22 01:12:49, aclements wrote:
> On 2011/03/22 00:44:57, Sameer Ajmani wrote:
> > On 2011/03/21 08:42:37, aclements wrote:
> > > I believe you no longer have to save mark and point, since
> > > shell-command-on-region is going to a different buffer instead of
replacing
> > the
> > > region.
> > 
> > Yes, but I still need to restore the point and mark when I replace the
current
> > buffer contents (when gofmt succeeds).
> 
> True.  Good to see you updated the comment, though.
> 
> It may be worth considering insert-file-contents to update the buffer (with
the
> replace argument set to t).  That tries really hard to keep point on the same
> text it started on, even if things move around it, but has the obvious
> disadvantage that the gofmt output has to go to a temporary file instead of
just
> a buffer.  Just something to think about.

I think what we have now is simpler, but this is useful to know.

(This is my first elisp outside my own .emacs, so I'm learning a I go.  Thanks
for the pointers.)

http://codereview.appspot.com/4276059/diff/3/misc/emacs/go-mode.el#newcode554
misc/emacs/go-mode.el:554: (when (eq major-mode 'go-mode) (gofmt)))
On 2011/03/22 01:12:49, aclements wrote:
> On 2011/03/22 00:44:57, Sameer Ajmani wrote:
> > On 2011/03/21 08:42:37, aclements wrote:
> > > Just a thought...  Since this is now mostly a rewrite of gofmt, would it
be
> > > better as two separate patches?  Certainly the commit message needs
> updating,
> > at
> > > least.
> > 
> > gofmt-before-save is trivial enough that I'd like to keep this as one patch.
> 
> Fair enough.  You should still update the commit message.

Whoops, forgot.  Updated.

[Sameer (personal), 2011-03-22 01:33:18]

Please ignore the bazillion patch sets.  Trying unsuccessfully to get hg submit
to work.

On 2011/03/22 01:23:54, Sameer Ajmani wrote:
> http://codereview.appspot.com/4276059/diff/3/misc/emacs/go-mode.el
> File misc/emacs/go-mode.el (right):
> 
> http://codereview.appspot.com/4276059/diff/3/misc/emacs/go-mode.el#newcode523
> misc/emacs/go-mode.el:523: (old-point (point))
> On 2011/03/22 01:12:49, aclements wrote:
> > On 2011/03/22 00:44:57, Sameer Ajmani wrote:
> > > On 2011/03/21 08:42:37, aclements wrote:
> > > > I believe you no longer have to save mark and point, since
> > > > shell-command-on-region is going to a different buffer instead of
> replacing
> > > the
> > > > region.
> > > 
> > > Yes, but I still need to restore the point and mark when I replace the
> current
> > > buffer contents (when gofmt succeeds).
> > 
> > True.  Good to see you updated the comment, though.
> > 
> > It may be worth considering insert-file-contents to update the buffer (with
> the
> > replace argument set to t).  That tries really hard to keep point on the
same
> > text it started on, even if things move around it, but has the obvious
> > disadvantage that the gofmt output has to go to a temporary file instead of
> just
> > a buffer.  Just something to think about.
> 
> I think what we have now is simpler, but this is useful to know.
> 
> (This is my first elisp outside my own .emacs, so I'm learning a I go.  Thanks
> for the pointers.)
> 
> http://codereview.appspot.com/4276059/diff/3/misc/emacs/go-mode.el#newcode554
> misc/emacs/go-mode.el:554: (when (eq major-mode 'go-mode) (gofmt)))
> On 2011/03/22 01:12:49, aclements wrote:
> > On 2011/03/22 00:44:57, Sameer Ajmani wrote:
> > > On 2011/03/21 08:42:37, aclements wrote:
> > > > Just a thought...  Since this is now mostly a rewrite of gofmt, would it
> be
> > > > better as two separate patches?  Certainly the commit message needs
> > updating,
> > > at
> > > > least.
> > > 
> > > gofmt-before-save is trivial enough that I'd like to keep this as one
patch.
> > 
> > Fair enough.  You should still update the commit message.
> 
> Whoops, forgot.  Updated.

[Sameer (personal), 2011-03-22 01:44:00]

Ah, I must not be a committer.  Russ, will you commit this CL?

I added my CONTRIBUTORS entry to this CL.

S

On Mon, Mar 21, 2011 at 9:33 PM, <ajmani@gmail.com> wrote:

> Please ignore the bazillion patch sets.  Trying unsuccessfully to get hg
> submit to work.
>
>
> On 2011/03/22 01:23:54, Sameer Ajmani wrote:
>
>> http://codereview.appspot.com/4276059/diff/3/misc/emacs/go-mode.el
>> File misc/emacs/go-mode.el (right):
>>
>
>
>
> http://codereview.appspot.com/4276059/diff/3/misc/emacs/go-mode.el#newcode523
>
>> misc/emacs/go-mode.el:523: (old-point (point))
>> On 2011/03/22 01:12:49, aclements wrote:
>> > On 2011/03/22 00:44:57, Sameer Ajmani wrote:
>> > > On 2011/03/21 08:42:37, aclements wrote:
>> > > > I believe you no longer have to save mark and point, since
>> > > > shell-command-on-region is going to a different buffer instead
>>
> of
>
>> replacing
>> > > the
>> > > > region.
>> > >
>> > > Yes, but I still need to restore the point and mark when I replace
>>
> the
>
>> current
>> > > buffer contents (when gofmt succeeds).
>> >
>> > True.  Good to see you updated the comment, though.
>> >
>> > It may be worth considering insert-file-contents to update the
>>
> buffer (with
>
>> the
>> > replace argument set to t).  That tries really hard to keep point on
>>
> the same
>
>> > text it started on, even if things move around it, but has the
>>
> obvious
>
>> > disadvantage that the gofmt output has to go to a temporary file
>>
> instead of
>
>> just
>> > a buffer.  Just something to think about.
>>
>
>  I think what we have now is simpler, but this is useful to know.
>>
>
>  (This is my first elisp outside my own .emacs, so I'm learning a I go.
>>
>  Thanks
>
>> for the pointers.)
>>
>
>
>
> http://codereview.appspot.com/4276059/diff/3/misc/emacs/go-mode.el#newcode554
>
>> misc/emacs/go-mode.el:554: (when (eq major-mode 'go-mode) (gofmt)))
>> On 2011/03/22 01:12:49, aclements wrote:
>> > On 2011/03/22 00:44:57, Sameer Ajmani wrote:
>> > > On 2011/03/21 08:42:37, aclements wrote:
>> > > > Just a thought...  Since this is now mostly a rewrite of gofmt,
>>
> would it
>
>> be
>> > > > better as two separate patches?  Certainly the commit message
>>
> needs
>
>> > updating,
>> > > at
>> > > > least.
>> > >
>> > > gofmt-before-save is trivial enough that I'd like to keep this as
>>
> one patch.
>
>> >
>> > Fair enough.  You should still update the commit message.
>>
>
>  Whoops, forgot.  Updated.
>>
>
>
>
> http://codereview.appspot.com/4276059/
>



-- 
Sameer
http://www.google.com/profiles/ajmani

[rsc, 2011-03-22 02:58:01]

yes, i will submit it.

[rsc, 2011-03-23 17:45:24]

please hg revert CONTRIBUTORS;  hg mail 4276059

[Sameer (personal), 2011-03-23 23:42:45]

Hello rsc, aclements, amdragon@mit.edu (cc: golang-dev@googlegroups.com),

Please take another look.

[aclements, 2011-03-24 01:22:24]

On 2011/03/23 23:42:45, Sameer Ajmani wrote:
> Hello rsc, aclements, mailto:amdragon@mit.edu (cc:
mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

LGTM

[Sameer (personal), 2011-03-24 14:31:20]

Done.

On Wed, Mar 23, 2011 at 1:45 PM, Russ Cox <rsc@golang.org> wrote:

> please hg revert CONTRIBUTORS;  hg mail 4276059
>



-- 
Sameer
http://www.google.com/profiles/ajmani

[rsc, 2011-03-24 14:35:44]

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

misc/emacs: gofmt: don't clobber the current buffer on failure

Change M-x gofmt to display errors in a new buffer instead of
clobbering the current buffer.

Add gofmt-before-save, which runs gofmt when in go-mode.  This
can be used with before-save-hook.  Add to your .emacs:
  (add-hook 'before-save-hook 'gofmt-before-save)

R=rsc, aclements, amdragon
CC=golang-dev
http://codereview.appspot.com/4276059

Committer: Russ Cox <rsc@golang.org>