Re: [PATCH 6/6] emacs: make `notmuch-search-operate-all' operate on threads, not messages

[Pieter Praet <pieter@praet.org>]

On Sat, 12 Nov 2011 11:35:02 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> Quoth David Bremner on Nov 12 at 11:13 am:
> > On Sun, 10 Jul 2011 17:55:35 +0200, Pieter Praet <pieter@praet.org> wrote:
> > > In order to be consistent with `notmuch-search-{add,remove}-tag' ("+"/"-"),
> > > `notmuch-search-operate-all' ("*") should operate on matching threads
> > > instead of matching messages.
> > > 
> > 
> > As far as I can tell, the follow-up series for the race condition kindof
> > got stalled.  Am I right in thinking this bug fix should still be
> > applied? I didn't see any review/feedback on the list.
> 
> We concluded that fixing the tagging race correctly was actually a lot
> of work, which should be done but hasn't yet.  We have to add message
> IDs or docids to the search results, which is difficult to do with the
> current text format, so rather than further entrenching ourselves, we
> should first we should migrate Emacs to using the JSON-based search
> output.
> 

Yeah, sorry I haven't replied there yet.  Still haven't found a
sufficiently uninterrupted stretch of time to give the *massive*
amount of work you did the attention it deserves.

> However, this series doesn't actually have much to do with the race.

Correct.  Only patch #4 is more or less relevant to fixing the
`notmuch-search-operate-all' race condition (safety net for when
I make stupid mistakes).

Patches #1-3 should have been in a separate thread (or as updates in
their original thread [1]), but since #1 and #2 are mainly there to
support #3 and #3 is tagging-related, I though it wouldn't hurt to
include them.

Patches #5-6 are a matter of opinion:

> I think the question here is whether notmuch-search-operate-all should
> affect only matched messages or entire threads.  It seems to me it
> should affect all threads, since that's what you're seeing visually,
> but other people may disagree.
> 

Same here.  I don't use it that often, but if its name includes
"operate-all", it should do just that, or the function should be
renamed.

> The test patches seem reasonable, though they could use a little
> review before being pushed.

I'd really appreciate it.

AFAIC, increasing test coverage should be a top priority.


Peace

-- 
Pieter

[1] id:"1305275652-22956-1-git-send-email-pieter@praet.org"