{"thread":{"id":"29354","subject":"[PATCH] git-blame.el: Fix compilation warnings.","startedAt":"2012-01-12T15:44:19Z","lastAt":"2012-06-14T09:38:00Z","messageCount":18,"participants":["Rüdiger Sonderfeld","Jonathan Nieder","Junio C Hamano","Lawrence Mitchell"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"182435","messageId":"2608010.fNV39qBMLu@descartes","threadId":"29354","inReplyTo":null,"subject":"[PATCH] git-blame.el: Fix compilation warnings.","fromName":"Rüdiger Sonderfeld","fromEmail":"ruediger@c-plusplus.de","sentAt":"2012-01-12T15:44:19Z","receivedAt":"2012-01-12T15:44:19Z","isPatch":true,"sender":{"key":"ruediger@c-plusplus.de","avatar":"https://avatars.githubusercontent.com/u/1803?v=4"},"body":"From 4958c1b43d7a66654e15c92cbb878b38533d626e Mon Sep 17 00:00:00 2001\nFrom: =?UTF-8?q?R=C3=BCdiger=20Sonderfeld?= \nDate: Thu, 12 Jan 2012 16:37:06 +0100\nSubject: [PATCH] git-blame.el: Fix compilation warnings.\nMIME-Version: 1.0\nContent-Type: text/plain; charset=UTF-8\nContent-Transfer-Encoding: 8bit\n\nReplace mapcar with mapc because accumulation of the results was not\nneeded. (git-blame-cleanup)\n\nReplace two occurrences of (save-excursion (set-buffer buf) ...)\nwith (with-current-buffer buf ...). (git-blame-filter and\ngit-blame-create-overlay)\n\nReplace goto-line with (goto-char (point-min)) (forward-line (1-\nstart-line)). According to the documentation of goto-line it should\nnot be called from elisp code. (git-blame-create-overlay)\n\nSigned-off-by: Rüdiger Sonderfeld \n---\n contrib/emacs/git-blame.el | 10 ++++------\n 1 files changed, 4 insertions(+), 6 deletions(-)\n\ndiff --git a/contrib/emacs/git-blame.el b/contrib/emacs/git-blame.el\nindex d351cfb..2e53fc6 100644\n--- a/contrib/emacs/git-blame.el\n+++ b/contrib/emacs/git-blame.el\n@@ -304,7 +304,7 @@ See also function `git-blame-mode'.\"\n \n (defun git-blame-cleanup ()\n \"Remove all blame properties\"\n- (mapcar 'delete-overlay git-blame-overlays)\n+ (mapc 'delete-overlay git-blame-overlays)\n (setq git-blame-overlays nil)\n (remove-git-blame-text-properties (point-min) (point-max)))\n \n@@ -337,8 +337,7 @@ See also function `git-blame-mode'.\"\n (defvar in-blame-filter nil)\n \n (defun git-blame-filter (proc str)\n- (save-excursion\n- (set-buffer (process-buffer proc))\n+ (with-current-buffer (process-buffer proc)\n (goto-char (process-mark proc))\n (insert-before-markers str)\n (goto-char 0)\n@@ -385,11 +384,10 @@ See also function `git-blame-mode'.\"\n info))))\n \n (defun git-blame-create-overlay (info start-line num-lines)\n- (save-excursion\n- (set-buffer git-blame-file)\n+ (with-current-buffer git-blame-file\n (let ((inhibit-point-motion-hooks t)\n (inhibit-modification-hooks t))\n- (goto-line start-line)\n+ (goto-char (point-min)) (forward-line (1- start-line))\n (let* ((start (point))\n (end (progn (forward-line num-lines) (point)))\n (ovl (make-overlay start end))\n-- \n1.7.8.3\n"},{"id":"182436","messageId":"20120112162617.GA2479@burratino","threadId":"29354","inReplyTo":"2608010.fNV39qBMLu@descartes","subject":"Re: [PATCH] git-blame.el: Fix compilation warnings.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-01-12T16:26:41Z","receivedAt":"2012-01-12T16:26:41Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(+cc: Sergei, Kevin)\nHi,\n\nRüdiger Sonderfeld wrote:\n\n> From 4958c1b43d7a66654e15c92cbb878b38533d626e Mon Sep 17 00:00:00 2001\n> From: =?UTF-8?q?R=C3=BCdiger=20Sonderfeld?= \n[...]\n\nThese lines should be left out [*].\n\n> Replace mapcar with mapc because accumulation of the results was not\n> needed. (git-blame-cleanup)\n>\n> Replace two occurrences of (save-excursion (set-buffer buf) ...)\n> with (with-current-buffer buf ...). (git-blame-filter and\n> git-blame-create-overlay)\n>\n> Replace goto-line with (goto-char (point-min)) (forward-line (1-\n> start-line)). According to the documentation of goto-line it should\n> not be called from elisp code. (git-blame-create-overlay)\n>\n> Signed-off-by: Rüdiger Sonderfeld \n\nI assume this was prompted by warning messages like this one:\n\n\tIn git-blame-cleanup:\n\tgit-blame.el:306:6:Warning: `mapcar' called for effect; use `mapc' or `dolist' instead\n\nLooks reasonable to my very much untrained eyes, and it's consistent\nwith the hints Kevin gave at [1].\n\nThanks,\nJonathan\n\n[1] http://bugs.debian.org/cgi-bin/bugreport.cgi?msg=63;bug=611931\n[*] The \"From \" line and following lines are for your mailer and can\nbe omited unless they differ from the mail header when reading your\npatch into an email body. See the DISCUSSION sections of\ngit-format-patch(1) and git-am(1) for more on this.\n\n(patch left unsnipped for Sergei and Kevin's convenience)\n\n> ---\n> contrib/emacs/git-blame.el | 10 ++++------\n> 1 files changed, 4 insertions(+), 6 deletions(-)\n>\n> diff --git a/contrib/emacs/git-blame.el b/contrib/emacs/git-blame.el\n> index d351cfb..2e53fc6 100644\n> --- a/contrib/emacs/git-blame.el\n> +++ b/contrib/emacs/git-blame.el\n> @@ -304,7 +304,7 @@ See also function `git-blame-mode'.\"\n> \n> (defun git-blame-cleanup ()\n> \"Remove all blame properties\"\n> - (mapcar 'delete-overlay git-blame-overlays)\n> + (mapc 'delete-overlay git-blame-overlays)\n> (setq git-blame-overlays nil)\n> (remove-git-blame-text-properties (point-min) (point-max)))\n> \n> @@ -337,8 +337,7 @@ See also function `git-blame-mode'.\"\n> (defvar in-blame-filter nil)\n> \n> (defun git-blame-filter (proc str)\n> - (save-excursion\n> - (set-buffer (process-buffer proc))\n> + (with-current-buffer (process-buffer proc)\n> (goto-char (process-mark proc))\n> (insert-before-markers str)\n> (goto-char 0)\n> @@ -385,11 +384,10 @@ See also function `git-blame-mode'.\"\n> info))))\n> \n> (defun git-blame-create-overlay (info start-line num-lines)\n> - (save-excursion\n> - (set-buffer git-blame-file)\n> + (with-current-buffer git-blame-file\n> (let ((inhibit-point-motion-hooks t)\n> (inhibit-modification-hooks t))\n> - (goto-line start-line)\n> + (goto-char (point-min)) (forward-line (1- start-line))\n> (let* ((start (point))\n> (end (progn (forward-line num-lines) (point)))\n> (ovl (make-overlay start end))\n"},{"id":"182440","messageId":"2304907.sEfEeC6Eon@descartes","threadId":"29354","inReplyTo":"20120112162617.GA2479@burratino","subject":"Re: [PATCH] git-blame.el: Fix compilation warnings.","fromName":"Rüdiger Sonderfeld","fromEmail":"ruediger@c-plusplus.de","sentAt":"2012-01-12T17:08:21Z","receivedAt":"2012-01-12T17:08:21Z","isPatch":true,"sender":{"key":"ruediger@c-plusplus.de","avatar":"https://avatars.githubusercontent.com/u/1803?v=4"},"body":"Hi,\n\nOn Thursday 12 January 2012 10:26:41 Jonathan Nieder wrote:\n> These lines should be left out [*].\n\nSorry, I wasn't sure whether to remove them or not. I followed the description \nin git-format-patch(1) on how to send patches with kmail. I'll remove them in \nthe future. Thanks for the advice.\n \n> I assume this was prompted by warning messages like this one:\n> \n> \tIn git-blame-cleanup:\n> \tgit-blame.el:306:6:Warning: `mapcar' called for effect; use `mapc' or\n> `dolist' instead\n> \n> Looks reasonable to my very much untrained eyes, and it's consistent\n> with the hints Kevin gave at [1].\n\nYes. I think the warnings are correct and should be addressed. E.g. Using \nmapcar compared to mapc is slower due to the required accumulation of the \nresults and the additional garbage collection costs. It's not very dramatic \nbut there is no reason not to fix it imho.\n\nRegards,\nRüdiger\n"},{"id":"182527","messageId":"20120113233158.GD7343@burratino","threadId":"29354","inReplyTo":"2304907.sEfEeC6Eon@descartes","subject":"Sending patches with KMail (Re: [PATCH] git-blame.el: Fix compilation warnings.)","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-01-13T23:31:58Z","receivedAt":"2012-01-13T23:31:58Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nRüdiger Sonderfeld wrote:\n> On Thursday 12 January 2012 10:26:41 Jonathan Nieder wrote:\n\n>> These lines should be left out [*].\n>\n> Sorry, I wasn't sure whether to remove them or not. I followed the description \n> in git-format-patch(1) on how to send patches with kmail. I'll remove them in \n> the future. Thanks for the advice.\n\nOh, thanks for the pointer. How about something like this?\n\nThe hints at [1] might also be useful, in case you would like to try\nand consider improving the manpage to document them if they work.\n\n-- >8 --\nSubject: Documentation/format-patch: mention removal of in-body headers for KMail\n\nThe opening \"From \" line and following lines in \"git format-patch\"\n\n\tFrom 13c41b41b832d41680ccd33a2422ef8217965566 Mon Sep 17 00:00:00 2001\n\tFrom: Jonathan Nieder \n\tDate: Fri, 13 Jan 2012 17:22:41 -0600\n\nare for your mailer and should be omitted except for fields that\ndiffer from the mail header when reading your patch into an email\nbody. Otherwise \"git am\" thinks these lines are part of the commit\nmessage when trying to reproduce the resulting patch from an mbox\nautomatically. Add a reminder in this direction to the KMail recipe.\n\nSuggested-by: Rüdiger Sonderfeld \nSigned-off-by: Jonathan Nieder \n---\n[1] http://thread.gmane.org/gmane.comp.version-control.git/171580/focus=171720\n\n Documentation/git-format-patch.txt | 6 ++++--\n 1 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt\nindex 6ea9be77..5e1d6d2c 100644\n--- a/Documentation/git-format-patch.txt\n+++ b/Documentation/git-format-patch.txt\n@@ -462,8 +462,10 @@ This should help you to submit patches inline using KMail.\n \n 4. Use Message -> Insert file... and insert the patch.\n \n-5. Back in the compose window: add whatever other text you wish to the\n- message, complete the addressing and subject fields, and press send.\n+5. Back in the compose window: remove the \"`From $SHA1 $magic_timestamp`\"\n+ marker and unwanted in-body headers, add whatever other text you wish\n+ to the message, complete the addressing and subject fields, and\n+ press send.\n \n \n EXAMPLES\n-- \n1.7.8.3\n"},{"id":"182528","messageId":"7vlipbxfne.fsf@alter.siamese.dyndns.org","threadId":"29354","inReplyTo":"20120113233158.GD7343@burratino","subject":"Re: Sending patches with KMail (Re: [PATCH] git-blame.el: Fix compilation warnings.)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-14T00:59:49Z","receivedAt":"2012-01-14T00:59:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> The hints at [1] might also be useful, in case you would like to try\n> and consider improving the manpage to document them if they work.\n\nDon't you need similar updates to sections for other MUAs and procedures?\n\nI suspect that the reason why you added the new text there is because you\nknow KMail users are very lazy bunch, and once they see a \"KMail\"\nsubsection, they will skip everything outside the subsection. Thunderbird\nusers would also be lazy---after choosing one of the three approaches\npresented, they will skip anything outside the subsubsection. So I can\nunderstand that we would need something in these individual subsections,\nbut the advice does not logically belong there.\n\nPerhaps rephrasing the early part of the Discussion section, with an\nillustration that is designed to be more visible, would be a better\napproach?\n\nFor example, we could take your log message and stuff it there:\n\n The opening \"From \" line and following lines in \"git format-patch\" are\n for your mailer and should be omitted except for fields that differ from\n the mail header when reading your patch into an email body. For example,\n the output of your format-patch may begin like this:\n\n From 13c41b41b832d41680ccd33a2422ef8217965566 Mon Sep 17 00:00:00 2001\n From: Jonathan Nieder \n Date: Fri, 13 Jan 2012 17:22:41 -0600\n Subject: Documentation/format-patch: mention removal of in-body headers\n\n The opening \"From \" line and following lines in ...\n\n The part you should send in the body of your e-mail message begins at the\n first blank line. The \"From $SHA1 $magic_timestamp\" line and other header\n lines are there to make it look like a mbox, and if you send it in e-mail,\n they will become redundant.\n\n You can leave \"From:\" and/or \"Subject:\" lines in, if they are\n different from the e-mail you will be sending out (e.g. you are\n forwarding a patch written by somebody else, as a follow up to an\n ongoing discussion and do not want the subject of your e-mail message\n to help threading). E.g. your message _may_ begin like this:\n\n From: Jonathan Nieder \n Subject: Documentation/format-patch: mention removal of in-body headers\n\n The opening \"From \" line and following lines in ...\n\n when you are not Jonathan, and you are sending it as a response to\n an existing discussion thread.\n\nOr something like that?\n"},{"id":"182558","messageId":"20120114183111.GC27850@burratino","threadId":"29354","inReplyTo":"7vlipbxfne.fsf@alter.siamese.dyndns.org","subject":"Re: Sending patches with KMail","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-01-14T18:31:11Z","receivedAt":"2012-01-14T18:31:11Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jonathan Nieder writes:\n\n>> The hints at [1] might also be useful, in case you would like to try\n>> and consider improving the manpage to document them if they work.\n>\n> Don't you need similar updates to sections for other MUAs and procedures?\n\nThunderbird approach 3, yes[*]. The others, no.\n\n[...]\n> Perhaps rephrasing the early part of the Discussion section, with an\n> illustration that is designed to be more visible, would be a better\n> approach?\n\nI understand what you mean, but I don't think so. The Discussion\nsection already seems clear to me, so I would prefer to wait to hear\nfrom someone confused by it to find what exactly in it needs tweaking.\nAdding additional paragraphs for each potential misunderstanding by\npeople who have not necessarily read the section has the potential to\nbackfire and lead even more people not to read the section...\n\nMy favorite approach would be to introduce a new option\n--format=plain|mbox, with the default being mbox, allowing\nformat-patch --format=plain to produce a nice patch that does _not_\ninclude a \"From \" line or q-encode its header lines, ready for use\nwithout much tweaking in an email body as an attachment. Then we can\njust say \"If you are not importing your patch as an mbox file, use the\n--format=plain option\".\n\nSane?\n\nJonathan\n\n[*] Though I'd rather just remove it, since \"how to use an external\neditor\" seems orthogonal to \"how to teach Thunderbird not to mangle my\npatches\".\n"},{"id":"182559","messageId":"20120114183446.GD27850@burratino","threadId":"29354","inReplyTo":"20120114183111.GC27850@burratino","subject":"Re: Sending patches with KMail","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-01-14T18:34:46Z","receivedAt":"2012-01-14T18:34:46Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n\n> My favorite approach would be to introduce a new option\n> --format=plain|mbox, with the default being mbox, allowing\n> format-patch --format=plain to produce a nice patch that does _not_\n> include a \"From \" line or q-encode its header lines, ready for use\n> without much tweaking in an email body as an attachment.\n\nThis should have said \"ready for use in an email body or as an\nattachment\" (missing \"or\"). Sorry for the confusion.\n"},{"id":"182561","messageId":"5720118.t52aoWEQJn@descartes","threadId":"29354","inReplyTo":"7vlipbxfne.fsf@alter.siamese.dyndns.org","subject":"Re: Sending patches with KMail (Re: [PATCH] git-blame.el: Fix compilation warnings.)","fromName":"Rüdiger Sonderfeld","fromEmail":"ruediger@c-plusplus.de","sentAt":"2012-01-14T19:18:08Z","receivedAt":"2012-01-14T19:18:08Z","isPatch":true,"sender":{"key":"ruediger@c-plusplus.de","avatar":"https://avatars.githubusercontent.com/u/1803?v=4"},"body":"On Friday 13 January 2012 16:59:49 Junio C Hamano wrote:\n> Perhaps rephrasing the early part of the Discussion section, with an\n> illustration that is designed to be more visible, would be a better\n> approach?\n\nWhy not do both?\n\nI think you are right, that it is currently not very visible in the Discussion \nsection. But on the other hand if you have a step by step guide it should \nprobably mention that as well. It has nothing to do with laziness. But most \npeople follow a step by step guide because they expect that it illustrates the \ncorrect procedure. Jonathan's addition is short but effective.\n\nRegards,\nRüdiger\n"},{"id":"182568","messageId":"7vwr8tww3r.fsf@alter.siamese.dyndns.org","threadId":"29354","inReplyTo":"20120114183111.GC27850@burratino","subject":"Re: Sending patches with KMail","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-15T02:14:16Z","receivedAt":"2012-01-15T02:14:16Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> My favorite approach would be to introduce a new option\n> --format=plain|mbox, with the default being mbox, allowing format-patch\n> --format=plain to produce a nice patch that does _not_ include a \"From \"\n> line or q-encode its header lines, ready for use without much tweaking\n> in an email body as an attachment.\n\nI actually like the removal of q-encoding part. But I am not sure what\nheaders it should produce. What should the beginning of the output file\nlook like? Does it just have \"Subject: \", or does it still have the \"From:\n\", \"Date: \" and \"Subject: \", the first two of which the user would almost\nalways want to remove?\n\nIf we can decide a sane behaviour wrt the pseudo header, and if the option\nis made _incompatible_ with --stdout when (and only when) emitting more\nthan one message, then I think it would be a good addition.\n"},{"id":"193233","messageId":"20120610073803.GA29461@burratino","threadId":"29354","inReplyTo":"2608010.fNV39qBMLu@descartes","subject":"[PATCH] git-blame.el: use mapc instead of mapcar","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-06-10T07:38:03Z","receivedAt":"2012-06-10T07:38:03Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"From: Rüdiger Sonderfeld \n\nUsing mapcar here is a waste of memory because the mapped result\nis not used.\n\nNoticed by emacs (\"Warning: `mapcar' called for effect\").\n\n[jn: split from a larger patch, with new description]\n\nSigned-off-by: Rüdiger Sonderfeld \nSigned-off-by: Jonathan Nieder \n---\nIn January, Rüdiger Sonderfeld wrote:\n\n> Replace mapcar with mapc because accumulation of the results was not\n> needed. (git-blame-cleanup)\n>\n> Replace two occurrences of (save-excursion (set-buffer buf) ...)\n> with (with-current-buffer buf ...). (git-blame-filter and\n> git-blame-create-overlay)\n>\n> Replace goto-line with (goto-char (point-min)) (forward-line (1-\n> start-line)). According to the documentation of goto-line it should\n> not be called from elisp code. (git-blame-create-overlay)\n>\n> Signed-off-by: Rüdiger Sonderfeld \n> ---\n> contrib/emacs/git-blame.el | 10 ++++------\n> 1 files changed, 4 insertions(+), 6 deletions(-)\n\nThanks again, and sorry for the long silence.\n\nI'd prefer to see someone more knowledgeable than I am about elisp\nsubmit the other two fixes. This one is simple enough that I can\nvouch for it, though. One out of three is not that bad, I guess. :)\n\nThoughts?\nJonathan\n\n contrib/emacs/git-blame.el | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/contrib/emacs/git-blame.el b/contrib/emacs/git-blame.el\nindex d351cfb6..37d797e1 100644\n--- a/contrib/emacs/git-blame.el\n+++ b/contrib/emacs/git-blame.el\n@@ -304,7 +304,7 @@ See also function `git-blame-mode'.\"\n \n (defun git-blame-cleanup ()\n \"Remove all blame properties\"\n- (mapcar 'delete-overlay git-blame-overlays)\n+ (mapc 'delete-overlay git-blame-overlays)\n (setq git-blame-overlays nil)\n (remove-git-blame-text-properties (point-min) (point-max)))\n \n-- \n1.7.10\n"},{"id":"193254","messageId":"1339329484-12088-1-git-send-email-wence@gmx.li","threadId":"29354","inReplyTo":"20120610073803.GA29461@burratino","subject":"[PATCH 1/3] git-blame.el: Do not use goto-line in lisp code","fromName":"Lawrence Mitchell","fromEmail":"wence@gmx.li","sentAt":"2012-06-10T11:58:02Z","receivedAt":"2012-06-10T11:58:02Z","isPatch":true,"sender":{"key":"wence@gmx.li","avatar":"https://avatars.githubusercontent.com/u/1126981?v=4"},"body":"From: Rüdiger Sonderfeld \n\ngoto-line is a user-level command, instead use the lisp-level\nconstruct recommended in Emacs documentation.\n\nSigned-off-by: Rüdiger Sonderfeld \nSigned-off-by: Lawrence Mitchell \n---\n contrib/emacs/git-blame.el | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\nHere we go, all Rüdiger's changes look sensible, I've split them into bits though\n\ndiff --git a/contrib/emacs/git-blame.el b/contrib/emacs/git-blame.el\nindex 37d797e..5428ff7 100644\n--- a/contrib/emacs/git-blame.el\n+++ b/contrib/emacs/git-blame.el\n@@ -389,7 +389,8 @@ See also function `git-blame-mode'.\"\n (set-buffer git-blame-file)\n (let ((inhibit-point-motion-hooks t)\n (inhibit-modification-hooks t))\n- (goto-line start-line)\n+ (goto-char (point-min))\n+ (forward-line (1- start-line))\n (let* ((start (point))\n (end (progn (forward-line num-lines) (point)))\n (ovl (make-overlay start end))\n-- \n1.7.10.2.552.gaa3bb87\n"},{"id":"193255","messageId":"1339329484-12088-2-git-send-email-wence@gmx.li","threadId":"29354","inReplyTo":"1339329484-12088-1-git-send-email-wence@gmx.li","subject":"[PATCH 2/3] git-blame.el: Use with-current-buffer where appropriate","fromName":"Lawrence Mitchell","fromEmail":"wence@gmx.li","sentAt":"2012-06-10T11:58:03Z","receivedAt":"2012-06-10T11:58:03Z","isPatch":true,"sender":{"key":"wence@gmx.li","avatar":"https://avatars.githubusercontent.com/u/1126981?v=4"},"body":"From: Rüdiger Sonderfeld \n\nIn git-blame-filter and git-blame-new-commit we need to execute the\nbody with (current-buffer) bound to the correct output buffer. We\nthen want to restore the previous value of (current-buffer). The\nidiom\n\n (save-excursion\n (set-buffer buf)\n ...)\n\nwill not correctly save the original buffer the code was executed in.\nInstead, use with-current-buffer as recommended in Emacs documentation.\n\nSigned-off-by: Rüdiger Sonderfeld \nSigned-off-by: Lawrence Mitchell \n---\n contrib/emacs/git-blame.el | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/contrib/emacs/git-blame.el b/contrib/emacs/git-blame.el\nindex 5428ff7..20cf9a6 100644\n--- a/contrib/emacs/git-blame.el\n+++ b/contrib/emacs/git-blame.el\n@@ -337,8 +337,7 @@ See also function `git-blame-mode'.\"\n (defvar in-blame-filter nil)\n \n (defun git-blame-filter (proc str)\n- (save-excursion\n- (set-buffer (process-buffer proc))\n+ (with-current-buffer (process-buffer proc)\n (goto-char (process-mark proc))\n (insert-before-markers str)\n (goto-char 0)\n@@ -385,8 +384,7 @@ See also function `git-blame-mode'.\"\n info))))\n \n (defun git-blame-create-overlay (info start-line num-lines)\n- (save-excursion\n- (set-buffer git-blame-file)\n+ (with-current-buffer git-blame-file\n (let ((inhibit-point-motion-hooks t)\n (inhibit-modification-hooks t))\n (goto-char (point-min))\n-- \n1.7.10.2.552.gaa3bb87\n"},{"id":"193253","messageId":"1339329484-12088-3-git-send-email-wence@gmx.li","threadId":"29354","inReplyTo":"1339329484-12088-2-git-send-email-wence@gmx.li","subject":"[PATCH 3/3] git-blame.el: Do not use bare 0 to mean (point-min)","fromName":"Lawrence Mitchell","fromEmail":"wence@gmx.li","sentAt":"2012-06-10T11:58:04Z","receivedAt":"2012-06-10T11:58:04Z","isPatch":true,"sender":{"key":"wence@gmx.li","avatar":"https://avatars.githubusercontent.com/u/1126981?v=4"},"body":"Signed-off-by: Lawrence Mitchell \n---\n contrib/emacs/git-blame.el | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\nA small cleanup I noticed while glancing at the code\n\ndiff --git a/contrib/emacs/git-blame.el b/contrib/emacs/git-blame.el\nindex 20cf9a6..ef1eebd 100644\n--- a/contrib/emacs/git-blame.el\n+++ b/contrib/emacs/git-blame.el\n@@ -340,7 +340,7 @@ See also function `git-blame-mode'.\"\n (with-current-buffer (process-buffer proc)\n (goto-char (process-mark proc))\n (insert-before-markers str)\n- (goto-char 0)\n+ (goto-char (point-min))\n (unless in-blame-filter\n (let ((more t)\n (in-blame-filter t))\n-- \n1.7.10.2.552.gaa3bb87\n"},{"id":"193616","messageId":"20120614050854.GG27586@burratino","threadId":"29354","inReplyTo":"1339329484-12088-1-git-send-email-wence@gmx.li","subject":"Re: [PATCH 1/3] git-blame.el: Do not use goto-line in lisp code","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-06-14T05:08:54Z","receivedAt":"2012-06-14T05:08:54Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi Lawrence,\n\nLawrence Mitchell wrote:\n\n> From: Rüdiger Sonderfeld \n>\n> goto-line is a user-level command, instead use the lisp-level\n> construct recommended in Emacs documentation.\n[...]\n> Here we go, all Rüdiger's changes look sensible, I've split them into bits though\n\nThanks for looking them over.\n\nWould you mind indulging my curiosity a little by describing what bad\nbehavior or potential bad behavior this change prevents?\n\nEven without that information, I'm all for applying this patch, since\nit seems to be what all the people who know elisp recommend. :)\n\nRegards,\nJonathan\n\n(patch kept unsnipped for convenience)\n> diff --git a/contrib/emacs/git-blame.el b/contrib/emacs/git-blame.el\n> index 37d797e..5428ff7 100644\n> --- a/contrib/emacs/git-blame.el\n> +++ b/contrib/emacs/git-blame.el\n> @@ -389,7 +389,8 @@ See also function `git-blame-mode'.\"\n> (set-buffer git-blame-file)\n> (let ((inhibit-point-motion-hooks t)\n> (inhibit-modification-hooks t))\n> - (goto-line start-line)\n> + (goto-char (point-min))\n> + (forward-line (1- start-line))\n> (let* ((start (point))\n> (end (progn (forward-line num-lines) (point)))\n> (ovl (make-overlay start end))\n"},{"id":"193633","messageId":"87k3za9rwj.fsf@gmx.li","threadId":"29354","inReplyTo":"20120614050854.GG27586@burratino","subject":"Re: [PATCH 1/3] git-blame.el: Do not use goto-line in lisp code","fromName":"Lawrence Mitchell","fromEmail":"wence@gmx.li","sentAt":"2012-06-14T09:14:36Z","receivedAt":"2012-06-14T09:14:36Z","isPatch":true,"sender":{"key":"wence@gmx.li","avatar":"https://avatars.githubusercontent.com/u/1126981?v=4"},"body":"Jonathan Nieder wrote:\n> Hi Lawrence,\n\n> Lawrence Mitchell wrote:\n\n>> From: Rüdiger Sonderfeld \n\n>> goto-line is a user-level command, instead use the lisp-level\n>> construct recommended in Emacs documentation.\n> [...]\n>> Here we go, all Rüdiger's changes look sensible, I've split them into bits though\n\n> Thanks for looking them over.\n\n> Would you mind indulging my curiosity a little by describing what bad\n> behavior or potential bad behavior this change prevents?\n\n\ngoto-line sets the mark, and respects the variable\nselective-display. It also widens the buffer before moving to\nthe relevant line. The first two are almost never what you'd\nwant in lisp code, the latter you'd probably want to make\nexplicit in the calls I guess.\n\nthe with-current-buffer issue is a bit more subtle, and I realise\nmy patch for this didn't actually fix the bug, or describe the\nproblem properly (reroll to come).\n\nBasically:\n\nsave-excursion saves point, mark and current-buffer in the buffer\nin scope when it is called, but if we do:\n\n(save-excursion\n (set-buffer buf)\n ;; modify point and mark in buf\n ...)\n\nhoping to save point and mark in buf, it doesn't happen.\nInstead, we need to make buf current before calling\nsave-excursion. And we want to restore the value of\ncurrent-buffer in scope at the beginning of the call afterward,\nhence the correct idiom is:\n\n(with-current-buffer buf\n (save-excursion ...))\n\nCheers,\n\nLawrence\n\n-- \nLawrence Mitchell \n"},{"id":"193638","messageId":"1339666680-4381-1-git-send-email-wence@gmx.li","threadId":"29354","inReplyTo":"87k3za9rwj.fsf@gmx.li","subject":"[PATCH v2 1/3] git-blame.el: Do not use goto-line in lisp code","fromName":"Lawrence Mitchell","fromEmail":"wence@gmx.li","sentAt":"2012-06-14T09:37:58Z","receivedAt":"2012-06-14T09:37:58Z","isPatch":true,"sender":{"key":"wence@gmx.li","avatar":"https://avatars.githubusercontent.com/u/1126981?v=4"},"body":"From: Rüdiger Sonderfeld \n\ngoto-line is a user-level command, instead use the lisp-level\nconstruct recommended in Emacs documentation.\n\nSigned-off-by: Rüdiger Sonderfeld \nSigned-off-by: Lawrence Mitchell \n---\n contrib/emacs/git-blame.el | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\nNo change from v1\n\ndiff --git a/contrib/emacs/git-blame.el b/contrib/emacs/git-blame.el\nindex 37d797e..5428ff7 100644\n--- a/contrib/emacs/git-blame.el\n+++ b/contrib/emacs/git-blame.el\n@@ -389,7 +389,8 @@ See also function `git-blame-mode'.\"\n (set-buffer git-blame-file)\n (let ((inhibit-point-motion-hooks t)\n (inhibit-modification-hooks t))\n- (goto-line start-line)\n+ (goto-char (point-min))\n+ (forward-line (1- start-line))\n (let* ((start (point))\n (end (progn (forward-line num-lines) (point)))\n (ovl (make-overlay start end))\n-- \n1.7.11.rc2.9.g10afb6c\n"},{"id":"193637","messageId":"1339666680-4381-2-git-send-email-wence@gmx.li","threadId":"29354","inReplyTo":"1339666680-4381-1-git-send-email-wence@gmx.li","subject":"[PATCH v2 2/3] git-blame.el: Use with-current-buffer where appropriate","fromName":"Lawrence Mitchell","fromEmail":"wence@gmx.li","sentAt":"2012-06-14T09:37:59Z","receivedAt":"2012-06-14T09:37:59Z","isPatch":true,"sender":{"key":"wence@gmx.li","avatar":"https://avatars.githubusercontent.com/u/1126981?v=4"},"body":"In git-blame-filter and git-blame-create-overlay we want to save\n(along with the values of point and mark) the current-buffer in scope\nwhen calling the functions. The idiom\n\n (save-excursion\n (set-buffer buf)\n ...)\n\nwill correctly restore the correct buffer, but will not save the\nvalues of point and mark in buf (only in the buffer current when the\nsave-excursion call is executed). The intention of these functions is\nto save the current buffer from the calling scope and the values of\npoint and mark in the buffer they are modifying. The correct idiom\nfor this is\n\n (with-current-buffer buf\n (save-excursion\n ...))\n\nSigned-off-by: Rüdiger Sonderfeld \nSigned-off-by: Lawrence Mitchell \n---\n contrib/emacs/git-blame.el | 74 +++++++++++++++++++++++-----------------------\n 1 file changed, 37 insertions(+), 37 deletions(-)\n\nUpdated commit message that actually correctly matches what Emacs\ndoes, plus don't just squash the byte-compiler warnings but actually\nfix the bug that it was pointing out to us.\n\nFor reference, here's the whitespace-squashed change:\n\n| @@ -337,8 +337,8 @@ See also function `git-blame-mode'.\"\n| (defvar in-blame-filter nil)\n \n| (defun git-blame-filter (proc str)\n| + (with-current-buffer (process-buffer proc)\n| (save-excursion\n| - (set-buffer (process-buffer proc))\n| (goto-char (process-mark proc))\n| (insert-before-markers str)\n| (goto-char 0)\n| @@ -346,7 +346,7 @@ See also function `git-blame-mode'.\"\n| (let ((more t)\n| (in-blame-filter t))\n| (while more\n| - (setq more (git-blame-parse)))))))\n| + (setq more (git-blame-parse))))))))\n \n| (defun git-blame-parse ()\n| (cond ((looking-at \"\\\\([0-9a-f]\\\\{40\\\\}\\\\) \\\\([0-9]+\\\\) \\\\([0-9]+\\\\) \\\\([0-9]+\\\\)\\n\")\n| @@ -385,8 +385,8 @@ See also function `git-blame-mode'.\"\n| info))))\n \n| (defun git-blame-create-overlay (info start-line num-lines)\n| + (with-current-buffer git-blame-file\n| (save-excursion\n| - (set-buffer git-blame-file)\n| (let ((inhibit-point-motion-hooks t)\n| (inhibit-modification-hooks t))\n| (goto-char (point-min))\n| @@ -411,7 +411,7 @@ See also function `git-blame-mode'.\"\n| (cdr (assq 'color (cdr info))))))\n| (overlay-put ovl 'line-prefix\n| (propertize (format-spec git-blame-prefix-format spec)\n| - 'face 'git-blame-prefix-face))))))\n| + 'face 'git-blame-prefix-face)))))))\n \n\n\ndiff --git a/contrib/emacs/git-blame.el b/contrib/emacs/git-blame.el\nindex 5428ff7..bb6d7bb 100644\n--- a/contrib/emacs/git-blame.el\n+++ b/contrib/emacs/git-blame.el\n@@ -337,16 +337,16 @@ See also function `git-blame-mode'.\"\n (defvar in-blame-filter nil)\n \n (defun git-blame-filter (proc str)\n- (save-excursion\n- (set-buffer (process-buffer proc))\n- (goto-char (process-mark proc))\n- (insert-before-markers str)\n- (goto-char 0)\n- (unless in-blame-filter\n- (let ((more t)\n- (in-blame-filter t))\n- (while more\n- (setq more (git-blame-parse)))))))\n+ (with-current-buffer (process-buffer proc)\n+ (save-excursion\n+ (goto-char (process-mark proc))\n+ (insert-before-markers str)\n+ (goto-char 0)\n+ (unless in-blame-filter\n+ (let ((more t)\n+ (in-blame-filter t))\n+ (while more\n+ (setq more (git-blame-parse))))))))\n \n (defun git-blame-parse ()\n (cond ((looking-at \"\\\\([0-9a-f]\\\\{40\\\\}\\\\) \\\\([0-9]+\\\\) \\\\([0-9]+\\\\) \\\\([0-9]+\\\\)\\n\")\n@@ -385,33 +385,33 @@ See also function `git-blame-mode'.\"\n info))))\n \n (defun git-blame-create-overlay (info start-line num-lines)\n- (save-excursion\n- (set-buffer git-blame-file)\n- (let ((inhibit-point-motion-hooks t)\n- (inhibit-modification-hooks t))\n- (goto-char (point-min))\n- (forward-line (1- start-line))\n- (let* ((start (point))\n- (end (progn (forward-line num-lines) (point)))\n- (ovl (make-overlay start end))\n- (hash (car info))\n- (spec `((?h . ,(substring hash 0 6))\n- (?H . ,hash)\n- (?a . ,(git-blame-get-info info 'author))\n- (?A . ,(git-blame-get-info info 'author-mail))\n- (?c . ,(git-blame-get-info info 'committer))\n- (?C . ,(git-blame-get-info info 'committer-mail))\n- (?s . ,(git-blame-get-info info 'summary)))))\n- (push ovl git-blame-overlays)\n- (overlay-put ovl 'git-blame info)\n- (overlay-put ovl 'help-echo\n- (format-spec git-blame-mouseover-format spec))\n- (if git-blame-use-colors\n- (overlay-put ovl 'face (list :background\n- (cdr (assq 'color (cdr info))))))\n- (overlay-put ovl 'line-prefix\n- (propertize (format-spec git-blame-prefix-format spec)\n- 'face 'git-blame-prefix-face))))))\n+ (with-current-buffer git-blame-file\n+ (save-excursion\n+ (let ((inhibit-point-motion-hooks t)\n+ (inhibit-modification-hooks t))\n+ (goto-char (point-min))\n+ (forward-line (1- start-line))\n+ (let* ((start (point))\n+ (end (progn (forward-line num-lines) (point)))\n+ (ovl (make-overlay start end))\n+ (hash (car info))\n+ (spec `((?h . ,(substring hash 0 6))\n+ (?H . ,hash)\n+ (?a . ,(git-blame-get-info info 'author))\n+ (?A . ,(git-blame-get-info info 'author-mail))\n+ (?c . ,(git-blame-get-info info 'committer))\n+ (?C . ,(git-blame-get-info info 'committer-mail))\n+ (?s . ,(git-blame-get-info info 'summary)))))\n+ (push ovl git-blame-overlays)\n+ (overlay-put ovl 'git-blame info)\n+ (overlay-put ovl 'help-echo\n+ (format-spec git-blame-mouseover-format spec))\n+ (if git-blame-use-colors\n+ (overlay-put ovl 'face (list :background\n+ (cdr (assq 'color (cdr info))))))\n+ (overlay-put ovl 'line-prefix\n+ (propertize (format-spec git-blame-prefix-format spec)\n+ 'face 'git-blame-prefix-face)))))))\n \n (defun git-blame-add-info (info key value)\n (nconc info (list (cons (intern key) value))))\n-- \n1.7.11.rc2.9.g10afb6c\n"},{"id":"193639","messageId":"1339666680-4381-3-git-send-email-wence@gmx.li","threadId":"29354","inReplyTo":"1339666680-4381-2-git-send-email-wence@gmx.li","subject":"[PATCH v2 3/3] git-blame.el: Do not use bare 0 to mean (point-min)","fromName":"Lawrence Mitchell","fromEmail":"wence@gmx.li","sentAt":"2012-06-14T09:38:00Z","receivedAt":"2012-06-14T09:38:00Z","isPatch":true,"sender":{"key":"wence@gmx.li","avatar":"https://avatars.githubusercontent.com/u/1126981?v=4"},"body":"Signed-off-by: Lawrence Mitchell \n---\n contrib/emacs/git-blame.el | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\nNo change from v1\n\ndiff --git a/contrib/emacs/git-blame.el b/contrib/emacs/git-blame.el\nindex bb6d7bb..e671f6c 100644\n--- a/contrib/emacs/git-blame.el\n+++ b/contrib/emacs/git-blame.el\n@@ -341,7 +341,7 @@ See also function `git-blame-mode'.\"\n (save-excursion\n (goto-char (process-mark proc))\n (insert-before-markers str)\n- (goto-char 0)\n+ (goto-char (point-min))\n (unless in-blame-filter\n (let ((more t)\n (in-blame-filter t))\n-- \n1.7.11.rc2.9.g10afb6c\n"}]}