[PATCH 1/2] lib: Add back the synchronization of 'T' flag with deleted tag.

[Antoine Beaupré]

On Sat, 07 Jan 2012 00:37:07 +0200, Jani Nikula  wrote:
> On Sat, 16 Jul 2011 23:56:12 -0400, Antoine Beaupr?  
> wrote:
> > +// TODO: this should probably be moved up in the stack to avoid
> > +// opening the config file on every message (!)
> > +config = notmuch_config_open (ctx, NULL, NULL);
> 
> The config file is for notmuch the command line tool, *not* for the
> lib. You can't call the cli from from the lib. The config (or command
> line argument) should be passed as argument, but that would require
> changing the lib interface.

I see. I wasn't aware of that.

> > + *   'T' iff the message has the "trashed" tag and
> > + *   state->reckless_trash is TRUE.
> 
> "trashed" tag?

That should probably be "deleted".

> The comment (and the commit message) is incorrect. You only check for
> reckless_trash in maildir_flags_to_tags, not tags_to_maildir_flags.
> With this patch, one-way syncing from tags to flags would be done
> unconditionally. And if I understand the problem correctly, you're
> fixing the less critical one of the two!

Indeed! What an oversight...

> I am wondering (but I'm too tired to check) if the original problem
> could be avoided by simply refusing to sync "deleted" tag to 'T' flag if
> there are more than one file for that message.

That would be a great idea.

> This is a dangerous feature, which is why it was originally
> disabled. Accidentally deleting mail is not something people take
> lightly. They'll be amused by "reckless trash" - until it recklessly
> deletes an important mail.

Yes, I understand this.

> However, something like this might be a useful feature to have for
> people who want to delete mail. It would need good tests to accompany
> it, though.

And to be honest, that's where I got off the boat. :) It just got too
hard, and anyways I use a custom script that deletes mails from notmuch
search tag:deleted, so syncing that flag isn't so important for me.

I guess this got everything covered for me. I would be ready to accept
this patch being dropped from the queue, although I think it's a key
step in having a more general tag to maildir flags synchronisation
strategy that would allow to run notmuch from multiple clients, without
having to sync databases around.

Thanks for the review, this patch is indeed not ready, and I am not sure
when I will have time to push it further.

Cheers,

A.

-- 
Modern man has a kind of poverty of the spirit which stands
in great contrast to his remarkable scientific and technological
achievements. We've learned to walk in outer space and yet we
haven't learned to walk to earth as brothers and sisters.
- Dr. Martin Luther King, Jr.
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 835 bytes
Desc: not available
URL: 

Re: [PATCH 1/2] lib: Add back the synchronization of 'T' flag with deleted tag.

[Antoine Beaupré]

On Sat, 07 Jan 2012 00:37:07 +0200, Jani Nikula j...@nikula.org wrote:
 On Sat, 16 Jul 2011 23:56:12 -0400, Antoine Beaupré anar...@koumbit.org 
 wrote:
  +// TODO: this should probably be moved up in the stack to avoid
  +// opening the config file on every message (!)
  +config = notmuch_config_open (ctx, NULL, NULL);
 
 The config file is for notmuch the command line tool, *not* for the
 lib. You can't call the cli from from the lib. The config (or command
 line argument) should be passed as argument, but that would require
 changing the lib interface.

I see. I wasn't aware of that.

  + *   'T' iff the message has the trashed tag and
  + *   state-reckless_trash is TRUE.
 
 trashed tag?

That should probably be deleted.

 The comment (and the commit message) is incorrect. You only check for
 reckless_trash in maildir_flags_to_tags, not tags_to_maildir_flags.
 With this patch, one-way syncing from tags to flags would be done
 unconditionally. And if I understand the problem correctly, you're
 fixing the less critical one of the two!

Indeed! What an oversight...

 I am wondering (but I'm too tired to check) if the original problem
 could be avoided by simply refusing to sync deleted tag to 'T' flag if
 there are more than one file for that message.

That would be a great idea.

 This is a dangerous feature, which is why it was originally
 disabled. Accidentally deleting mail is not something people take
 lightly. They'll be amused by reckless trash - until it recklessly
 deletes an important mail.

Yes, I understand this.

 However, something like this might be a useful feature to have for
 people who want to delete mail. It would need good tests to accompany
 it, though.

And to be honest, that's where I got off the boat. :) It just got too
hard, and anyways I use a custom script that deletes mails from notmuch
search tag:deleted, so syncing that flag isn't so important for me.

I guess this got everything covered for me. I would be ready to accept
this patch being dropped from the queue, although I think it's a key
step in having a more general tag to maildir flags synchronisation
strategy that would allow to run notmuch from multiple clients, without
having to sync databases around.

Thanks for the review, this patch is indeed not ready, and I am not sure
when I will have time to push it further.

Cheers,

A.

-- 
Modern man has a kind of poverty of the spirit which stands
in great contrast to his remarkable scientific and technological
achievements. We've learned to walk in outer space and yet we
haven't learned to walk to earth as brothers and sisters.
- Dr. Martin Luther King, Jr.


pgpTZLrCrIF2C.pgp
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH 1/2] lib: Add back the synchronization of 'T' flag with deleted tag.

[Jani Nikula]

On Sat, 16 Jul 2011 23:56:12 -0400, Antoine Beaupr?  
wrote:
> This adds a special configuration, off by default, that allows notmuch
> to synchronize the T flag again. The configuration is named
> maildir_reckless_trash and quite clearly indicates that it could be
> dangerous to use in the context described in commit 2c26204, which I
> could actually reproduce.

Thanks for the commit reference.

Please find some comments below.

> In contexts where notmuch is the only mail client used, this is actually
> safe to use. Besides, (T)rashed messages are not necessarily immediately
> expunged from the Maildir by the client or the IMAP server.
> 
> Signed-off-by: Antoine Beaupr? 
> ---
>  lib/message.cc   |   14 +-
>  lib/notmuch.h|4 
>  notmuch-client.h |7 +++
>  notmuch-config.c |   50 +++---
>  4 files changed, 71 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/message.cc b/lib/message.cc
> index d993cde..f633887 100644
> --- a/lib/message.cc
> +++ b/lib/message.cc
> @@ -58,7 +58,8 @@ static struct maildir_flag_tag flag2tag[] = {
>  { 'F', "flagged", false},
>  { 'P', "passed",  false},
>  { 'R', "replied", false},
> -{ 'S', "unread",  true }
> +{ 'S', "unread",  true },
> +{ 'T', "deleted", false},
>  };
>  
>  /* We end up having to call the destructor explicitly because we had
> @@ -993,6 +994,7 @@ notmuch_message_maildir_flags_to_tags (notmuch_message_t 
> *message)
>  char *combined_flags = talloc_strdup (message, "");
>  unsigned i;
>  int seen_maildir_info = 0;
> +notmuch_bool_t reckless_trash;
>  
>  for (filenames = notmuch_message_get_filenames (message);
>notmuch_filenames_valid (filenames);
> @@ -1020,7 +1022,17 @@ notmuch_message_maildir_flags_to_tags 
> (notmuch_message_t *message)
>  if (status)
>   return status;
>  
> +// TODO: this should probably be moved up in the stack to avoid
> +// opening the config file on every message (!)
> +config = notmuch_config_open (ctx, NULL, NULL);

The config file is for notmuch the command line tool, *not* for the
lib. You can't call the cli from from the lib. The config (or command
line argument) should be passed as argument, but that would require
changing the lib interface.

> +if (config == NULL)
> + return 1;
> +reckless_trash = notmuch_config_get_maildir_reckless_trash (config);
> +
>  for (i = 0; i < ARRAY_SIZE(flag2tag); i++) {
> + if (flag2tag[i].flag == 'T' && !reckless_trash) {
> + continue;
> + }
>   if ((strchr (combined_flags, flag2tag[i].flag) != NULL)
>   ^ 
>   flag2tag[i].inverse)
> diff --git a/lib/notmuch.h b/lib/notmuch.h
> index 974be8d..f0c1b67 100644
> --- a/lib/notmuch.h
> +++ b/lib/notmuch.h
> @@ -922,6 +922,8 @@ notmuch_message_remove_all_tags (notmuch_message_t 
> *message);
>   *   'P' Adds the "passed" tag to the message
>   *   'R' Adds the "replied" tag to the message
>   *   'S' Removes the "unread" tag from the message
> + *   'T' Adds the "deleted" tag to the message and
> + *   state->reckless_trash is TRUE.
>   *
>   * For each flag that is not present, the opposite action (add/remove)
>   * is performed for the corresponding tags.
> @@ -962,6 +964,8 @@ notmuch_message_maildir_flags_to_tags (notmuch_message_t 
> *message);
>   *   'P' iff the message has the "passed" tag
>   *   'R' iff the message has the "replied" tag
>   *   'S' iff the message does not have the "unread" tag
> + *   'T' iff the message has the "trashed" tag and
> + *   state->reckless_trash is TRUE.

"trashed" tag?

The comment (and the commit message) is incorrect. You only check for
reckless_trash in maildir_flags_to_tags, not tags_to_maildir_flags.
With this patch, one-way syncing from tags to flags would be done
unconditionally. And if I understand the problem correctly, you're
fixing the less critical one of the two!

I am wondering (but I'm too tired to check) if the original problem
could be avoided by simply refusing to sync "deleted" tag to 'T' flag if
there are more than one file for that message.

This is a dangerous feature, which is why it was originally
disabled. Accidentally deleting mail is not something people take
lightly. They'll be amused by "reckless trash" - until it recklessly
deletes an important mail.

However, something like this might be a useful feature to have for
people who want to delete mail. It would need good tests to accompany
it, though.


BR,
Jani.


>   *
>   * Any existing flags unmentioned in the list above will be preserved
>   * in the renaming.
> diff --git a/notmuch-client.h b/notmuch-client.h
> index 63be337..62d1e0e 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -235,6 +235,13 @@ notmuch_config_set_maildir_synchronize_flags 
> (notmuch_config_t *config,
> notmuch_bool_t synchronize_flags);
>  
>  notmuch_bool_t
> 

[PATCH 1/2] lib: Add back the synchronization of 'T' flag with deleted tag.

[Antoine Beaupré]

This adds a special configuration, off by default, that allows notmuch
to synchronize the T flag again. The configuration is named
maildir_reckless_trash and quite clearly indicates that it could be
dangerous to use in the context described in commit 2c26204, which I
could actually reproduce.

In contexts where notmuch is the only mail client used, this is actually
safe to use. Besides, (T)rashed messages are not necessarily immediately
expunged from the Maildir by the client or the IMAP server.

Signed-off-by: Antoine Beaupr? 
---
 lib/message.cc   |   14 +-
 lib/notmuch.h|4 
 notmuch-client.h |7 +++
 notmuch-config.c |   50 +++---
 4 files changed, 71 insertions(+), 4 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index d993cde..f633887 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -58,7 +58,8 @@ static struct maildir_flag_tag flag2tag[] = {
 { 'F', "flagged", false},
 { 'P', "passed",  false},
 { 'R', "replied", false},
-{ 'S', "unread",  true }
+{ 'S', "unread",  true },
+{ 'T', "deleted", false},
 };

 /* We end up having to call the destructor explicitly because we had
@@ -993,6 +994,7 @@ notmuch_message_maildir_flags_to_tags (notmuch_message_t 
*message)
 char *combined_flags = talloc_strdup (message, "");
 unsigned i;
 int seen_maildir_info = 0;
+notmuch_bool_t reckless_trash;

 for (filenames = notmuch_message_get_filenames (message);
 notmuch_filenames_valid (filenames);
@@ -1020,7 +1022,17 @@ notmuch_message_maildir_flags_to_tags (notmuch_message_t 
*message)
 if (status)
return status;

+// TODO: this should probably be moved up in the stack to avoid
+// opening the config file on every message (!)
+config = notmuch_config_open (ctx, NULL, NULL);
+if (config == NULL)
+   return 1;
+reckless_trash = notmuch_config_get_maildir_reckless_trash (config);
+
 for (i = 0; i < ARRAY_SIZE(flag2tag); i++) {
+   if (flag2tag[i].flag == 'T' && !reckless_trash) {
+   continue;
+   }
if ((strchr (combined_flags, flag2tag[i].flag) != NULL)
^ 
flag2tag[i].inverse)
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 974be8d..f0c1b67 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -922,6 +922,8 @@ notmuch_message_remove_all_tags (notmuch_message_t 
*message);
  * 'P' Adds the "passed" tag to the message
  * 'R' Adds the "replied" tag to the message
  * 'S' Removes the "unread" tag from the message
+ * 'T' Adds the "deleted" tag to the message and
+ * state->reckless_trash is TRUE.
  *
  * For each flag that is not present, the opposite action (add/remove)
  * is performed for the corresponding tags.
@@ -962,6 +964,8 @@ notmuch_message_maildir_flags_to_tags (notmuch_message_t 
*message);
  *   'P' iff the message has the "passed" tag
  *   'R' iff the message has the "replied" tag
  *   'S' iff the message does not have the "unread" tag
+ *   'T' iff the message has the "trashed" tag and
+ *   state->reckless_trash is TRUE.
  *
  * Any existing flags unmentioned in the list above will be preserved
  * in the renaming.
diff --git a/notmuch-client.h b/notmuch-client.h
index 63be337..62d1e0e 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -235,6 +235,13 @@ notmuch_config_set_maildir_synchronize_flags 
(notmuch_config_t *config,
  notmuch_bool_t synchronize_flags);

 notmuch_bool_t
+notmuch_config_get_maildir_reckless_trash (notmuch_config_t *config);
+
+void
+notmuch_config_set_maildir_reckless_trash (notmuch_config_t *config,
+  notmuch_bool_t reckless_trash);
+
+notmuch_bool_t
 debugger_is_active (void);

 #endif
diff --git a/notmuch-config.c b/notmuch-config.c
index 485fa72..613fefc 100644
--- a/notmuch-config.c
+++ b/notmuch-config.c
@@ -67,9 +67,11 @@ static const char maildir_config_comment[] =
 " The following option is supported here:\n"
 "\n"
 "\tsynchronize_flags  Valid values are true and false.\n"
+"\treckless_trash Valid values are true and false.\n"
 "\n"
-"\tIf true, then the following maildir flags (in message filenames)\n"
-"\twill be synchronized with the corresponding notmuch tags:\n"
+"\tIf synchronize_flags is true, then the following maildir flags\n"
+"\t(in message filenames) will be synchronized with the corresponding\n"
+"\tnotmuch tags:\n"
 "\n"
 "\t\tFlag  Tag\n"
 "\t\t  ---\n"
@@ -78,10 +80,26 @@ static const char maildir_config_comment[] =
 "\t\tP passed\n"
 "\t\tR replied\n"
 "\t\tS unread (added when 'S' flag is not present)\n"
+"\t\tT trashed (only if maildir_reckless_trash is enabled)\n"
 "\n"
 "\tThe \"notmuch new\" command will notice flag changes in filenames\n"
 "\tand update tags, while the \"notmuch 

[PATCH 1/2] lib: Add back the synchronization of 'T' flag with deleted tag.

[Antoine Beaupré]

This adds a special configuration, off by default, that allows notmuch
to synchronize the T flag again. The configuration is named
maildir_reckless_trash and quite clearly indicates that it could be
dangerous to use in the context described in commit 2c26204, which I
could actually reproduce.

In contexts where notmuch is the only mail client used, this is actually
safe to use. Besides, (T)rashed messages are not necessarily immediately
expunged from the Maildir by the client or the IMAP server.

Signed-off-by: Antoine Beaupré anar...@koumbit.org
---
 lib/message.cc   |   14 +-
 lib/notmuch.h|4 
 notmuch-client.h |7 +++
 notmuch-config.c |   50 +++---
 4 files changed, 71 insertions(+), 4 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index d993cde..f633887 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -58,7 +58,8 @@ static struct maildir_flag_tag flag2tag[] = {
 { 'F', flagged, false},
 { 'P', passed,  false},
 { 'R', replied, false},
-{ 'S', unread,  true }
+{ 'S', unread,  true },
+{ 'T', deleted, false},
 };
 
 /* We end up having to call the destructor explicitly because we had
@@ -993,6 +994,7 @@ notmuch_message_maildir_flags_to_tags (notmuch_message_t 
*message)
 char *combined_flags = talloc_strdup (message, );
 unsigned i;
 int seen_maildir_info = 0;
+notmuch_bool_t reckless_trash;
 
 for (filenames = notmuch_message_get_filenames (message);
 notmuch_filenames_valid (filenames);
@@ -1020,7 +1022,17 @@ notmuch_message_maildir_flags_to_tags (notmuch_message_t 
*message)
 if (status)
return status;
 
+// TODO: this should probably be moved up in the stack to avoid
+// opening the config file on every message (!)
+config = notmuch_config_open (ctx, NULL, NULL);
+if (config == NULL)
+   return 1;
+reckless_trash = notmuch_config_get_maildir_reckless_trash (config);
+
 for (i = 0; i  ARRAY_SIZE(flag2tag); i++) {
+   if (flag2tag[i].flag == 'T'  !reckless_trash) {
+   continue;
+   }
if ((strchr (combined_flags, flag2tag[i].flag) != NULL)
^ 
flag2tag[i].inverse)
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 974be8d..f0c1b67 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -922,6 +922,8 @@ notmuch_message_remove_all_tags (notmuch_message_t 
*message);
  * 'P' Adds the passed tag to the message
  * 'R' Adds the replied tag to the message
  * 'S' Removes the unread tag from the message
+ * 'T' Adds the deleted tag to the message and
+ * state-reckless_trash is TRUE.
  *
  * For each flag that is not present, the opposite action (add/remove)
  * is performed for the corresponding tags.
@@ -962,6 +964,8 @@ notmuch_message_maildir_flags_to_tags (notmuch_message_t 
*message);
  *   'P' iff the message has the passed tag
  *   'R' iff the message has the replied tag
  *   'S' iff the message does not have the unread tag
+ *   'T' iff the message has the trashed tag and
+ *   state-reckless_trash is TRUE.
  *
  * Any existing flags unmentioned in the list above will be preserved
  * in the renaming.
diff --git a/notmuch-client.h b/notmuch-client.h
index 63be337..62d1e0e 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -235,6 +235,13 @@ notmuch_config_set_maildir_synchronize_flags 
(notmuch_config_t *config,
  notmuch_bool_t synchronize_flags);
 
 notmuch_bool_t
+notmuch_config_get_maildir_reckless_trash (notmuch_config_t *config);
+
+void
+notmuch_config_set_maildir_reckless_trash (notmuch_config_t *config,
+  notmuch_bool_t reckless_trash);
+
+notmuch_bool_t
 debugger_is_active (void);
 
 #endif
diff --git a/notmuch-config.c b/notmuch-config.c
index 485fa72..613fefc 100644
--- a/notmuch-config.c
+++ b/notmuch-config.c
@@ -67,9 +67,11 @@ static const char maildir_config_comment[] =
  The following option is supported here:\n
 \n
 \tsynchronize_flags  Valid values are true and false.\n
+\treckless_trash Valid values are true and false.\n
 \n
-\tIf true, then the following maildir flags (in message filenames)\n
-\twill be synchronized with the corresponding notmuch tags:\n
+\tIf synchronize_flags is true, then the following maildir flags\n
+\t(in message filenames) will be synchronized with the corresponding\n
+\tnotmuch tags:\n
 \n
 \t\tFlag  Tag\n
 \t\t  ---\n
@@ -78,10 +80,26 @@ static const char maildir_config_comment[] =
 \t\tP passed\n
 \t\tR replied\n
 \t\tS unread (added when 'S' flag is not present)\n
+\t\tT trashed (only if maildir_reckless_trash is enabled)\n
 \n
 \tThe \notmuch new\ command will notice flag changes in filenames\n
 \tand update tags, while the \notmuch tag\ and \notmuch restore\\n
-\tcommands will