Re: pull request [was Re: pull request]

[Carl Worth <cworth@cworth.org>]

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

On Sat, 03 Apr 2010 07:32:44 +0100, David Edmondson <dme@dme.org> wrote:
> * commit a9590dfb4efc2c05a35948ef4522c362eb788c10
> | Author: David Edmondson <dme@dme.org>
> | Date:   Thu Apr 1 11:38:30 2010 +0100
> | 
> |     Makefile: Add the emacs directory to load-path when compiling

That's a nice one-line summary of the commit that says "what" the patch
does just fine. But the commit is missing the rest of the commit message
that should give the "why".

What's the motivation of the change? Is something perhaps broken without
this? Or is this a preparation for something else that will be coming
along in a future commit?

From looking at the next commit, I assume this is to enable the
"(require 'notmuch-show)" that is being added subsequently. So I've just
noted that.

> * commit 4de9f3f09e998d7312be2a1c08526e59bbf135a9
> | Author: David Edmondson <dme@dme.org>
> | Date:   Sun Mar 21 09:54:08 2010 +0000
> | 
> |     emacs/Makefile.local: Use makefile mode

I added similar treatment to the other instances of files named
Makefile.local.

> * commit 94893f25d36aaf43487e111fbfba4f7dae808dd2
> | Author: David Edmondson <dme@dme.org>
> | Date:   Tue Mar 23 07:04:34 2010 +0000
> | 
> |     emacs/notmuch.el: Improve tag highlighting in search mode
> |     
> |     Assume that tags never include an opening bracket, and hence improve
> |     the regular expression used to highlight them. This avoids false
> |     matches where the 'from' address of a thread participant includes an
> |     opening bracket.

Thanks. That's a good fix.

The above are all pushed.

> * commit f7ecad654fd8d0274fc75833d92117c8e496bcea
> | Author: David Edmondson <dme@dme.org>
> | Date:   Thu Apr 1 18:36:21 2010 +0100
> | 
> |     emacs: Move notmuch-show functionality to notmuch-show.el
> |     
> |     To ease the transition to a JSON based implementation of
> |     `notmuch-show', move the current implementation into a separate file.

This is definitely a nice improvement in modularization. But there are
some aspects of doing multiple-file emacs code that I'm unclear on.

If I apply this patch as is, then when compiling the notmuch-show.el I
get the following warnings:

  In notmuch-show:
  notmuch-show.el:969:34:Warning: reference to free variable `notmuch-command'

  In end of data:
  notmuch-show.el:983:1:Warning: the following functions are not known to be
      defined: point-invisible-p, mail-header-extract-no-properties,
      notmuch-select-tag-with-completion, union, intersection, set-difference,
      notmuch-search-show-thread, mm-display-parts, mm-dissect-buffer,
      notmuch-save-attachments, notmuch-count-attachments, notmuch-reply,
      mm-handle-type, mm-display-part, notmuch-fontify-headers

I can eliminate a few of these by copying the various require calls from
notmuch.el to notmuch-show.el, but that still leaves problems for all of
the functionality defined in notmuch.el and referenced in
notmuch-show.el:

  In notmuch-show:
  notmuch-show.el:973:34:Warning: reference to free variable `notmuch-command'

  In end of data:
  notmuch-show.el:987:1:Warning: the following functions are not known to be
      defined: point-invisible-p, notmuch-select-tag-with-completion,
      notmuch-search-show-thread, notmuch-save-attachments,
      notmuch-count-attachments, notmuch-reply, notmuch-fontify-headers

Does anyone know the right way to fix this? I'd like to get the output
clean as I plan to move the compilation of the emacs code from "make
install-emacs" to "make", (made conditional on a check for the presence
of emacs by configure).

So I haven't merged this commit yet.

-Carl

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]