git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Lawrence Mitchell <wence@gmx.li>
To: git@vger.kernel.org
Cc: "Rüdiger Sonderfeld" <ruediger@c-plusplus.de>,
	davidk@lysator.liu.se, user42@zip.com.au, osv@javad.com
Subject: Re: [PATCH 1/3] git-blame.el: Do not use goto-line in lisp code
Date: Thu, 14 Jun 2012 10:14:36 +0100	[thread overview]
Message-ID: <87k3za9rwj.fsf@gmx.li> (raw)
In-Reply-To: 20120614050854.GG27586@burratino

Jonathan Nieder wrote:
> Hi Lawrence,

> Lawrence Mitchell wrote:

>> From: Rüdiger Sonderfeld <ruediger@c-plusplus.de>

>> goto-line is a user-level command, instead use the lisp-level
>> construct recommended in Emacs documentation.
> [...]
>> Here we go, all Rüdiger's changes look sensible, I've split them into bits though

> Thanks for looking them over.

> Would you mind indulging my curiosity a little by describing what bad
> behavior or potential bad behavior this change prevents?


goto-line sets the mark, and respects the variable
selective-display.  It also widens the buffer before moving to
the relevant line.  The first two are almost never what you'd
want in lisp code, the latter you'd probably want to make
explicit in the calls I guess.

the with-current-buffer issue is a bit more subtle, and I realise
my patch for this didn't actually fix the bug, or describe the
problem properly (reroll to come).

Basically:

save-excursion saves point, mark and current-buffer in the buffer
in scope when it is called, but if we do:

(save-excursion
  (set-buffer buf)
  ;; modify point and mark in buf
  ...)

hoping to save point and mark in buf, it doesn't happen.
Instead, we need to make buf current before calling
save-excursion.  And we want to restore the value of
current-buffer in scope at the beginning of the call afterward,
hence the correct idiom is:

(with-current-buffer buf
  (save-excursion ...))

Cheers,

Lawrence

-- 
Lawrence Mitchell <wence@gmx.li>

  reply	other threads:[~2012-06-14  9:15 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-01-12 15:44 [PATCH] git-blame.el: Fix compilation warnings Rüdiger Sonderfeld
2012-01-12 16:26 ` Jonathan Nieder
2012-01-12 17:08   ` Rüdiger Sonderfeld
2012-01-13 23:31     ` Sending patches with KMail (Re: [PATCH] git-blame.el: Fix compilation warnings.) Jonathan Nieder
2012-01-14  0:59       ` Junio C Hamano
2012-01-14 18:31         ` Sending patches with KMail Jonathan Nieder
2012-01-14 18:34           ` Jonathan Nieder
2012-01-15  2:14           ` Junio C Hamano
2012-01-14 19:18         ` Sending patches with KMail (Re: [PATCH] git-blame.el: Fix compilation warnings.) Rüdiger Sonderfeld
2012-06-10  7:38 ` [PATCH] git-blame.el: use mapc instead of mapcar Jonathan Nieder
2012-06-10 11:58   ` [PATCH 1/3] git-blame.el: Do not use goto-line in lisp code Lawrence Mitchell
2012-06-10 11:58     ` [PATCH 2/3] git-blame.el: Use with-current-buffer where appropriate Lawrence Mitchell
2012-06-10 11:58       ` [PATCH 3/3] git-blame.el: Do not use bare 0 to mean (point-min) Lawrence Mitchell
2012-06-14  5:08     ` [PATCH 1/3] git-blame.el: Do not use goto-line in lisp code Jonathan Nieder
2012-06-14  9:14       ` Lawrence Mitchell [this message]
2012-06-14  9:37         ` [PATCH v2 " Lawrence Mitchell
2012-06-14  9:37           ` [PATCH v2 2/3] git-blame.el: Use with-current-buffer where appropriate Lawrence Mitchell
2012-06-14  9:38             ` [PATCH v2 3/3] git-blame.el: Do not use bare 0 to mean (point-min) Lawrence Mitchell

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87k3za9rwj.fsf@gmx.li \
    --to=wence@gmx.li \
    --cc=davidk@lysator.liu.se \
    --cc=git@vger.kernel.org \
    --cc=osv@javad.com \
    --cc=ruediger@c-plusplus.de \
    --cc=user42@zip.com.au \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).