[PATCH 2/2] contrib: pick: thread tagging (including archiving) implemented

[Mark Walters]

Thanks for the review. A couple of comments below:

On Sat, 01 Jun 2013, David Bremner <david at tethera.net> wrote:
> Mark Walters <markwalters1009 at gmail.com> writes:
>
>> Previously pick had no actions based on the entire thread: this adds
>> some. Note in this version '*' is bound to `tag thread' which is not
>> consistent with search or show. However it still might be the most
>> natural thing (as it is similar to running * in the show pane).
>
> Hmm. not sure about this. But since nobody complained in six months,
> let's go with it unless they wake up.

Ok

>> +  (setq tag-changes (funcall 'notmuch-tag
>> (notmuch-pick-get-messages-ids-thread-search) tag-changes))
>
> Why "(funcall 'notmuch-tag ...) instead of a "(notmuch-tag ...)" here?
> Personally I'd use let instead of setq here, but ymmv.

I agree with both of these but this is almost identical code to that in
notmuch-show-tag-all. 

>> +Archive each message currently shown by applying the tag changes
>> +in `notmuch-archive-tags' to each (remove the \"inbox\" tag by
>> +default). If a prefix argument is given, the messages will be
>> +\"unarchived\", i.e. the tag changes in `notmuch-archive-tags'
>> +will be reversed.
>
> It seems to me the default value for notmuch-archive-tags should not be
> hard-coded into this docstring, especially since notmuch-archive-tags
> turns into a link in the help text.

I am happy either way on this one but again this is the same as
notmuch-show-archive-thread.

I think it's probably best to keep show and pick the same where they are
similar but I can change the above if you disagree.

Best wishes

Mark