[PATCH 00/11] notmuch insert updates
On Thu, Sep 25 2014, David Bremner wrote: > Jani Nikula writes: > >> This series refactors and cleans up insert, improves error handling and >> reporting, and adds post-insert hook. I intend to add documentation and >> more tests, but the code is ready for review. Also, at least some of the >> cleanups and fixes in the beginning of the series could go in without >> additional tests or documentation. > > The first 4 seem trivial, and I marked them so. > > 5-8 don't introduce new functionality, and don't break > any existing tests. Subject to a little more review, these could > probably be merged now. > > 5 is almost trivial; it wouldn't hurt to explicitly document the return > values of sync_dir, while touching this code. > > 6 and 8 seem ok, but should probably have another pair of eyes. > > 7 seems ok. At first I worried about the usual issue of races between > name definition and open, but it seems like nothing changes here. 5-8 in this series LGTM. Tomi
[PATCH 1/5] cli: Refactor option passing in the search command
On Thu, Sep 25 2014, Tomi Ollila wrote: > On Mon, Sep 22 2014, Michal Sojka wrote: > >> Many functions that implement the search command need to access command >> line options. Instead of passing each option in a separate variable, put >> them in a structure and pass only this structure. > > This patch looks good to me. Thanks for the review. > Although the test and the implementation in the next patches look OK, I'd > prefer the FLAG implementation Jani suggested earlier. IMO now that I > compare these two it looks cleaner and simpler... The question is which kind of simplicity you have in mind. I think that my version is simpler to type (less keystrokes). But if others have different opinion, I don't mind. > I.e. I'd prefer notmuch search --output=sender --output=recipients ... > (same output regardless the order these options given). This should be the case with both implementations. > I'd postpone the unique handling to a bit later phase; there are quite a > few options how to do that (*) > > > Tomi > > (*) IMO the default unique (when requested) would be exact case-sensitive > match of full name & address Why do you think that case-sensitive address matching should be the default? In theory local-part can be case sensitive, but I've never seen that in reality. So this default would only be useful if you want to research how people type your email address :) > parts (phrase, address & comment); What do you mean by phrase and comment? Address syntax is defined by http://tools.ietf.org/html/rfc5322#section-3.4.1. > then (a subset of possible) options could be: >+) case-insensitive (first match taken (or last match?) -- option?) >+) unique email addresses (take phrase/comment from first/last?) > -- or use first that has something additional to plain address > -- or use last that has something additional to plain address Yes, there is a lot of possible options. I don't think that notmuch has to support all of them. If people need something special like "use last that has something additional to plain address", they can always do --unique=none and do their own post-processing. -Michal
[PATCH 1/5] cli: Refactor option passing in the search command
On Mon, Sep 22 2014, Michal Sojka wrote: > Many functions that implement the search command need to access command > line options. Instead of passing each option in a separate variable, put > them in a structure and pass only this structure. This patch looks good to me. Although the test and the implementation in the next patches look OK, I'd prefer the FLAG implementation Jani suggested earlier. IMO now that I compare these two it looks cleaner and simpler... I.e. I'd prefer notmuch search --output=sender --output=recipients ... (same output regardless the order these options given). I'd postpone the unique handling to a bit later phase; there are quite a few options how to do that (*) Tomi (*) IMO the default unique (when requested) would be exact case-sensitive match of full name & address parts (phrase, address & comment); then (a subset of possible) options could be: +) case-insensitive (first match taken (or last match?) -- option?) +) unique email addresses (take phrase/comment from first/last?) -- or use first that has something additional to plain address -- or use last that has something additional to plain address > This will become handy in the following patches. > --- > notmuch-search.c | 122 > --- > 1 file changed, 62 insertions(+), 60 deletions(-) > > diff --git a/notmuch-search.c b/notmuch-search.c > index bc9be45..5ac2a26 100644 > --- a/notmuch-search.c > +++ b/notmuch-search.c
[PATCH 00/11] notmuch insert updates
Jani Nikula writes: > This series refactors and cleans up insert, improves error handling and > reporting, and adds post-insert hook. I intend to add documentation and > more tests, but the code is ready for review. Also, at least some of the > cleanups and fixes in the beginning of the series could go in without > additional tests or documentation. The first 4 seem trivial, and I marked them so. 5-8 don't introduce new functionality, and don't break any existing tests. Subject to a little more review, these could probably be merged now. 5 is almost trivial; it wouldn't hurt to explicitly document the return values of sync_dir, while touching this code. 6 and 8 seem ok, but should probably have another pair of eyes. 7 seems ok. At first I worried about the usual issue of races between name definition and open, but it seems like nothing changes here.
Re: [PATCH 00/11] notmuch insert updates
Jani Nikula j...@nikula.org writes: This series refactors and cleans up insert, improves error handling and reporting, and adds post-insert hook. I intend to add documentation and more tests, but the code is ready for review. Also, at least some of the cleanups and fixes in the beginning of the series could go in without additional tests or documentation. The first 4 seem trivial, and I marked them so. 5-8 don't introduce new functionality, and don't break any existing tests. Subject to a little more review, these could probably be merged now. 5 is almost trivial; it wouldn't hurt to explicitly document the return values of sync_dir, while touching this code. 6 and 8 seem ok, but should probably have another pair of eyes. 7 seems ok. At first I worried about the usual issue of races between name definition and open, but it seems like nothing changes here. ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH 1/5] cli: Refactor option passing in the search command
On Mon, Sep 22 2014, Michal Sojka sojk...@fel.cvut.cz wrote: Many functions that implement the search command need to access command line options. Instead of passing each option in a separate variable, put them in a structure and pass only this structure. This patch looks good to me. Although the test and the implementation in the next patches look OK, I'd prefer the FLAG implementation Jani suggested earlier. IMO now that I compare these two it looks cleaner and simpler... I.e. I'd prefer notmuch search --output=sender --output=recipients ... (same output regardless the order these options given). I'd postpone the unique handling to a bit later phase; there are quite a few options how to do that (*) Tomi (*) IMO the default unique (when requested) would be exact case-sensitive match of full name address parts (phrase, address comment); then (a subset of possible) options could be: +) case-insensitive (first match taken (or last match?) -- option?) +) unique email addresses (take phrase/comment from first/last?) -- or use first that has something additional to plain address -- or use last that has something additional to plain address This will become handy in the following patches. --- notmuch-search.c | 122 --- 1 file changed, 62 insertions(+), 60 deletions(-) diff --git a/notmuch-search.c b/notmuch-search.c index bc9be45..5ac2a26 100644 --- a/notmuch-search.c +++ b/notmuch-search.c ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH 00/11] notmuch insert updates
On Thu, Sep 25 2014, David Bremner da...@tethera.net wrote: Jani Nikula j...@nikula.org writes: This series refactors and cleans up insert, improves error handling and reporting, and adds post-insert hook. I intend to add documentation and more tests, but the code is ready for review. Also, at least some of the cleanups and fixes in the beginning of the series could go in without additional tests or documentation. The first 4 seem trivial, and I marked them so. 5-8 don't introduce new functionality, and don't break any existing tests. Subject to a little more review, these could probably be merged now. 5 is almost trivial; it wouldn't hurt to explicitly document the return values of sync_dir, while touching this code. 6 and 8 seem ok, but should probably have another pair of eyes. 7 seems ok. At first I worried about the usual issue of races between name definition and open, but it seems like nothing changes here. 5-8 in this series LGTM. Tomi ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH 1/5] cli: Refactor option passing in the search command
On Thu, Sep 25 2014, Tomi Ollila wrote: On Mon, Sep 22 2014, Michal Sojka sojk...@fel.cvut.cz wrote: Many functions that implement the search command need to access command line options. Instead of passing each option in a separate variable, put them in a structure and pass only this structure. This patch looks good to me. Thanks for the review. Although the test and the implementation in the next patches look OK, I'd prefer the FLAG implementation Jani suggested earlier. IMO now that I compare these two it looks cleaner and simpler... The question is which kind of simplicity you have in mind. I think that my version is simpler to type (less keystrokes). But if others have different opinion, I don't mind. I.e. I'd prefer notmuch search --output=sender --output=recipients ... (same output regardless the order these options given). This should be the case with both implementations. I'd postpone the unique handling to a bit later phase; there are quite a few options how to do that (*) Tomi (*) IMO the default unique (when requested) would be exact case-sensitive match of full name address Why do you think that case-sensitive address matching should be the default? In theory local-part can be case sensitive, but I've never seen that in reality. So this default would only be useful if you want to research how people type your email address :) parts (phrase, address comment); What do you mean by phrase and comment? Address syntax is defined by http://tools.ietf.org/html/rfc5322#section-3.4.1. then (a subset of possible) options could be: +) case-insensitive (first match taken (or last match?) -- option?) +) unique email addresses (take phrase/comment from first/last?) -- or use first that has something additional to plain address -- or use last that has something additional to plain address Yes, there is a lot of possible options. I don't think that notmuch has to support all of them. If people need something special like use last that has something additional to plain address, they can always do --unique=none and do their own post-processing. -Michal ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
