[PATCH] emacs: add missing paren to fix defun in notmuch-address.el.

[PATCH] emacs: add missing paren to fix defun in notmuch-address.el.

[Karl Fogel]

[-- Attachment #1: Type: text/plain, Size: 209 bytes --]

This patch fixes a trivial missing-paren problem in notmuch-address.el
(and reindents the following defun accordingly).  I'm not subscribed
to this list, so please keep me CC'd on any followups.

Best,
-Karl


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Trivial fix to restore missing paren in notmuch-address.el. --]
[-- Type: text/x-diff, Size: 1619 bytes --]

From 4c74ad313f608f0834961c63c70d1f811ef103b7 Mon Sep 17 00:00:00 2001
From: Karl Fogel <kfogel@red-bean.com>
Date: Sat, 6 Apr 2013 13:21:54 -0400
Subject: [PATCH] emacs: add missing paren to close
 `notmuch-bbdb/snarf-headers' definition.

This unbreaks emacs/notmuch-address.el.  Without this fix,
the notmuch build process fails with this error:

  $ make
  ...
  In toplevel form:
  emacs/notmuch.el:56:1:Error: End of file during parsing: \
    /path/to/source/tree/notmuch/emacs/notmuch-address.el
  make: *** [emacs/notmuch.elc] Error 1
  $
---
 emacs/notmuch-address.el |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/emacs/notmuch-address.el b/emacs/notmuch-address.el
index 32c8490..4eda098 100644
--- a/emacs/notmuch-address.el
+++ b/emacs/notmuch-address.el
@@ -107,13 +107,13 @@ line."
   (let ((addrs (bbdb-get-addresses nil nil 'notmuch-bbdb/get-header-content))
 	(bbdb-get-addresses-headers headers) ; headers to read
 	(bbdb-gag-messages t)) ; suppress m/n processed message)
-    (bbdb-update-records addrs t t))
+    (bbdb-update-records addrs t t)))
 
-  (defun notmuch-bbdb/snarf-from ()
-    "Import the sender of the current message into BBDB"
-    (interactive)
-    (notmuch-bbdb/snarf-headers
-     (list (assoc 'authors bbdb-get-addresses-headers))))
+(defun notmuch-bbdb/snarf-from ()
+  "Import the sender of the current message into BBDB"
+  (interactive)
+  (notmuch-bbdb/snarf-headers
+   (list (assoc 'authors bbdb-get-addresses-headers))))
 
 (defun notmuch-bbdb/snarf-to ()
   "Import all recipients of the current message into BBDB"
-- 
1.7.10.4

Re: [PATCH] emacs: add missing paren to fix defun in notmuch-address.el.

[David Bremner]

Karl Fogel <kfogel@red-bean.com> writes:

> This patch fixes a trivial missing-paren problem in notmuch-address.el
> (and reindents the following defun accordingly).  I'm not subscribed
> to this list, so please keep me CC'd on any followups.
>
> Best,
> -Karl

Dear Karl;

Thanks very much for the patch.

Since the offending commit is now reverted, it would be great if
somebody (TM) would combine your patch with  238bf4cb09

Cheers,

d

Re: [PATCH] emacs: add missing paren to fix defun in notmuch-address.el.

[Karl Fogel]

David Bremner <david@tethera.net> writes:
>Karl Fogel <kfogel@red-bean.com> writes:
>> This patch fixes a trivial missing-paren problem in notmuch-address.el
>> (and reindents the following defun accordingly).  I'm not subscribed
>> to this list, so please keep me CC'd on any followups.
>
>Dear Karl;
>
>Thanks very much for the patch.
>
>Since the offending commit is now reverted, it would be great if
>somebody (TM) would combine your patch with  238bf4cb09.

Oh, it's trivial.  The problem with 238bf4cb09 was simply that the
function (defun) `notmuch-bbdb/snarf-headers' was missing a closing
paren.  A visible symptom of this was that the *next* defun after it,
`notmuch-bbdb/snarf-from', was spuriously indented inward.  If anyone
had tried reindenting further, all the code below it would also have
indented inward, making the problem more obvious.

So the solution is to:

  1) Re-apply the 238bf4cb09 patch

  2) Add a parenthesis to the end of `notmuch-bbdb/snarf-headers',
     such that the line "(bbdb-update-records addrs t t))" becomes
     "(bbdb-update-records addrs t t)))"

  3) Unindent the function `notmuch-bbdb/snarf-from' immediately below
     there, which just means pulling each line leftward two spaces

  4) Commit, push, profit :-).

The above recipe is, of course, equivalent to re-applying the 238bf4cb09
patch, then applying my patch (4c74ad313f608f0834961c63c70d1f811ef103b7)
on top of it.  I'm not sure what the gitmost way to do that is, but if
you want I can simply submit a combined change whose commit message
makes clear what's going on.

-Karl

Re: [PATCH] emacs: add missing paren to fix defun in notmuch-address.el.

[Tomi Ollila]

On Mon, Apr 08 2013, Karl Fogel <kfogel@red-bean.com> wrote:

> David Bremner <david@tethera.net> writes:
>>Karl Fogel <kfogel@red-bean.com> writes:
>>> This patch fixes a trivial missing-paren problem in notmuch-address.el
>>> (and reindents the following defun accordingly).  I'm not subscribed
>>> to this list, so please keep me CC'd on any followups.
>>
>>Dear Karl;
>>
>>Thanks very much for the patch.
>>
>>Since the offending commit is now reverted, it would be great if
>>somebody (TM) would combine your patch with  238bf4cb09.
>
> Oh, it's trivial.  The problem with 238bf4cb09 was simply that the
> function (defun) `notmuch-bbdb/snarf-headers' was missing a closing
> paren.  A visible symptom of this was that the *next* defun after it,
> `notmuch-bbdb/snarf-from', was spuriously indented inward.  If anyone
> had tried reindenting further, all the code below it would also have
> indented inward, making the problem more obvious.
>
> So the solution is to:
>
>   1) Re-apply the 238bf4cb09 patch
>
>   2) Add a parenthesis to the end of `notmuch-bbdb/snarf-headers',
>      such that the line "(bbdb-update-records addrs t t))" becomes
>      "(bbdb-update-records addrs t t)))"
>
>   3) Unindent the function `notmuch-bbdb/snarf-from' immediately below
>      there, which just means pulling each line leftward two spaces
>
>   4) Commit, push, profit :-).
>
> The above recipe is, of course, equivalent to re-applying the 238bf4cb09
> patch, then applying my patch (4c74ad313f608f0834961c63c70d1f811ef103b7)
> on top of it.  I'm not sure what the gitmost way to do that is, but if
> you want I can simply submit a combined change whose commit message
> makes clear what's going on.

The most tolerable way is to send new patch series w/ these 2 patches
that apply cleanly on top of current master (661dcf87aeb70) so that the
things that David needs to do are just to run `git am` and `make test`.

... also, proceeding this way helps getting reviewers to the changes,
which in turn makes changes more likely to be pushed to our master
repository.

> -Karl

Tomi

Re: [PATCH] emacs: add missing paren to fix defun in notmuch-address.el.

[Jani Nikula]

On Mon, 08 Apr 2013, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> The most tolerable way is to send new patch series w/ these 2 patches
> that apply cleanly on top of current master (661dcf87aeb70) so that the
> things that David needs to do are just to run `git am` and `make test`.

I'd say just squash the two together. We already have the old reverted
commit that doesn't build, let's not add another commit that breaks
bisect (= every commit must build, work, and pass tests).

BR,
Jani.

Re: [PATCH] emacs: add missing paren to fix defun in notmuch-address.el.

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

> On Mon, 08 Apr 2013, Tomi Ollila <tomi.ollila@iki.fi> wrote:
>> The most tolerable way is to send new patch series w/ these 2 patches
>> that apply cleanly on top of current master (661dcf87aeb70) so that the
>> things that David needs to do are just to run `git am` and `make test`.
>
> I'd say just squash the two together. We already have the old reverted
> commit that doesn't build, let's not add another commit that breaks
> bisect (= every commit must build, work, and pass tests).
>
> BR,
> Jani.

Squashed sounds good to me too. Sorry I'm too lazy/busy at the moment to
do it myself.

d

Re: [PATCH] emacs: add missing paren to fix defun in notmuch-address.el.

[Jed Brown]

[-- Attachment #1: Type: text/plain, Size: 243 bytes --]

David Bremner <david@tethera.net> writes:

> Squashed sounds good to me too. Sorry I'm too lazy/busy at the moment to
> do it myself.

I tested the attached and it's working as expected.  Sent as an
attachment to preserve author information.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-emacs-functions-to-import-sender-or-recipient-into-B.patch --]
[-- Type: text/x-patch, Size: 2487 bytes --]

From 5404ac5bf13f8b5349d5b94f9f2000e9d1832b83 Mon Sep 17 00:00:00 2001
From: Daniel Bergey <bergey@alum.mit.edu>
Date: Mon, 8 Apr 2013 19:55:04 -0500
Subject: [PATCH] emacs: functions to import sender or recipient into BBDB [v2]

From a show buffer, notmuch-bbdb/snarf-from imports the sender into
bbdb.  notmuch-bbdb/snarf-to imports all recipients.  Newly imported
contacts are reported in the minibuffer / Messages buffer.

Both functions use the BBDB parser to recognize email address formats.

[v2] Fixes missing close parenthesis in original.
     Spotted by Karl Fogel <kfogel@red-bean.com>.
---
 emacs/notmuch-address.el | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/emacs/notmuch-address.el b/emacs/notmuch-address.el
index 2bf762b..4eda098 100644
--- a/emacs/notmuch-address.el
+++ b/emacs/notmuch-address.el
@@ -96,6 +96,47 @@ line."
 (when (notmuch-address-locate-command notmuch-address-command)
   (notmuch-address-message-insinuate))
 
+;; functions to add sender / recipients to BBDB
+
+(defun notmuch-bbdb/snarf-headers (headers)
+  ;; Helper function to avoid code duplication in the two below
+  ;; headers should have the same format as bbdb-get-addresses-headers
+
+  ;; bbdb-get-addresses reads these
+  ;; Ugh, pass-by-global
+  (let ((addrs (bbdb-get-addresses nil nil 'notmuch-bbdb/get-header-content))
+	(bbdb-get-addresses-headers headers) ; headers to read
+	(bbdb-gag-messages t)) ; suppress m/n processed message)
+    (bbdb-update-records addrs t t)))
+
+(defun notmuch-bbdb/snarf-from ()
+  "Import the sender of the current message into BBDB"
+  (interactive)
+  (notmuch-bbdb/snarf-headers
+   (list (assoc 'authors bbdb-get-addresses-headers))))
+
+(defun notmuch-bbdb/snarf-to ()
+  "Import all recipients of the current message into BBDB"
+  (interactive)
+  (notmuch-bbdb/snarf-headers
+   (list (assoc 'recipients bbdb-get-addresses-headers))))
+
+(defvar notmuch-bbdb/header-by-name
+  ;; both are case sensitive
+  '( ("From" . :From)
+     ("To" . :To)
+     ("CC" . :Cc)
+     ("BCC" . :Bcc)
+     ("Resent-From" . nil)
+     ("Reply-To" . nil)
+     ("Resent-To" . nil)
+     ("Resent-CC" . nil))
+  "Alist for dispatching header symbols as used by notmuch-show-get-header
+from strings as used by bbdb-get-addresses")
+
+(defun notmuch-bbdb/get-header-content (name)
+  (notmuch-show-get-header (cdr (assoc name notmuch-bbdb/header-by-name))))
+
 ;;
 
 (provide 'notmuch-address)
-- 
1.8.2.1

Re: [PATCH] emacs: add missing paren to fix defun in notmuch-address.el.

[Tomi Ollila]

On Tue, Apr 09 2013, Jed Brown wrote:

> David Bremner <david@tethera.net> writes:
>
>> Squashed sounds good to me too. Sorry I'm too lazy/busy at the moment to
>> do it myself.
>
> I tested the attached and it's working as expected.  Sent as an
> attachment to preserve author information.

LGTM (this time tested, too). Also compared diffs by hand, change as announced.
comments in id:m26207wgyt.fsf@guru.guru-group.fi (amend!) apply. 

Tomi


>From 5404ac5bf13f8b5349d5b94f9f2000e9d1832b83 Mon Sep 17 00:00:00 2001
> From: Daniel Bergey <bergey@alum.mit.edu>
> Date: Mon, 8 Apr 2013 19:55:04 -0500
> Subject: [PATCH] emacs: functions to import sender or recipient into BBDB [v2]
>
>From a show buffer, notmuch-bbdb/snarf-from imports the sender into
> bbdb.  notmuch-bbdb/snarf-to imports all recipients.  Newly imported
> contacts are reported in the minibuffer / Messages buffer.
>
> Both functions use the BBDB parser to recognize email address formats.
>
> [v2] Fixes missing close parenthesis in original.
>      Spotted by Karl Fogel <kfogel@red-bean.com>.
> ---
>  emacs/notmuch-address.el | 41 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>
> diff --git a/emacs/notmuch-address.el b/emacs/notmuch-address.el
> index 2bf762b..4eda098 100644
> --- a/emacs/notmuch-address.el
> +++ b/emacs/notmuch-address.el
> @@ -96,6 +96,47 @@ line."
>  (when (notmuch-address-locate-command notmuch-address-command)
>    (notmuch-address-message-insinuate))
>  
> +;; functions to add sender / recipients to BBDB
> +
> +(defun notmuch-bbdb/snarf-headers (headers)
> +  ;; Helper function to avoid code duplication in the two below
> +  ;; headers should have the same format as bbdb-get-addresses-headers
> +
> +  ;; bbdb-get-addresses reads these
> +  ;; Ugh, pass-by-global
> +  (let ((addrs (bbdb-get-addresses nil nil 'notmuch-bbdb/get-header-content))
> +	(bbdb-get-addresses-headers headers) ; headers to read
> +	(bbdb-gag-messages t)) ; suppress m/n processed message)
> +    (bbdb-update-records addrs t t)))
> +
> +(defun notmuch-bbdb/snarf-from ()
> +  "Import the sender of the current message into BBDB"
> +  (interactive)
> +  (notmuch-bbdb/snarf-headers
> +   (list (assoc 'authors bbdb-get-addresses-headers))))
> +
> +(defun notmuch-bbdb/snarf-to ()
> +  "Import all recipients of the current message into BBDB"
> +  (interactive)
> +  (notmuch-bbdb/snarf-headers
> +   (list (assoc 'recipients bbdb-get-addresses-headers))))
> +
> +(defvar notmuch-bbdb/header-by-name
> +  ;; both are case sensitive
> +  '( ("From" . :From)
> +     ("To" . :To)
> +     ("CC" . :Cc)
> +     ("BCC" . :Bcc)
> +     ("Resent-From" . nil)
> +     ("Reply-To" . nil)
> +     ("Resent-To" . nil)
> +     ("Resent-CC" . nil))
> +  "Alist for dispatching header symbols as used by notmuch-show-get-header
> +from strings as used by bbdb-get-addresses")
> +
> +(defun notmuch-bbdb/get-header-content (name)
> +  (notmuch-show-get-header (cdr (assoc name notmuch-bbdb/header-by-name))))
> +
>  ;;
>  
>  (provide 'notmuch-address)
> -- 
> 1.8.2.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] emacs: add missing paren to fix defun in notmuch-address.el.

[David Bremner]

Tomi Ollila <tomi.ollila@iki.fi> writes:

> On Tue, Apr 09 2013, Jed Brown wrote:
>
>> David Bremner <david@tethera.net> writes:
>>
>>> Squashed sounds good to me too. Sorry I'm too lazy/busy at the moment to
>>> do it myself.
>>
>> I tested the attached and it's working as expected.  Sent as an
>> attachment to preserve author information.
>
> LGTM (this time tested, too). Also compared diffs by hand, change as announced.
> comments in id:m26207wgyt.fsf@guru.guru-group.fi (amend!) apply. 

There seems to be a few warnings:

In notmuch-bbdb/snarf-from:
notmuch-address.el:116:26:Warning: reference to free variable
    `bbdb-get-addresses-headers'

In notmuch-bbdb/snarf-to:
notmuch-address.el:122:29:Warning: reference to free variable
    `bbdb-get-addresses-headers'

In end of data:
notmuch-address.el:143:1:Warning: the following functions are not known to be
    defined: bbdb-get-addresses, bbdb-update-records, notmuch-show-get-header

Do we need a few defvars?

d

Re: [PATCH] emacs: add missing paren to fix defun in notmuch-address.el.

[Tomi Ollila]

On Tue, Apr 09 2013, David Bremner wrote:

> Tomi Ollila <tomi.ollila@iki.fi> writes:
>
>> On Tue, Apr 09 2013, Jed Brown wrote:
>>
>>> David Bremner <david@tethera.net> writes:
>>>
>>>> Squashed sounds good to me too. Sorry I'm too lazy/busy at the moment to
>>>> do it myself.
>>>
>>> I tested the attached and it's working as expected.  Sent as an
>>> attachment to preserve author information.
>>
>> LGTM (this time tested, too). Also compared diffs by hand, change as announced.
>> comments in id:m26207wgyt.fsf@guru.guru-group.fi (amend!) apply. 
>
> There seems to be a few warnings:
>
> In notmuch-bbdb/snarf-from:
> notmuch-address.el:116:26:Warning: reference to free variable
>     `bbdb-get-addresses-headers'
>
> In notmuch-bbdb/snarf-to:
> notmuch-address.el:122:29:Warning: reference to free variable
>     `bbdb-get-addresses-headers'
>
> In end of data:
> notmuch-address.el:143:1:Warning: the following functions are not known to be
>     defined: bbdb-get-addresses, bbdb-update-records, notmuch-show-get-header
>
> Do we need a few defvars?

For the above set, something like:

(defvar bbdb-get-addresses-headers)

(declare-function notmuch-show-get-header "notmuch-show" (header &optional props))

(declare-function bbdb-get-addresses "bbdb-com" 
  (only-first-address
   uninteresting-senders
   get-header-content-function
   &rest get-header-content-function-args))

(declare-function bbdb-update-records "bbdb-com" (addrs auto-create-p offer-to-create))

> d

Tomi

Re: [PATCH] emacs: add missing paren to fix defun in notmuch-address.el.

[Sebastian Lipp]

[-- Attachment #1: Type: text/plain, Size: 1242 bytes --]

Tomi Ollila <tomi.ollila@iki.fi> writes:
> On Tue, Apr 09 2013, David Bremner wrote:
>> There seems to be a few warnings:
>>
>> In notmuch-bbdb/snarf-from:
>> notmuch-address.el:116:26:Warning: reference to free variable
>>     `bbdb-get-addresses-headers'
>>
>> In notmuch-bbdb/snarf-to:
>> notmuch-address.el:122:29:Warning: reference to free variable
>>     `bbdb-get-addresses-headers'
>>
>> In end of data:
>> notmuch-address.el:143:1:Warning: the following functions are not known to be
>>     defined: bbdb-get-addresses, bbdb-update-records, notmuch-show-get-header
>>
>> Do we need a few defvars?
>
> For the above set, something like:
>
> (defvar bbdb-get-addresses-headers)
>
> (declare-function notmuch-show-get-header "notmuch-show" (header &optional props))
>
> (declare-function bbdb-get-addresses "bbdb-com" 
>   (only-first-address
>    uninteresting-senders
>    get-header-content-function
>    &rest get-header-content-function-args))
>
> (declare-function bbdb-update-records "bbdb-com" (addrs auto-create-p offer-to-create))

As I'd like to see this in notmuch I made the change. The patch is
attached. As it is my first contribution to notmuch at all: Just tell me
if I'm supposed to do it in any other way.

LG
basti


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-emacs-functions-to-import-sender-or-recipient-into-B.patch --]
[-- Type: text/x-diff, Size: 3028 bytes --]

From 522e4294258e6392a02c923b4b7e78a898986fca Mon Sep 17 00:00:00 2001
From: Daniel Bergey <bergey@alum.mit.edu>
Date: Mon, 8 Apr 2013 19:55:04 -0500
Subject: [PATCH] emacs: functions to import sender or recipient into BBDB [v3]

From a show buffer, notmuch-bbdb/snarf-from imports the sender into
bbdb.  notmuch-bbdb/snarf-to imports all recipients.  Newly imported
contacts are reported in the minibuffer / Messages buffer.

Both functions use the BBDB parser to recognize email address formats.

[v3] Fixes a few warnings as suggested by Tomi Ollila in
     id:87vc7vgbym.fsf@zancas.localnet
[v2] Fixes missing close parenthesis in original.
     Spotted by Karl Fogel <kfogel@red-bean.com>.
---
 emacs/notmuch-address.el | 53 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/emacs/notmuch-address.el b/emacs/notmuch-address.el
index fa65cd5..ee7b169 100644
--- a/emacs/notmuch-address.el
+++ b/emacs/notmuch-address.el
@@ -113,6 +113,59 @@ to know how address selection is made by default."
 (when (notmuch-address-locate-command notmuch-address-command)
   (notmuch-address-message-insinuate))
 
+;; functions to add sender / recipients to BBDB
+
+(defvar bbdb-get-addresses-headers)
+
+(declare-function notmuch-show-get-header "notmuch-show" (header &optional props))
+
+(declare-function bbdb-get-addresses "bbdb-com" 
+  (only-first-address
+   uninteresting-senders
+   get-header-content-function
+   &rest get-header-content-function-args))
+
+(declare-function bbdb-update-records "bbdb-com" (addrs auto-create-p offer-to-create))
+
+(defun notmuch-bbdb/snarf-headers (headers)
+  ;; Helper function to avoid code duplication in the two below
+  ;; headers should have the same format as bbdb-get-addresses-headers
+
+  ;; bbdb-get-addresses reads these
+  ;; Ugh, pass-by-global
+  (let ((addrs (bbdb-get-addresses nil nil 'notmuch-bbdb/get-header-content))
+	(bbdb-get-addresses-headers headers) ; headers to read
+	(bbdb-gag-messages t)) ; suppress m/n processed message)
+    (bbdb-update-records addrs t t)))
+
+(defun notmuch-bbdb/snarf-from ()
+  "Import the sender of the current message into BBDB"
+  (interactive)
+  (notmuch-bbdb/snarf-headers
+   (list (assoc 'authors bbdb-get-addresses-headers))))
+
+(defun notmuch-bbdb/snarf-to ()
+  "Import all recipients of the current message into BBDB"
+  (interactive)
+  (notmuch-bbdb/snarf-headers
+   (list (assoc 'recipients bbdb-get-addresses-headers))))
+
+(defvar notmuch-bbdb/header-by-name
+  ;; both are case sensitive
+  '( ("From" . :From)
+     ("To" . :To)
+     ("CC" . :Cc)
+     ("BCC" . :Bcc)
+     ("Resent-From" . nil)
+     ("Reply-To" . nil)
+     ("Resent-To" . nil)
+     ("Resent-CC" . nil))
+  "Alist for dispatching header symbols as used by notmuch-show-get-header
+from strings as used by bbdb-get-addresses")
+
+(defun notmuch-bbdb/get-header-content (name)
+  (notmuch-show-get-header (cdr (assoc name notmuch-bbdb/header-by-name))))
+
 ;;
 
 (provide 'notmuch-address)
-- 
2.0.1

Re: [PATCH] emacs: add missing paren to fix defun in notmuch-address.el.

[Karl Fogel]

Sebastian Lipp <bacuh@riseup.net> writes:
>Tomi Ollila <tomi.ollila@iki.fi> writes:
>> On Tue, Apr 09 2013, David Bremner wrote:
>>> There seems to be a few warnings:
>>>
>>> In notmuch-bbdb/snarf-from:
>>> notmuch-address.el:116:26:Warning: reference to free variable
>>>     `bbdb-get-addresses-headers'
>>>
>>> In notmuch-bbdb/snarf-to:
>>> notmuch-address.el:122:29:Warning: reference to free variable
>>>     `bbdb-get-addresses-headers'
>>>
>>> In end of data:
>>> notmuch-address.el:143:1:Warning: the following functions are not known to be
>>>     defined: bbdb-get-addresses, bbdb-update-records, notmuch-show-get-header
>>>
>>> Do we need a few defvars?
>>
>> For the above set, something like:
>>
>> (defvar bbdb-get-addresses-headers)
>>
>> (declare-function notmuch-show-get-header "notmuch-show" (header &optional props))
>>
>> (declare-function bbdb-get-addresses "bbdb-com" 
>>   (only-first-address
>>    uninteresting-senders
>>    get-header-content-function
>>    &rest get-header-content-function-args))
>>
>> (declare-function bbdb-update-records "bbdb-com" (addrs auto-create-p offer-to-create))
>
>As I'd like to see this in notmuch I made the change. The patch is
>attached. As it is my first contribution to notmuch at all: Just tell me
>if I'm supposed to do it in any other way.

I think your patch includes much more than just the above, though.

(It helps to include a log message with the patch -- then people can see
what you intended the code change to be, and compare that against the
code change the patch actually makes.)

>diff --git a/emacs/notmuch-address.el b/emacs/notmuch-address.el
>index fa65cd5..ee7b169 100644
>--- a/emacs/notmuch-address.el
>+++ b/emacs/notmuch-address.el
>@@ -113,6 +113,59 @@ to know how address selection is made by default."
> (when (notmuch-address-locate-command notmuch-address-command)
>   (notmuch-address-message-insinuate))
> 
>+;; functions to add sender / recipients to BBDB
>+
>+(defvar bbdb-get-addresses-headers)

I think it's good to include an initial value (even an invalid
placeholder one, if the real initialization has not happened yet), and a
doc string.  C-h f defvar RET will say more about how to do that.

(By the way, this isn't a user-customizeable variable, right?  If it
were, then `defcustom' would be better than `defvar'.)

>+(declare-function notmuch-show-get-header "notmuch-show" (header &optional props))
>+
>+(declare-function bbdb-get-addresses "bbdb-com" 
>+  (only-first-address
>+   uninteresting-senders
>+   get-header-content-function
>+   &rest get-header-content-function-args))
>+
>+(declare-function bbdb-update-records "bbdb-com" (addrs auto-create-p offer-to-create))

At this point, your patch has accomplished what Tomi originally
suggested.  But then the patch continues with what looks like
substantive new code:

>+(defun notmuch-bbdb/snarf-headers (headers)
>+  ;; Helper function to avoid code duplication in the two below
>+  ;; headers should have the same format as bbdb-get-addresses-headers
>+
>+  ;; bbdb-get-addresses reads these
>+  ;; Ugh, pass-by-global
>+  (let ((addrs (bbdb-get-addresses nil nil 'notmuch-bbdb/get-header-content))
>+	(bbdb-get-addresses-headers headers) ; headers to read
>+	(bbdb-gag-messages t)) ; suppress m/n processed message)
>+    (bbdb-update-records addrs t t)))
>+
>+(defun notmuch-bbdb/snarf-from ()
>+  "Import the sender of the current message into BBDB"
>+  (interactive)
>+  (notmuch-bbdb/snarf-headers
>+   (list (assoc 'authors bbdb-get-addresses-headers))))
>+
>+(defun notmuch-bbdb/snarf-to ()
>+  "Import all recipients of the current message into BBDB"
>+  (interactive)
>+  (notmuch-bbdb/snarf-headers
>+   (list (assoc 'recipients bbdb-get-addresses-headers))))
>+
>+(defvar notmuch-bbdb/header-by-name
>+  ;; both are case sensitive
>+  '( ("From" . :From)
>+     ("To" . :To)
>+     ("CC" . :Cc)
>+     ("BCC" . :Bcc)
>+     ("Resent-From" . nil)
>+     ("Reply-To" . nil)
>+     ("Resent-To" . nil)
>+     ("Resent-CC" . nil))
>+  "Alist for dispatching header symbols as used by notmuch-show-get-header
>+from strings as used by bbdb-get-addresses")
>+
>+(defun notmuch-bbdb/get-header-content (name)
>+  (notmuch-show-get-header (cdr (assoc name notmuch-bbdb/header-by-name))))

It looks like new code, but I suspect what actually happened is that
this is just the original code, somehow mis-expressed as "+" lines in
the patch.  Is that what happened?

Best,
-Karl

Re: [PATCH] emacs: add missing paren to fix defun in notmuch-address.el.

[Sebastian Lipp]

Karl Fogel <kfogel@red-bean.com> writes:
> Sebastian Lipp <bacuh@riseup.net> writes:
>>As I'd like to see this in notmuch I made the change. The patch is
>>attached. As it is my first contribution to notmuch at all: Just tell me
>>if I'm supposed to do it in any other way.
>
> I think your patch includes much more than just the above, though.

True. See below on that.

>>diff --git a/emacs/notmuch-address.el b/emacs/notmuch-address.el
>>index fa65cd5..ee7b169 100644
>>--- a/emacs/notmuch-address.el
>>+++ b/emacs/notmuch-address.el
>>@@ -113,6 +113,59 @@ to know how address selection is made by default."
>> (when (notmuch-address-locate-command notmuch-address-command)
>>   (notmuch-address-message-insinuate))
>> 
>>+;; functions to add sender / recipients to BBDB
>>+
>>+(defvar bbdb-get-addresses-headers)
>
> I think it's good to include an initial value (even an invalid
> placeholder one, if the real initialization has not happened yet), and a
> doc string.  C-h f defvar RET will say more about how to do that.

I will read into this and get back to it.

>>+(declare-function notmuch-show-get-header "notmuch-show" (header &optional props))
>>+
>>+(declare-function bbdb-get-addresses "bbdb-com" 
>>+  (only-first-address
>>+   uninteresting-senders
>>+   get-header-content-function
>>+   &rest get-header-content-function-args))
>>+
>>+(declare-function bbdb-update-records "bbdb-com" (addrs auto-create-p offer-to-create))
>
> At this point, your patch has accomplished what Tomi originally
> suggested.  But then the patch continues with what looks like
> substantive new code:
>
>>+(defun notmuch-bbdb/snarf-headers (headers)
>>+  ;; Helper function to avoid code duplication in the two below
>>+  ;; headers should have the same format as bbdb-get-addresses-headers
>>+
>>+  ;; bbdb-get-addresses reads these
>>+  ;; Ugh, pass-by-global
>>+  (let ((addrs (bbdb-get-addresses nil nil 'notmuch-bbdb/get-header-content))
>>+	(bbdb-get-addresses-headers headers) ; headers to read
>>+	(bbdb-gag-messages t)) ; suppress m/n processed message)
>>+    (bbdb-update-records addrs t t)))
>>+
>>+(defun notmuch-bbdb/snarf-from ()
>>+  "Import the sender of the current message into BBDB"
>>+  (interactive)
>>+  (notmuch-bbdb/snarf-headers
>>+   (list (assoc 'authors bbdb-get-addresses-headers))))
>>+
>>+(defun notmuch-bbdb/snarf-to ()
>>+  "Import all recipients of the current message into BBDB"
>>+  (interactive)
>>+  (notmuch-bbdb/snarf-headers
>>+   (list (assoc 'recipients bbdb-get-addresses-headers))))
>>+
>>+(defvar notmuch-bbdb/header-by-name
>>+  ;; both are case sensitive
>>+  '( ("From" . :From)
>>+     ("To" . :To)
>>+     ("CC" . :Cc)
>>+     ("BCC" . :Bcc)
>>+     ("Resent-From" . nil)
>>+     ("Reply-To" . nil)
>>+     ("Resent-To" . nil)
>>+     ("Resent-CC" . nil))
>>+  "Alist for dispatching header symbols as used by notmuch-show-get-header
>>+from strings as used by bbdb-get-addresses")
>>+
>>+(defun notmuch-bbdb/get-header-content (name)
>>+  (notmuch-show-get-header (cdr (assoc name notmuch-bbdb/header-by-name))))
>
> It looks like new code, but I suspect what actually happened is that
> this is just the original code, somehow mis-expressed as "+" lines in
> the patch.  Is that what happened?

No, it happened by intention because what is "original" to you is not
part of my freshly cloned notmuch. So I thought good practise is to
construct *one* patch that brings the already fixed feature to the
notmuch codebase to keep it clean.

If you like better, my next patch will only base Tomis and your
suggestions on top of the "original" patch.

basti

Re: [PATCH] emacs: add missing paren to fix defun in notmuch-address.el.

[Tomi Ollila]

On Fri, Jul 04 2014, Sebastian Lipp <bacuh@riseup.net> wrote:

> Tomi Ollila <tomi.ollila@iki.fi> writes:
>> On Tue, Apr 09 2013, David Bremner wrote:
>>> There seems to be a few warnings:
>>>
>>> In notmuch-bbdb/snarf-from:
>>> notmuch-address.el:116:26:Warning: reference to free variable
>>>     `bbdb-get-addresses-headers'
>>>
>>> In notmuch-bbdb/snarf-to:
>>> notmuch-address.el:122:29:Warning: reference to free variable
>>>     `bbdb-get-addresses-headers'
>>>
>>> In end of data:
>>> notmuch-address.el:143:1:Warning: the following functions are not known to be
>>>     defined: bbdb-get-addresses, bbdb-update-records, notmuch-show-get-header
>>>
>>> Do we need a few defvars?
>>
>> For the above set, something like:
>>
>> (defvar bbdb-get-addresses-headers)
>>
>> (declare-function notmuch-show-get-header "notmuch-show" (header &optional props))
>>
>> (declare-function bbdb-get-addresses "bbdb-com" 
>>   (only-first-address
>>    uninteresting-senders
>>    get-header-content-function
>>    &rest get-header-content-function-args))
>>
>> (declare-function bbdb-update-records "bbdb-com" (addrs auto-create-p offer-to-create))
>
> As I'd like to see this in notmuch I made the change. The patch is
> attached. As it is my first contribution to notmuch at all: Just tell me
> if I'm supposed to do it in any other way.
>
> LG
> basti

I saved this patch with 'w' in emacs mua -- and have to remove '>' From the
first line so that 'git am' could recognize it. 

There was one whitespace error -- I reapplied with

git am --whitespace=fix ~/0001-emacs-functions-to-import-sender-or-recipient-into-B.patch

more inline (in addition to Karl's good comments)

IMO all of that can be in one patch...

>From 522e4294258e6392a02c923b4b7e78a898986fca Mon Sep 17 00:00:00 2001
> From: Daniel Bergey <bergey@alum.mit.edu>
> Date: Mon, 8 Apr 2013 19:55:04 -0500
> Subject: [PATCH] emacs: functions to import sender or recipient into BBDB [v3]
>
>From a show buffer, notmuch-bbdb/snarf-from imports the sender into

Interestingly the line above was lost when I applied the patch -- 
probably '>From' somehow borke git am... -- should git am be more capable
there (or would the required heuristics pass the tolerance threshold) ?

> bbdb.  notmuch-bbdb/snarf-to imports all recipients.  Newly imported
> contacts are reported in the minibuffer / Messages buffer.
>
> Both functions use the BBDB parser to recognize email address formats.
>
> [v3] Fixes a few warnings as suggested by Tomi Ollila in
>      id:87vc7vgbym.fsf@zancas.localnet
> [v2] Fixes missing close parenthesis in original.
>      Spotted by Karl Fogel <kfogel@red-bean.com>.
> ---
>  emacs/notmuch-address.el | 53 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
>
> diff --git a/emacs/notmuch-address.el b/emacs/notmuch-address.el
> index fa65cd5..ee7b169 100644
> --- a/emacs/notmuch-address.el
> +++ b/emacs/notmuch-address.el
> @@ -113,6 +113,59 @@ to know how address selection is made by default."
>  (when (notmuch-address-locate-command notmuch-address-command)
>    (notmuch-address-message-insinuate))
>  
> +;; functions to add sender / recipients to BBDB
> +
> +(defvar bbdb-get-addresses-headers)

It would be nice to have comment where bbdb-get-addresses-headers is defined.

> +
> +(declare-function notmuch-show-get-header "notmuch-show" (header &optional props))
> +
> +(declare-function bbdb-get-addresses "bbdb-com" 
> +  (only-first-address
> +   uninteresting-senders
> +   get-header-content-function
> +   &rest get-header-content-function-args))
> +
> +(declare-function bbdb-update-records "bbdb-com" (addrs auto-create-p offer-to-create))
> +
> +(defun notmuch-bbdb/snarf-headers (headers)
> +  ;; Helper function to avoid code duplication in the two below
> +  ;; headers should have the same format as bbdb-get-addresses-headers
> +
> +  ;; bbdb-get-addresses reads these
> +  ;; Ugh, pass-by-global

Somehow this comment section looks a bit odd -- is it the empty line there,
or just that (at least part of) it is not docstring.

> +  (let ((addrs (bbdb-get-addresses nil nil 'notmuch-bbdb/get-header-content))
> +	(bbdb-get-addresses-headers headers) ; headers to read
> +	(bbdb-gag-messages t)) ; suppress m/n processed message)
> +    (bbdb-update-records addrs t t)))
> +
> +(defun notmuch-bbdb/snarf-from ()
> +  "Import the sender of the current message into BBDB"
> +  (interactive)
> +  (notmuch-bbdb/snarf-headers
> +   (list (assoc 'authors bbdb-get-addresses-headers))))
> +
> +(defun notmuch-bbdb/snarf-to ()
> +  "Import all recipients of the current message into BBDB"
> +  (interactive)
> +  (notmuch-bbdb/snarf-headers
> +   (list (assoc 'recipients bbdb-get-addresses-headers))))
> +
> +(defvar notmuch-bbdb/header-by-name
> +  ;; both are case sensitive
> +  '( ("From" . :From)
> +     ("To" . :To)
> +     ("CC" . :Cc)
> +     ("BCC" . :Bcc)
> +     ("Resent-From" . nil)
> +     ("Reply-To" . nil)
> +     ("Resent-To" . nil)
> +     ("Resent-CC" . nil))
> +  "Alist for dispatching header symbols as used by notmuch-show-get-header
> +from strings as used by bbdb-get-addresses")
> +
> +(defun notmuch-bbdb/get-header-content (name)
> +  (notmuch-show-get-header (cdr (assoc name notmuch-bbdb/header-by-name))))
> +
>  ;;
>  
>  (provide 'notmuch-address)
> -- 
> 2.0.1

Tomi

Re: [PATCH] emacs: add missing paren to fix defun in notmuch-address.el.

[Karl Fogel]

Sebastian Lipp <bacuh@riseup.net> writes:
>No, it happened by intention because what is "original" to you is not
>part of my freshly cloned notmuch. So I thought good practise is to
>construct *one* patch that brings the already fixed feature to the
>notmuch codebase to keep it clean.
>
>If you like better, my next patch will only base Tomis and your
>suggestions on top of the "original" patch.

Oh, I had thought the changes were already in the notmuch tree.  Now I
understand what you're saying, and yes, it makes sense.

In general, supplying a log message with the patch with avoid such
confusion.  If there is some prose expressing what the change is
supposed to to, and giving any historical context (such as the mailing
list thread starting from last year), then it will be easy for any
reviewer to understand what the patch is intended to do, and check if it
actually does that.

Best,
-Karl

Re: [PATCH] emacs: add missing paren to fix defun in notmuch-address.el.

[Sebastian Lipp]

Karl Fogel <kfogel@red-bean.com> writes:
> Sebastian Lipp <bacuh@riseup.net> writes:
>>diff --git a/emacs/notmuch-address.el b/emacs/notmuch-address.el
>>index fa65cd5..ee7b169 100644
>>--- a/emacs/notmuch-address.el
>>+++ b/emacs/notmuch-address.el
>>@@ -113,6 +113,59 @@ to know how address selection is made by default."
>> (when (notmuch-address-locate-command notmuch-address-command)
>>   (notmuch-address-message-insinuate))
>> 
>>+;; functions to add sender / recipients to BBDB
>>+
>>+(defvar bbdb-get-addresses-headers)
>
> I think it's good to include an initial value (even an invalid
> placeholder one, if the real initialization has not happened yet), and a
> doc string.  C-h f defvar RET will say more about how to do that.

I hope I got that right now. (I've got no real clue of Lisp yet because
I just recently switched to emacs partly because of notmuch. :)

How about

    (defvar bbdb-get-addresses-headers nil
      "List of Addresses to import into bbdb")

?

> (By the way, this isn't a user-customizeable variable, right?  If it
> were, then `defcustom' would be better than `defvar'.)

As far as I understand it: It's not.

Re: [PATCH] emacs: add missing paren to fix defun in notmuch-address.el.

[Sebastian Lipp]

Karl Fogel <kfogel@red-bean.com>
> In general, supplying a log message with the patch with avoid such
> confusion.  If there is some prose expressing what the change is
> supposed to to, and giving any historical context (such as the mailing
> list thread starting from last year), then it will be easy for any
> reviewer to understand what the patch is intended to do, and check if it
> actually does that.

Okay, next time I will provide that information inline. Thought keeping
References and In-Reply-To headers would be sufficient reference.

Re: [PATCH] emacs: add missing paren to fix defun in notmuch-address.el.

[Karl Fogel]

Sebastian Lipp <bacuh@riseup.net> writes:
>Karl Fogel <kfogel@red-bean.com>
>> In general, supplying a log message with the patch with avoid such
>> confusion.  If there is some prose expressing what the change is
>> supposed to to, and giving any historical context (such as the mailing
>> list thread starting from last year), then it will be easy for any
>> reviewer to understand what the patch is intended to do, and check if it
>> actually does that.
>
>Okay, next time I will provide that information inline. Thought keeping
>References and In-Reply-To headers would be sufficient reference.

Well, when the patch is committed into a version control system, it's
going to need a log message (commit message) anyway.  Since that message
is considered part of the change -- anyone reading the change will start
by reading the commit message -- it's typical to just include it with
the diff.

The commit message can certainly include a reference to the email
thread; in fact, it's good if it does so.

Best,
­Karl

Re: [PATCH] emacs: add missing paren to fix defun in notmuch-address.el.

[Karl Fogel]

Sebastian Lipp <bacuh@riseup.net> writes:
>> I think it's good to include an initial value (even an invalid
>> placeholder one, if the real initialization has not happened yet), and a
>> doc string.  C-h f defvar RET will say more about how to do that.
>
>I hope I got that right now. (I've got no real clue of Lisp yet because
>I just recently switched to emacs partly because of notmuch. :)
>
>How about
>
>    (defvar bbdb-get-addresses-headers nil
>      "List of Addresses to import into bbdb")

That's interesting -- from seeing the variable's name, I would not have
guessed that it is a list.

Can the doc string describe the exact format of the list, and something
about how the list is initialized and used?

In other words, if you were a programmer seeing this variable for the
first time, and you had only a basic working knowledge of what the
notmuch Elisp code does, what would you want to see in this doc string
to give you a good understanding of what this variable is, how it gets
set, and how it's used?

One test I often use for myself is that if after I've read the doc
string, I can look at any value and say whether or not it is a valid
value that this variable might hold (when the code is running), then the
doc string has done its job.

Best,
-Karl

Re: [PATCH] emacs: add missing paren to fix defun in notmuch-address.el.

[Tomi Ollila]

On Sun, Jul 06 2014, Sebastian Lipp <bacuh@riseup.net> wrote:

> Karl Fogel <kfogel@red-bean.com> writes:
>> Sebastian Lipp <bacuh@riseup.net> writes:
>>>diff --git a/emacs/notmuch-address.el b/emacs/notmuch-address.el
>>>index fa65cd5..ee7b169 100644
>>>--- a/emacs/notmuch-address.el
>>>+++ b/emacs/notmuch-address.el
>>>@@ -113,6 +113,59 @@ to know how address selection is made by default."
>>> (when (notmuch-address-locate-command notmuch-address-command)
>>>   (notmuch-address-message-insinuate))
>>> 
>>>+;; functions to add sender / recipients to BBDB
>>>+
>>>+(defvar bbdb-get-addresses-headers)
>>
>> I think it's good to include an initial value (even an invalid
>> placeholder one, if the real initialization has not happened yet), and a
>> doc string.  C-h f defvar RET will say more about how to do that.
>
> I hope I got that right now. (I've got no real clue of Lisp yet because
> I just recently switched to emacs partly because of notmuch. :)
>
> How about
>
>     (defvar bbdb-get-addresses-headers nil
>       "List of Addresses to import into bbdb")
>
> ?
>
>> (By the way, this isn't a user-customizeable variable, right?  If it
>> were, then `defcustom' would be better than `defvar'.)
>
> As far as I understand it: It's not.

someone(tm) could explain why or why-not this is defvar and not defcustom
(or wice versa) so that I don't have to spend time digging into it

but the bbdb-* looks a bit too generic (goes deep into bbdb "namespace"
in a file not part of bbdb -package)


Tomi

Re: [PATCH] emacs: add missing paren to fix defun in notmuch-address.el.

[Karl Fogel]

Tomi Ollila <tomi.ollila@iki.fi> writes:
>but the bbdb-* looks a bit too generic (goes deep into bbdb "namespace"
>in a file not part of bbdb -package)

Good point.  Stuff in notmuch should have a consistent prefix.  Prefixes
are Emacs' module system, after all :-).  (It could be `notmuch-bbdb-*'
or whatever, of course.)