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