From: Tomi Ollila <tomi.ollila@iki.fi>
To: Mark Walters <markwalters1009@gmail.com>, notmuch@notmuchmail.org
Subject: Re: [PATCH 00/11] Move notmuch-tree to mainline
In-Reply-To: <1383087338-10220-1-git-send-email-markwalters1009@gmail.com>
References: <1383087338-10220-1-git-send-email-markwalters1009@gmail.com>
User-Agent: Notmuch/0.16+119~g219c55f (http://notmuchmail.org) Emacs/24.3.1
	(x86_64-unknown-linux-gnu)
Date: Wed, 30 Oct 2013 20:01:51 +0200
Message-ID: <m2bo26eiy8.fsf@guru.guru-group.fi>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: list

On Wed, Oct 30 2013, Mark Walters <markwalters1009@gmail.com> wrote:

> This is a more complete version of the move of pick/tree to
> mainline. The previous version was at
> id:1382257699-29860-1-git-send-email-markwalters1009@gmail.com

This series looks pretty good to me. IMO patches 1-6 are ready...

> The previous version did not get the autoloading right: it all worked
> on my setup as I had stale files around in my tree which were loaded.
> This versions builds and tests pass at each stage on a fresh clone.

You and Me seem to have different understanding what "autoload"ing means.
To me autoloading means using (autoload ...) function. In patch 7
( id:1383087338-10220-8-git-send-email-markwalters1009@gmail.com )
(require 'notmuch-tree) is used.

To me it is fine if this approach is taken, but then calling it autoloading
in commit message (in patches 7 and 8) is confusing.

However, if (autoload ...) syntax were actually used, it could be done
the somewhat the following way:

* Don't add (require 'notmuch-tree) in notmuch.el (Patch 7) 

* Add (autoload 'notmuch-tree-from-search-current-query "notmuch-tree" "docstring" t)
  to notmuch.el (maybe we can live without declare-function ?)

* Add (autoload 'notmuch-tree "notmuch-tree" "docstring" t) to 
  notmuch-lib.el.

* Now Patch 9 is good as it is now.

* Don't move the functions mentioned in Patch 9 from notmuch-tree.el to
  notmuch.el but write autoload (for notmuch-tree-from-search-thread) to
  notmuch.el

* Now Patch 10 is good as it is now.

* Leave notmuch-tree-from-show-current-query in notmuch-tree.el, and add
  autoload for it in notmuch-show.el. Now you can also drop 
  (declare-function notmuch-tree ...) out from notmuch-show.el (Patch 11).


Tomi


> The diffstat at the bottom of this message is huge, so I include the
> rather more informative output from git diff -stat -M here.
>
> Apart from the first patch (which moves 80 lines from notmuch.el to notmuch-lib.el) the total is:
>  emacs/Makefile.local                               |    1 +
>  emacs/notmuch-lib.el                               |    1 +
>  emacs/notmuch-show.el                              |   10 ++++
>  {contrib/notmuch-pick => emacs}/notmuch-tree.el    |   47 ++-----------------
>  emacs/notmuch.el                                   |   16 +++++++
>  {contrib/notmuch-pick/test => test}/emacs-tree     |   13 -----
>  test/notmuch-test                                  |    1 +
>  .../tree.expected-output/notmuch-tree-show-window  |    0
>  .../notmuch-tree-single-thread                     |    0
>  .../tree.expected-output/notmuch-tree-tag-inbox    |    0
>  .../notmuch-tree-tag-inbox-tagged                  |    0
>  .../notmuch-tree-tag-inbox-thread-tagged           |    0
>  12 files changed, 34 insertions(+), 55 deletions(-)
>
>
> Obviously this clashes with the remap/help series at
> id:1382821479-23384-1-git-send-email-markwalters1009@gmail.com but
> rebasing either way would be easy.
>
> Best wishes
>
> Mark
>
>
>
>
>
> Mark Walters (11):
>   emacs: move notmuch-help to lib
>   emacs: tree: remove unneeded declarations
>   emacs: move notmuch-tree from contrib to mainline
>   emacs: add tree to the makefile
>   test: move emacs-tree test into mainline
>   emacs: tree: remove test for emacs from tree test
>   emacs: minimal change to autoload notmuch-tree
>   test: tree: remove require from tests
>   emacs: move search based tree functions to notmuch.el
>   emacs: add z to common keymap
>   emacs: move the show entry to tree into show.el
>
>  contrib/notmuch-pick/notmuch-tree.el               |  951 --------------------
>  contrib/notmuch-pick/test/emacs-tree               |  210 -----
>  .../tree.expected-output/notmuch-tree-show-window  |   40 -
>  .../notmuch-tree-single-thread                     |    6 -
>  .../tree.expected-output/notmuch-tree-tag-inbox    |   53 --
>  .../notmuch-tree-tag-inbox-tagged                  |   53 --
>  .../notmuch-tree-tag-inbox-thread-tagged           |   53 --
>  emacs/Makefile.local                               |    1 +
>  emacs/notmuch-lib.el                               |   88 ++
>  emacs/notmuch-show.el                              |   10 +
>  emacs/notmuch-tree.el                              |  914 +++++++++++++++++++
>  emacs/notmuch.el                                   |  103 +--
>  test/emacs-tree                                    |  197 ++++
>  test/notmuch-test                                  |    1 +
>  test/tree.expected-output/notmuch-tree-show-window |   40 +
>  .../notmuch-tree-single-thread                     |    6 +
>  test/tree.expected-output/notmuch-tree-tag-inbox   |   53 ++
>  .../notmuch-tree-tag-inbox-tagged                  |   53 ++
>  .../notmuch-tree-tag-inbox-thread-tagged           |   53 ++
>  19 files changed, 1432 insertions(+), 1453 deletions(-)
>  delete mode 100644 contrib/notmuch-pick/notmuch-tree.el
>  delete mode 100755 contrib/notmuch-pick/test/emacs-tree
>  delete mode 100644 contrib/notmuch-pick/test/tree.expected-output/notmuch-tree-show-window
>  delete mode 100644 contrib/notmuch-pick/test/tree.expected-output/notmuch-tree-single-thread
>  delete mode 100644 contrib/notmuch-pick/test/tree.expected-output/notmuch-tree-tag-inbox
>  delete mode 100644 contrib/notmuch-pick/test/tree.expected-output/notmuch-tree-tag-inbox-tagged
>  delete mode 100644 contrib/notmuch-pick/test/tree.expected-output/notmuch-tree-tag-inbox-thread-tagged
>  create mode 100644 emacs/notmuch-tree.el
>  create mode 100755 test/emacs-tree
>  create mode 100644 test/tree.expected-output/notmuch-tree-show-window
>  create mode 100644 test/tree.expected-output/notmuch-tree-single-thread
>  create mode 100644 test/tree.expected-output/notmuch-tree-tag-inbox
>  create mode 100644 test/tree.expected-output/notmuch-tree-tag-inbox-tagged
>  create mode 100644 test/tree.expected-output/notmuch-tree-tag-inbox-thread-tagged
>
> -- 
> 1.7.9.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch