From: Mark Walters <markwalters1009@gmail.com>
To: Peter Wang <novalazy@gmail.com>, notmuch@notmuchmail.org
Subject: Re: [PATCH v2 00/20] insert command
In-Reply-To: <1353806206-29133-1-git-send-email-novalazy@gmail.com>
References: <1353806206-29133-1-git-send-email-novalazy@gmail.com>
User-Agent: Notmuch/0.14+81~g9730584 (http://notmuchmail.org) Emacs/23.4.1
	(x86_64-pc-linux-gnu)
Date: Sun, 25 Nov 2012 16:11:12 +0000
Message-ID: <87boelk6hr.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list


This is series is looking good. I have a few minor issues but no more.

There are two parts I don't really know enough about to be certain
about: the fsyncing and the sigtrap bit. 

For fsync: I have some recollection that you have to fsync all subpaths
to the root to guarantee that it makes it to the disk. 

One in particular that may need more fsyncing is if you create a folder
A/B/C/D/E/ for the messages then I think you only fsync the message and
A/B/C/D/E/new.

My other comments are all very minor and are made in individual replies.

Best wishes

Mark


On Sun, 25 Nov 2012, Peter Wang <novalazy@gmail.com> wrote:
> This series mainly addresses the issues raised by Mark:
>
> - check talloc failures
> - deadlock in maildir_open_tmp
> - stricter file modes (0600 and 0700)
> - shared tag operation parser with notmuch-tag.c
> - simplified mkdir_parents
> - trap SIGINT
> - fsync after writing and rename
> - added a couple of tests
> - man page wording
> - comments
>
> Due to new restriction on tags beginning with '-', an argument beginning with
> "--" is no longer ambiguous so I have removed the optional "--" separator
> between options and tag operations.
>
> Peter Wang (20):
>   tag: factor out tag operation parsing
>   tag: make tag operation parser public
>   cli: add stub for insert command
>   insert: open Maildir tmp file
>   insert: copy stdin to Maildir tmp file
>   insert: move file from Maildir tmp to new
>   insert: add new message to database
>   insert: support --folder option
>   insert: prevent writes outside Maildir hierarchy
>   insert: apply default tags to new message
>   insert: parse command-line tag operations
>   insert: apply command-line tag operations
>   insert: add --create-folder option
>   insert: fsync after writing tmp file
>   insert: fsync new directory after rename
>   insert: trap SIGINT and clean up
>   insert: add copyright line from notmuch-deliver
>   test: add tests for insert
>   man: document 'insert' command
>   man: reference notmuch-insert.1
>
>  Makefile.local                  |   1 +
>  man/Makefile.local              |   1 +
>  man/man1/notmuch-config.1       |   4 +-
>  man/man1/notmuch-count.1        |   4 +-
>  man/man1/notmuch-dump.1         |   4 +-
>  man/man1/notmuch-insert.1       |  60 +++++
>  man/man1/notmuch-new.1          |   4 +-
>  man/man1/notmuch-reply.1        |   3 +-
>  man/man1/notmuch-restore.1      |   3 +-
>  man/man1/notmuch-search.1       |   3 +-
>  man/man1/notmuch-show.1         |   3 +-
>  man/man1/notmuch-tag.1          |   3 +-
>  man/man1/notmuch.1              |   3 +-
>  man/man5/notmuch-hooks.5        |   4 +-
>  man/man7/notmuch-search-terms.7 |   3 +-
>  notmuch-client.h                |  12 +
>  notmuch-insert.c                | 493 ++++++++++++++++++++++++++++++++++++++++
>  notmuch-tag.c                   |  79 ++++---
>  notmuch.c                       |   3 +
>  test/insert                     |  93 ++++++++
>  test/notmuch-test               |   1 +
>  21 files changed, 733 insertions(+), 51 deletions(-)
>  create mode 100644 man/man1/notmuch-insert.1
>  create mode 100644 notmuch-insert.c
>  create mode 100755 test/insert
>
> -- 
> 1.7.12.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch