From: Jani Nikula <jani@nikula.org>
To: Antoine =?utf-8?Q?Beaupr=C3=A9?= <anarcat@koumbit.org>,
	notmuch@notmuchmail.org
Subject: Re: [PATCH 1/2] lib: Add back the synchronization of 'T' flag with
	deleted tag.
In-Reply-To: <1310874973-28437-1-git-send-email-anarcat@koumbit.org>
References: <1310874973-28437-1-git-send-email-anarcat@koumbit.org>
User-Agent: Notmuch/0.10.2+182~g93862a2 (http://notmuchmail.org) Emacs/23.3.1
	(i686-pc-linux-gnu)
Date: Sat, 07 Jan 2012 00:37:07 +0200
Message-ID: <874nw88nl8.fsf@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Cc: Antoine =?utf-8?Q?Beaupr=C3=A9?= <anarcat@koumbit.org>
Precedence: list

On Sat, 16 Jul 2011 23:56:12 -0400, Antoine Beaupr=C3=A9 <anarcat@koumbit.o=
rg> 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.
>=20
> Signed-off-by: Antoine Beaupr=C3=A9 <anarcat@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(-)
>=20
> 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[] =3D {
>      { 'F', "flagged", false},
>      { 'P', "passed",  false},
>      { 'R', "replied", false},
> -    { 'S', "unread",  true }
> +    { 'S', "unread",  true },
> +    { 'T', "deleted", false},
>  };
>=20=20
>  /* We end up having to call the destructor explicitly because we had
> @@ -993,6 +994,7 @@ notmuch_message_maildir_flags_to_tags (notmuch_messag=
e_t *message)
>      char *combined_flags =3D talloc_strdup (message, "");
>      unsigned i;
>      int seen_maildir_info =3D 0;
> +    notmuch_bool_t reckless_trash;
>=20=20
>      for (filenames =3D notmuch_message_get_filenames (message);
>  	 notmuch_filenames_valid (filenames);
> @@ -1020,7 +1022,17 @@ notmuch_message_maildir_flags_to_tags (notmuch_mes=
sage_t *message)
>      if (status)
>  	return status;
>=20=20
> +    // TODO: this should probably be moved up in the stack to avoid
> +    // opening the config file on every message (!)
> +    config =3D 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 =3D=3D NULL)
> +	return 1;
> +    reckless_trash =3D notmuch_config_get_maildir_reckless_trash (config=
);
> +
>      for (i =3D 0; i < ARRAY_SIZE(flag2tag); i++) {
> +	if (flag2tag[i].flag =3D=3D 'T' && !reckless_trash) {
> +	    continue;
> +	}
>  	if ((strchr (combined_flags, flag2tag[i].flag) !=3D NULL)
>  	    ^=20
>  	    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 *m=
essage);
>   *	'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_messag=
e_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 (notmuc=
h_config_t *config,
>  					      notmuch_bool_t synchronize_flags);
>=20=20
>  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);
>=20=20
>  #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[] =3D
>      " 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 correspondin=
g\n"
> +    "\tnotmuch tags:\n"
>      "\n"
>      "\t\tFlag	Tag\n"
>      "\t\t----	-------\n"
> @@ -78,10 +80,26 @@ static const char maildir_config_comment[] =3D
>      "\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 notice tag changes and update flags in filenames\n";
> +    "\tcommands will notice tag changes and update flags in filenames\n"
> +    "\n"
> +    "\tBy default the maildir synchronization code doesn't look at the\n"
> +    "\t'trashed' (T) flag on messages. The reason for this behaviour is\=
n"
> +    "\tit can be dangerous if another mail client is trying to delete\n"
> +    "\tduplicates of a message with the same message ID.\n"
> +    "\n"
> +    "\tA workaround for this issue is to expunge the duplicate messages\=
n"
> +    "\twith the other client before running notmuch new.\n"
> +    "\n"
> +    "\tIf you are using only notmuch to handle your emails or are never\=
n"
> +    "\tdoing such operations, enabling reckless_trash should be safe,\n"
> +    "\tbut otherwise it is safer to keep this disabled and delete mails\=
n"
> +    "\tmanually with a shell command like:\n"
> +    "\n"
> +    "\tnotmuch search --format=3Dfiles tag:deleted | xargs rm\n";
>=20=20
>  struct _notmuch_config {
>      char *filename;
> @@ -95,6 +113,7 @@ struct _notmuch_config {
>      const char **new_tags;
>      size_t new_tags_length;
>      notmuch_bool_t maildir_synchronize_flags;
> +    notmuch_bool_t maildir_reckless_trash;
>  };
>=20=20
>  static int
> @@ -251,6 +270,7 @@ notmuch_config_open (void *ctx,
>      config->new_tags =3D NULL;
>      config->new_tags_length =3D 0;
>      config->maildir_synchronize_flags =3D TRUE;
> +    config->maildir_reckless_trash =3D FALSE;
>=20=20
>      if (! g_key_file_load_from_file (config->key_file,
>  				     config->filename,
> @@ -353,6 +373,15 @@ notmuch_config_open (void *ctx,
>  	g_error_free (error);
>      }
>=20=20
> +    error =3D NULL;
> +    config->maildir_reckless_trash =3D
> +	g_key_file_get_boolean (config->key_file,
> +				"maildir", "reckless_trash", &error);
> +    if (error) {
> +	notmuch_config_set_maildir_reckless_trash (config, FALSE);
> +	g_error_free (error);
> +    }
> +
>      /* Whenever we know of configuration sections that don't appear in
>       * the configuration file, we add some comments to help the user
>       * understand what can be done. */
> @@ -764,3 +793,18 @@ notmuch_config_set_maildir_synchronize_flags (notmuc=
h_config_t *config,
>  			    "maildir", "synchronize_flags", synchronize_flags);
>      config->maildir_synchronize_flags =3D synchronize_flags;
>  }
> +
> +notmuch_bool_t
> +notmuch_config_get_maildir_reckless_trash (notmuch_config_t *config)
> +{
> +    return config->maildir_reckless_trash;
> +}
> +
> +void
> +notmuch_config_set_maildir_reckless_trash (notmuch_config_t *config,
> +					   notmuch_bool_t reckless_trash)
> +{
> +    g_key_file_set_boolean (config->key_file,
> +			    "maildir", "reckless_trash", reckless_trash);
> +    config->maildir_reckless_trash =3D reckless_trash;
> +}
> --=20
> 1.7.2.5
>=20
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch