[PATCH] git-blame.el: Fix compilation warnings.

[PATCH] git-blame.el: Fix compilation warnings.

[Rüdiger Sonderfeld]

From 4958c1b43d7a66654e15c92cbb878b38533d626e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=BCdiger=20Sonderfeld?= <ruediger@c-plusplus.de>
Date: Thu, 12 Jan 2012 16:37:06 +0100
Subject: [PATCH] git-blame.el: Fix compilation warnings.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Replace mapcar with mapc because accumulation of the results was not
needed. (git-blame-cleanup)

Replace two occurrences of (save-excursion (set-buffer buf) ...)
with (with-current-buffer buf ...). (git-blame-filter and
git-blame-create-overlay)

Replace goto-line with (goto-char (point-min)) (forward-line (1-
start-line)). According to the documentation of goto-line it should
not be called from elisp code. (git-blame-create-overlay)

Signed-off-by: Rüdiger Sonderfeld <ruediger@c-plusplus.de>
---
 contrib/emacs/git-blame.el |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/contrib/emacs/git-blame.el b/contrib/emacs/git-blame.el
index d351cfb..2e53fc6 100644
--- a/contrib/emacs/git-blame.el
+++ b/contrib/emacs/git-blame.el
@@ -304,7 +304,7 @@ See also function `git-blame-mode'."
 
 (defun git-blame-cleanup ()
   "Remove all blame properties"
-    (mapcar 'delete-overlay git-blame-overlays)
+    (mapc 'delete-overlay git-blame-overlays)
     (setq git-blame-overlays nil)
     (remove-git-blame-text-properties (point-min) (point-max)))
 
@@ -337,8 +337,7 @@ See also function `git-blame-mode'."
 (defvar in-blame-filter nil)
 
 (defun git-blame-filter (proc str)
-  (save-excursion
-    (set-buffer (process-buffer proc))
+  (with-current-buffer (process-buffer proc)
     (goto-char (process-mark proc))
     (insert-before-markers str)
     (goto-char 0)
@@ -385,11 +384,10 @@ See also function `git-blame-mode'."
           info))))
 
 (defun git-blame-create-overlay (info start-line num-lines)
-  (save-excursion
-    (set-buffer git-blame-file)
+  (with-current-buffer git-blame-file
     (let ((inhibit-point-motion-hooks t)
           (inhibit-modification-hooks t))
-      (goto-line start-line)
+      (goto-char (point-min)) (forward-line (1- start-line))
       (let* ((start (point))
              (end (progn (forward-line num-lines) (point)))
              (ovl (make-overlay start end))
-- 
1.7.8.3

Re: [PATCH] git-blame.el: Fix compilation warnings.

[Jonathan Nieder]

(+cc: Sergei, Kevin)
Hi,

Rüdiger Sonderfeld wrote:

> From 4958c1b43d7a66654e15c92cbb878b38533d626e Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?R=C3=BCdiger=20Sonderfeld?= <ruediger@c-plusplus.de>
[...]

These lines should be left out [*].

> Replace mapcar with mapc because accumulation of the results was not
> needed. (git-blame-cleanup)
>
> Replace two occurrences of (save-excursion (set-buffer buf) ...)
> with (with-current-buffer buf ...). (git-blame-filter and
> git-blame-create-overlay)
>
> Replace goto-line with (goto-char (point-min)) (forward-line (1-
> start-line)). According to the documentation of goto-line it should
> not be called from elisp code. (git-blame-create-overlay)
>
> Signed-off-by: Rüdiger Sonderfeld <ruediger@c-plusplus.de>

I assume this was prompted by warning messages like this one:

	In git-blame-cleanup:
	git-blame.el:306:6:Warning: `mapcar' called for effect; use `mapc' or `dolist' instead

Looks reasonable to my very much untrained eyes, and it's consistent
with the hints Kevin gave at [1].

Thanks,
Jonathan

[1] http://bugs.debian.org/cgi-bin/bugreport.cgi?msg=63;bug=611931
[*] The "From " line and following lines are for your mailer and can
be omited unless they differ from the mail header when reading your
patch into an email body.  See the DISCUSSION sections of
git-format-patch(1) and git-am(1) for more on this.

(patch left unsnipped for Sergei and Kevin's convenience)

> ---
>  contrib/emacs/git-blame.el |   10 ++++------
>  1 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/contrib/emacs/git-blame.el b/contrib/emacs/git-blame.el
> index d351cfb..2e53fc6 100644
> --- a/contrib/emacs/git-blame.el
> +++ b/contrib/emacs/git-blame.el
> @@ -304,7 +304,7 @@ See also function `git-blame-mode'."
>  
>  (defun git-blame-cleanup ()
>    "Remove all blame properties"
> -    (mapcar 'delete-overlay git-blame-overlays)
> +    (mapc 'delete-overlay git-blame-overlays)
>      (setq git-blame-overlays nil)
>      (remove-git-blame-text-properties (point-min) (point-max)))
>  
> @@ -337,8 +337,7 @@ See also function `git-blame-mode'."
>  (defvar in-blame-filter nil)
>  
>  (defun git-blame-filter (proc str)
> -  (save-excursion
> -    (set-buffer (process-buffer proc))
> +  (with-current-buffer (process-buffer proc)
>      (goto-char (process-mark proc))
>      (insert-before-markers str)
>      (goto-char 0)
> @@ -385,11 +384,10 @@ See also function `git-blame-mode'."
>            info))))
>  
>  (defun git-blame-create-overlay (info start-line num-lines)
> -  (save-excursion
> -    (set-buffer git-blame-file)
> +  (with-current-buffer git-blame-file
>      (let ((inhibit-point-motion-hooks t)
>            (inhibit-modification-hooks t))
> -      (goto-line start-line)
> +      (goto-char (point-min)) (forward-line (1- start-line))
>        (let* ((start (point))
>               (end (progn (forward-line num-lines) (point)))
>               (ovl (make-overlay start end))

Re: [PATCH] git-blame.el: Fix compilation warnings.

[Rüdiger Sonderfeld]

Hi,

On Thursday 12 January 2012 10:26:41 Jonathan Nieder wrote:
> These lines should be left out [*].

Sorry, I wasn't sure whether to remove them or not. I followed the description 
in git-format-patch(1) on how to send patches with kmail. I'll remove them in 
the future. Thanks for the advice.
 
> I assume this was prompted by warning messages like this one:
> 
> 	In git-blame-cleanup:
> 	git-blame.el:306:6:Warning: `mapcar' called for effect; use `mapc' or
> `dolist' instead
> 
> Looks reasonable to my very much untrained eyes, and it's consistent
> with the hints Kevin gave at [1].

Yes. I think the warnings are correct and should be addressed. E.g. Using 
mapcar compared to mapc is slower due to the required accumulation of the 
results and the additional garbage collection costs. It's not very dramatic 
but there is no reason not to fix it imho.

Regards,
Rüdiger

Sending patches with KMail (Re: [PATCH] git-blame.el: Fix compilation warnings.)

[Jonathan Nieder]

Hi,

Rüdiger Sonderfeld wrote:
> On Thursday 12 January 2012 10:26:41 Jonathan Nieder wrote:

>> These lines should be left out [*].
>
> Sorry, I wasn't sure whether to remove them or not. I followed the description 
> in git-format-patch(1) on how to send patches with kmail. I'll remove them in 
> the future. Thanks for the advice.

Oh, thanks for the pointer.  How about something like this?

The hints at [1] might also be useful, in case you would like to try
and consider improving the manpage to document them if they work.

-- >8 --
Subject: Documentation/format-patch: mention removal of in-body headers for KMail

The opening "From " line and following lines in "git format-patch"

	From 13c41b41b832d41680ccd33a2422ef8217965566 Mon Sep 17 00:00:00 2001
	From: Jonathan Nieder <jrnieder@gmail.com>
	Date: Fri, 13 Jan 2012 17:22:41 -0600

are for your mailer and should be omitted except for fields that
differ from the mail header when reading your patch into an email
body.  Otherwise "git am" thinks these lines are part of the commit
message  when trying to reproduce the resulting patch from an mbox
automatically.  Add a reminder in this direction to the KMail recipe.

Suggested-by: Rüdiger Sonderfeld <ruediger@c-plusplus.de>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
[1] http://thread.gmane.org/gmane.comp.version-control.git/171580/focus=171720

 Documentation/git-format-patch.txt |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 6ea9be77..5e1d6d2c 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -462,8 +462,10 @@ This should help you to submit patches inline using KMail.
 
 4. Use Message -> Insert file... and insert the patch.
 
-5. Back in the compose window: add whatever other text you wish to the
-   message, complete the addressing and subject fields, and press send.
+5. Back in the compose window: remove the "`From $SHA1 $magic_timestamp`"
+   marker and unwanted in-body headers, add whatever other text you wish
+   to the message, complete the addressing and subject fields, and
+   press send.
 
 
 EXAMPLES
-- 
1.7.8.3

Re: Sending patches with KMail (Re: [PATCH] git-blame.el: Fix compilation warnings.)

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> The hints at [1] might also be useful, in case you would like to try
> and consider improving the manpage to document them if they work.

Don't you need similar updates to sections for other MUAs and procedures?

I suspect that the reason why you added the new text there is because you
know KMail users are very lazy bunch, and once they see a "KMail"
subsection, they will skip everything outside the subsection. Thunderbird
users would also be lazy---after choosing one of the three approaches
presented, they will skip anything outside the subsubsection. So I can
understand that we would need something in these individual subsections,
but the advice does not logically belong there.

Perhaps rephrasing the early part of the Discussion section, with an
illustration that is designed to be more visible, would be a better
approach?

For example, we could take your log message and stuff it there:

    The opening "From " line and following lines in "git format-patch" are
    for your mailer and should be omitted except for fields that differ from
    the mail header when reading your patch into an email body. For example,
    the output of your format-patch may begin like this:

          From 13c41b41b832d41680ccd33a2422ef8217965566 Mon Sep 17 00:00:00 2001
          From: Jonathan Nieder <jrnieder@gmail.com>
          Date: Fri, 13 Jan 2012 17:22:41 -0600
          Subject: Documentation/format-patch: mention removal of in-body headers

          The opening "From " line and following lines in ...

    The part you should send in the body of your e-mail message begins at the
    first blank line. The "From $SHA1 $magic_timestamp" line and other header
    lines are there to make it look like a mbox, and if you send it in e-mail,
    they will become redundant.

    You can leave "From:" and/or "Subject:" lines in, if they are
    different from the e-mail you will be sending out (e.g. you are
    forwarding a patch written by somebody else, as a follow up to an
    ongoing discussion and do not want the subject of your e-mail message
    to help threading).  E.g. your message _may_ begin like this:

          From: Jonathan Nieder <jrnieder@gmail.com>
          Subject: Documentation/format-patch: mention removal of in-body headers

          The opening "From " line and following lines in ...

    when you are not Jonathan, and you are sending it as a response to
    an existing discussion thread.

Or something like that?

Re: Sending patches with KMail

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> The hints at [1] might also be useful, in case you would like to try
>> and consider improving the manpage to document them if they work.
>
> Don't you need similar updates to sections for other MUAs and procedures?

Thunderbird approach 3, yes[*].  The others, no.

[...]
> Perhaps rephrasing the early part of the Discussion section, with an
> illustration that is designed to be more visible, would be a better
> approach?

I understand what you mean, but I don't think so.  The Discussion
section already seems clear to me, so I would prefer to wait to hear
from someone confused by it to find what exactly in it needs tweaking.
Adding additional paragraphs for each potential misunderstanding by
people who have not necessarily read the section has the potential to
backfire and lead even more people not to read the section...

My favorite approach would be to introduce a new option
--format=plain|mbox, with the default being mbox, allowing
format-patch --format=plain to produce a nice patch that does _not_
include a "From " line or q-encode its header lines, ready for use
without much tweaking in an email body as an attachment.  Then we can
just say "If you are not importing your patch as an mbox file, use the
--format=plain option".

Sane?

Jonathan

[*] Though I'd rather just remove it, since "how to use an external
editor" seems orthogonal to "how to teach Thunderbird not to mangle my
patches".

Re: Sending patches with KMail

[Jonathan Nieder]

Jonathan Nieder wrote:

> My favorite approach would be to introduce a new option
> --format=plain|mbox, with the default being mbox, allowing
> format-patch --format=plain to produce a nice patch that does _not_
> include a "From " line or q-encode its header lines, ready for use
> without much tweaking in an email body as an attachment.

This should have said "ready for use in an email body or as an
attachment" (missing "or").  Sorry for the confusion.

Re: Sending patches with KMail

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> My favorite approach would be to introduce a new option
> --format=plain|mbox, with the default being mbox, allowing format-patch
> --format=plain to produce a nice patch that does _not_ include a "From "
> line or q-encode its header lines, ready for use without much tweaking
> in an email body as an attachment.

I actually like the removal of q-encoding part. But I am not sure what
headers it should produce.  What should the beginning of the output file
look like? Does it just have "Subject: ", or does it still have the "From:
", "Date: " and "Subject: ", the first two of which the user would almost
always want to remove?

If we can decide a sane behaviour wrt the pseudo header, and if the option
is made _incompatible_ with --stdout when (and only when) emitting more
than one message, then I think it would be a good addition.

Re: Sending patches with KMail (Re: [PATCH] git-blame.el: Fix compilation warnings.)

[Rüdiger Sonderfeld]

On Friday 13 January 2012 16:59:49 Junio C Hamano wrote:
> Perhaps rephrasing the early part of the Discussion section, with an
> illustration that is designed to be more visible, would be a better
> approach?

Why not do both?

I think you are right, that it is currently not very visible in the Discussion 
section. But on the other hand if you have a step by step guide it should 
probably mention that as well. It has nothing to do with laziness. But most 
people follow a step by step guide because they expect that it illustrates the 
correct procedure. Jonathan's addition is short but effective.

Regards,
Rüdiger

[PATCH] git-blame.el: use mapc instead of mapcar

[Jonathan Nieder]

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

Using mapcar here is a waste of memory because the mapped result
is not used.

Noticed by emacs ("Warning: `mapcar' called for effect").

[jn: split from a larger patch, with new description]

Signed-off-by: Rüdiger Sonderfeld <ruediger@c-plusplus.de>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
In January, Rüdiger Sonderfeld wrote:

> Replace mapcar with mapc because accumulation of the results was not
> needed. (git-blame-cleanup)
>
> Replace two occurrences of (save-excursion (set-buffer buf) ...)
> with (with-current-buffer buf ...). (git-blame-filter and
> git-blame-create-overlay)
>
> Replace goto-line with (goto-char (point-min)) (forward-line (1-
> start-line)). According to the documentation of goto-line it should
> not be called from elisp code. (git-blame-create-overlay)
>
> Signed-off-by: Rüdiger Sonderfeld <ruediger@c-plusplus.de>
> ---
>  contrib/emacs/git-blame.el |   10 ++++------
>  1 files changed, 4 insertions(+), 6 deletions(-)

Thanks again, and sorry for the long silence.

I'd prefer to see someone more knowledgeable than I am about elisp
submit the other two fixes.  This one is simple enough that I can
vouch for it, though.  One out of three is not that bad, I guess. :)

Thoughts?
Jonathan

 contrib/emacs/git-blame.el |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/emacs/git-blame.el b/contrib/emacs/git-blame.el
index d351cfb6..37d797e1 100644
--- a/contrib/emacs/git-blame.el
+++ b/contrib/emacs/git-blame.el
@@ -304,7 +304,7 @@ See also function `git-blame-mode'."
 
 (defun git-blame-cleanup ()
   "Remove all blame properties"
-    (mapcar 'delete-overlay git-blame-overlays)
+    (mapc 'delete-overlay git-blame-overlays)
     (setq git-blame-overlays nil)
     (remove-git-blame-text-properties (point-min) (point-max)))
 
-- 
1.7.10

[PATCH 1/3] git-blame.el: Do not use goto-line in lisp code

[Lawrence Mitchell]

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.

Signed-off-by: Rüdiger Sonderfeld <ruediger@c-plusplus.de>
Signed-off-by: Lawrence Mitchell <wence@gmx.li>
---
 contrib/emacs/git-blame.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Here we go, all Rüdiger's changes look sensible, I've split them into bits though

diff --git a/contrib/emacs/git-blame.el b/contrib/emacs/git-blame.el
index 37d797e..5428ff7 100644
--- a/contrib/emacs/git-blame.el
+++ b/contrib/emacs/git-blame.el
@@ -389,7 +389,8 @@ See also function `git-blame-mode'."
     (set-buffer git-blame-file)
     (let ((inhibit-point-motion-hooks t)
           (inhibit-modification-hooks t))
-      (goto-line start-line)
+      (goto-char (point-min))
+      (forward-line (1- start-line))
       (let* ((start (point))
              (end (progn (forward-line num-lines) (point)))
              (ovl (make-overlay start end))
-- 
1.7.10.2.552.gaa3bb87

[PATCH 2/3] git-blame.el: Use with-current-buffer where appropriate

[Lawrence Mitchell]

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

In git-blame-filter and git-blame-new-commit we need to execute the
body with (current-buffer) bound to the correct output buffer.  We
then want to restore the previous value of (current-buffer).  The
idiom

   (save-excursion
     (set-buffer buf)
     ...)

will not correctly save the original buffer the code was executed in.
Instead, use with-current-buffer as recommended in Emacs documentation.

Signed-off-by: Rüdiger Sonderfeld <ruediger@c-plusplus.de>
Signed-off-by: Lawrence Mitchell <wence@gmx.li>
---
 contrib/emacs/git-blame.el | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/contrib/emacs/git-blame.el b/contrib/emacs/git-blame.el
index 5428ff7..20cf9a6 100644
--- a/contrib/emacs/git-blame.el
+++ b/contrib/emacs/git-blame.el
@@ -337,8 +337,7 @@ See also function `git-blame-mode'."
 (defvar in-blame-filter nil)
 
 (defun git-blame-filter (proc str)
-  (save-excursion
-    (set-buffer (process-buffer proc))
+  (with-current-buffer (process-buffer proc)
     (goto-char (process-mark proc))
     (insert-before-markers str)
     (goto-char 0)
@@ -385,8 +384,7 @@ See also function `git-blame-mode'."
           info))))
 
 (defun git-blame-create-overlay (info start-line num-lines)
-  (save-excursion
-    (set-buffer git-blame-file)
+  (with-current-buffer git-blame-file
     (let ((inhibit-point-motion-hooks t)
           (inhibit-modification-hooks t))
       (goto-char (point-min))
-- 
1.7.10.2.552.gaa3bb87

[PATCH 3/3] git-blame.el: Do not use bare 0 to mean (point-min)

[Lawrence Mitchell]

Signed-off-by: Lawrence Mitchell <wence@gmx.li>
---
 contrib/emacs/git-blame.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

A small cleanup I noticed while glancing at the code

diff --git a/contrib/emacs/git-blame.el b/contrib/emacs/git-blame.el
index 20cf9a6..ef1eebd 100644
--- a/contrib/emacs/git-blame.el
+++ b/contrib/emacs/git-blame.el
@@ -340,7 +340,7 @@ See also function `git-blame-mode'."
   (with-current-buffer (process-buffer proc)
     (goto-char (process-mark proc))
     (insert-before-markers str)
-    (goto-char 0)
+    (goto-char (point-min))
     (unless in-blame-filter
       (let ((more t)
             (in-blame-filter t))
-- 
1.7.10.2.552.gaa3bb87

Re: [PATCH 1/3] git-blame.el: Do not use goto-line in lisp code

[Jonathan Nieder]

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?

Even without that information, I'm all for applying this patch, since
it seems to be what all the people who know elisp recommend. :)

Regards,
Jonathan

(patch kept unsnipped for convenience)
> diff --git a/contrib/emacs/git-blame.el b/contrib/emacs/git-blame.el
> index 37d797e..5428ff7 100644
> --- a/contrib/emacs/git-blame.el
> +++ b/contrib/emacs/git-blame.el
> @@ -389,7 +389,8 @@ See also function `git-blame-mode'."
>      (set-buffer git-blame-file)
>      (let ((inhibit-point-motion-hooks t)
>            (inhibit-modification-hooks t))
> -      (goto-line start-line)
> +      (goto-char (point-min))
> +      (forward-line (1- start-line))
>        (let* ((start (point))
>               (end (progn (forward-line num-lines) (point)))
>               (ovl (make-overlay start end))

Re: [PATCH 1/3] git-blame.el: Do not use goto-line in lisp code

[Lawrence Mitchell]

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>

[PATCH v2 1/3] git-blame.el: Do not use goto-line in lisp code

[Lawrence Mitchell]

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.

Signed-off-by: Rüdiger Sonderfeld <ruediger@c-plusplus.de>
Signed-off-by: Lawrence Mitchell <wence@gmx.li>
---
 contrib/emacs/git-blame.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

No change from v1

diff --git a/contrib/emacs/git-blame.el b/contrib/emacs/git-blame.el
index 37d797e..5428ff7 100644
--- a/contrib/emacs/git-blame.el
+++ b/contrib/emacs/git-blame.el
@@ -389,7 +389,8 @@ See also function `git-blame-mode'."
     (set-buffer git-blame-file)
     (let ((inhibit-point-motion-hooks t)
           (inhibit-modification-hooks t))
-      (goto-line start-line)
+      (goto-char (point-min))
+      (forward-line (1- start-line))
       (let* ((start (point))
              (end (progn (forward-line num-lines) (point)))
              (ovl (make-overlay start end))
-- 
1.7.11.rc2.9.g10afb6c

[PATCH v2 2/3] git-blame.el: Use with-current-buffer where appropriate

[Lawrence Mitchell]

In git-blame-filter and git-blame-create-overlay we want to save
(along with the values of point and mark) the current-buffer in scope
when calling the functions.  The idiom

    (save-excursion
      (set-buffer buf)
      ...)

will correctly restore the correct buffer, but will not save the
values of point and mark in buf (only in the buffer current when the
save-excursion call is executed).  The intention of these functions is
to save the current buffer from the calling scope and the values of
point and mark in the buffer they are modifying.  The correct idiom
for this is

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

Signed-off-by: Rüdiger Sonderfeld <ruediger@c-plusplus.de>
Signed-off-by: Lawrence Mitchell <wence@gmx.li>
---
 contrib/emacs/git-blame.el | 74 +++++++++++++++++++++++-----------------------
 1 file changed, 37 insertions(+), 37 deletions(-)

Updated commit message that actually correctly matches what Emacs
does, plus don't just squash the byte-compiler warnings but actually
fix the bug that it was pointing out to us.

For reference, here's the whitespace-squashed change:

| @@ -337,8 +337,8 @@ See also function `git-blame-mode'."
|  (defvar in-blame-filter nil)
 
|  (defun git-blame-filter (proc str)
| +  (with-current-buffer (process-buffer proc)
|      (save-excursion
| -    (set-buffer (process-buffer proc))
|        (goto-char (process-mark proc))
|        (insert-before-markers str)
|        (goto-char 0)
| @@ -346,7 +346,7 @@ See also function `git-blame-mode'."
|          (let ((more t)
|                (in-blame-filter t))
|            (while more
| -          (setq more (git-blame-parse)))))))
| +            (setq more (git-blame-parse))))))))
 
|  (defun git-blame-parse ()
|    (cond ((looking-at "\\([0-9a-f]\\{40\\}\\) \\([0-9]+\\) \\([0-9]+\\) \\([0-9]+\\)\n")
| @@ -385,8 +385,8 @@ See also function `git-blame-mode'."
|            info))))
 
|  (defun git-blame-create-overlay (info start-line num-lines)
| +  (with-current-buffer git-blame-file
|      (save-excursion
| -    (set-buffer git-blame-file)
|        (let ((inhibit-point-motion-hooks t)
|              (inhibit-modification-hooks t))
|          (goto-char (point-min))
| @@ -411,7 +411,7 @@ See also function `git-blame-mode'."
|                                             (cdr (assq 'color (cdr info))))))
|            (overlay-put ovl 'line-prefix
|                         (propertize (format-spec git-blame-prefix-format spec)
| -                                 'face 'git-blame-prefix-face))))))
| +                                   'face 'git-blame-prefix-face)))))))
 


diff --git a/contrib/emacs/git-blame.el b/contrib/emacs/git-blame.el
index 5428ff7..bb6d7bb 100644
--- a/contrib/emacs/git-blame.el
+++ b/contrib/emacs/git-blame.el
@@ -337,16 +337,16 @@ See also function `git-blame-mode'."
 (defvar in-blame-filter nil)
 
 (defun git-blame-filter (proc str)
-  (save-excursion
-    (set-buffer (process-buffer proc))
-    (goto-char (process-mark proc))
-    (insert-before-markers str)
-    (goto-char 0)
-    (unless in-blame-filter
-      (let ((more t)
-            (in-blame-filter t))
-        (while more
-          (setq more (git-blame-parse)))))))
+  (with-current-buffer (process-buffer proc)
+    (save-excursion
+      (goto-char (process-mark proc))
+      (insert-before-markers str)
+      (goto-char 0)
+      (unless in-blame-filter
+        (let ((more t)
+              (in-blame-filter t))
+          (while more
+            (setq more (git-blame-parse))))))))
 
 (defun git-blame-parse ()
   (cond ((looking-at "\\([0-9a-f]\\{40\\}\\) \\([0-9]+\\) \\([0-9]+\\) \\([0-9]+\\)\n")
@@ -385,33 +385,33 @@ See also function `git-blame-mode'."
           info))))
 
 (defun git-blame-create-overlay (info start-line num-lines)
-  (save-excursion
-    (set-buffer git-blame-file)
-    (let ((inhibit-point-motion-hooks t)
-          (inhibit-modification-hooks t))
-      (goto-char (point-min))
-      (forward-line (1- start-line))
-      (let* ((start (point))
-             (end (progn (forward-line num-lines) (point)))
-             (ovl (make-overlay start end))
-             (hash (car info))
-             (spec `((?h . ,(substring hash 0 6))
-                     (?H . ,hash)
-                     (?a . ,(git-blame-get-info info 'author))
-                     (?A . ,(git-blame-get-info info 'author-mail))
-                     (?c . ,(git-blame-get-info info 'committer))
-                     (?C . ,(git-blame-get-info info 'committer-mail))
-                     (?s . ,(git-blame-get-info info 'summary)))))
-        (push ovl git-blame-overlays)
-        (overlay-put ovl 'git-blame info)
-        (overlay-put ovl 'help-echo
-                     (format-spec git-blame-mouseover-format spec))
-        (if git-blame-use-colors
-            (overlay-put ovl 'face (list :background
-                                         (cdr (assq 'color (cdr info))))))
-        (overlay-put ovl 'line-prefix
-                     (propertize (format-spec git-blame-prefix-format spec)
-                                 'face 'git-blame-prefix-face))))))
+  (with-current-buffer git-blame-file
+    (save-excursion
+      (let ((inhibit-point-motion-hooks t)
+            (inhibit-modification-hooks t))
+        (goto-char (point-min))
+        (forward-line (1- start-line))
+        (let* ((start (point))
+               (end (progn (forward-line num-lines) (point)))
+               (ovl (make-overlay start end))
+               (hash (car info))
+               (spec `((?h . ,(substring hash 0 6))
+                       (?H . ,hash)
+                       (?a . ,(git-blame-get-info info 'author))
+                       (?A . ,(git-blame-get-info info 'author-mail))
+                       (?c . ,(git-blame-get-info info 'committer))
+                       (?C . ,(git-blame-get-info info 'committer-mail))
+                       (?s . ,(git-blame-get-info info 'summary)))))
+          (push ovl git-blame-overlays)
+          (overlay-put ovl 'git-blame info)
+          (overlay-put ovl 'help-echo
+                       (format-spec git-blame-mouseover-format spec))
+          (if git-blame-use-colors
+              (overlay-put ovl 'face (list :background
+                                           (cdr (assq 'color (cdr info))))))
+          (overlay-put ovl 'line-prefix
+                       (propertize (format-spec git-blame-prefix-format spec)
+                                   'face 'git-blame-prefix-face)))))))
 
 (defun git-blame-add-info (info key value)
   (nconc info (list (cons (intern key) value))))
-- 
1.7.11.rc2.9.g10afb6c

[PATCH v2 3/3] git-blame.el: Do not use bare 0 to mean (point-min)

[Lawrence Mitchell]

Signed-off-by: Lawrence Mitchell <wence@gmx.li>
---
 contrib/emacs/git-blame.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

No change from v1

diff --git a/contrib/emacs/git-blame.el b/contrib/emacs/git-blame.el
index bb6d7bb..e671f6c 100644
--- a/contrib/emacs/git-blame.el
+++ b/contrib/emacs/git-blame.el
@@ -341,7 +341,7 @@ See also function `git-blame-mode'."
     (save-excursion
       (goto-char (process-mark proc))
       (insert-before-markers str)
-      (goto-char 0)
+      (goto-char (point-min))
       (unless in-blame-filter
         (let ((more t)
               (in-blame-filter t))
-- 
1.7.11.rc2.9.g10afb6c