From: Jani Nikula <jani@nikula.org>
To: Antoine =?utf-8?Q?Beaupr=C3=A9?= <anarcat@koumbit.org>,
	notmuch@notmuchmail.org
Subject: Re: [PATCH 2/2] lib: add 'safe' setting for flags
In-Reply-To: <1310874973-28437-2-git-send-email-anarcat@koumbit.org>
References: <1310874973-28437-1-git-send-email-anarcat@koumbit.org>
	<1310874973-28437-2-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:45:30 +0200
Message-ID: <871urc8n79.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:13 -0400, Antoine Beaupr=C3=A9 <anarcat@koumbit.o=
rg> wrote:
> the 'safe' setting needs to be 'true' for flags to be manipulated by
> notmuch new/tag/restore.
>=20
> for now, only the (T)rash tag is configurable and set to false (by
> default) but this could be extended to allow the user to configure which
> flags are allowed to be synchronized.
>=20
> the reason why only T is configurable is because (a) it's the the only
> one that is actually dangerous and (b) I couldn't figure out how to
> properly configure multiple settings like this.
> ---
>  lib/message.cc    |   40 +++++++++++++++++++++++++---------------
>  lib/notmuch.h     |   20 ++++++++++++++++----
>  notmuch-new.c     |    1 +
>  notmuch-restore.c |    1 +
>  notmuch-tag.c     |    1 +
>  5 files changed, 44 insertions(+), 19 deletions(-)
>=20
> diff --git a/lib/message.cc b/lib/message.cc
> index f633887..f812648 100644
> --- a/lib/message.cc
> +++ b/lib/message.cc
> @@ -50,16 +50,17 @@ struct maildir_flag_tag {
>      char flag;
>      const char *tag;
>      bool inverse;
> +    bool safe;
>  };
>=20=20
>  /* ASCII ordered table of Maildir flags and associated tags */
>  static struct maildir_flag_tag flag2tag[] =3D {
> -    { 'D', "draft",   false},
> -    { 'F', "flagged", false},
> -    { 'P', "passed",  false},
> -    { 'R', "replied", false},
> -    { 'S', "unread",  true },
> -    { 'T', "deleted", false},
> +    { 'D', "draft",   false, true},
> +    { 'F', "flagged", false, true},
> +    { 'P', "passed",  false, true},
> +    { 'R', "replied", false, true},
> +    { 'S', "unread",  true , true},
> +    { 'T', "deleted", false, false},
>  };
>=20=20
>  /* We end up having to call the destructor explicitly because we had
> @@ -994,7 +995,6 @@ 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);
> @@ -1022,15 +1022,8 @@ 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);
> -    if (config =3D=3D NULL)
> -	return 1;
> -    reckless_trash =3D notmuch_config_get_maildir_reckless_trash (config=
);
> -

Antoine -

You do *not* send a patch 1/2 that adds features and then 2/2 that takes
them away. I feel like I totally wasted my time reviewing the first.


Jani.



>      for (i =3D 0; i < ARRAY_SIZE(flag2tag); i++) {
> -	if (flag2tag[i].flag =3D=3D 'T' && !reckless_trash) {
> +	if (!flag2tag[i].safe) {
>  	    continue;
>  	}
>  	if ((strchr (combined_flags, flag2tag[i].flag) !=3D NULL)
> @@ -1119,6 +1112,9 @@ _get_maildir_flag_actions (notmuch_message_t *messa=
ge,
>  	tag =3D notmuch_tags_get (tags);
>=20=20
>  	for (i =3D 0; i < ARRAY_SIZE (flag2tag); i++) {
> +	    if (!flag2tag[i].safe) {
> +		continue;
> +	    }
>  	    if (strcmp (tag, flag2tag[i].tag) =3D=3D 0) {
>  		if (flag2tag[i].inverse)
>  		    to_clear =3D talloc_asprintf_append (to_clear,
> @@ -1134,6 +1130,9 @@ _get_maildir_flag_actions (notmuch_message_t *messa=
ge,
>=20=20
>      /* Then, find the flags for all tags not present. */
>      for (i =3D 0; i < ARRAY_SIZE (flag2tag); i++) {
> +	if (!flag2tag[i].safe) {
> +	    continue;
> +	}
>  	if (flag2tag[i].inverse) {
>  	    if (strchr (to_clear, flag2tag[i].flag) =3D=3D NULL)
>  		to_set =3D talloc_asprintf_append (to_set, "%c", flag2tag[i].flag);
> @@ -1256,6 +1255,17 @@ _new_maildir_filename (void *ctx,
>      return filename_new;
>  }
>=20=20
> +void
> +notmuch_message_set_flag_safety (char flag, notmuch_bool_t safe)
> +{
> +    unsigned i;
> +    for (i =3D 0; i < ARRAY_SIZE (flag2tag); i++) {
> +        if (flag2tag[i].flag =3D=3D flag) {
> +            flag2tag[i].safe =3D safe;
> +        }
> +    }
> +}
> +
>  notmuch_status_t
>  notmuch_message_tags_to_maildir_flags (notmuch_message_t *message)
>  {
> diff --git a/lib/notmuch.h b/lib/notmuch.h
> index f0c1b67..475e75a 100644
> --- a/lib/notmuch.h
> +++ b/lib/notmuch.h
> @@ -922,8 +922,7 @@ 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.
> + * 	'T'	Adds the "deleted" tag to the message
>   *
>   * For each flag that is not present, the opposite action (add/remove)
>   * is performed for the corresponding tags.
> @@ -941,6 +940,9 @@ notmuch_message_remove_all_tags (notmuch_message_t *m=
essage);
>   * notmuch_database_add_message. See also
>   * notmuch_message_tags_to_maildir_flags for synchronizing tag changes
>   * back to maildir flags.
> + *
> + * Actions are performed only if the tag is marked as "safe" in the
> + * configuration (only used by maildir_reckless_trash for now).
>   */
>  notmuch_status_t
>  notmuch_message_maildir_flags_to_tags (notmuch_message_t *message);
> @@ -964,8 +966,7 @@ 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.
> + *   'T' iff the message has the "trashed" tag
>   *
>   * Any existing flags unmentioned in the list above will be preserved
>   * in the renaming.
> @@ -979,10 +980,21 @@ notmuch_message_maildir_flags_to_tags (notmuch_mess=
age_t *message);
>   * notmuch_message_remove_tag, or notmuch_message_freeze/
>   * notmuch_message_thaw). See also notmuch_message_maildir_flags_to_tags
>   * for synchronizing maildir flag changes back to tags.
> + *
> + * Actions are performed only if the tag is marked as "safe" in the
> + * configuration (only used by maildir_reckless_trash for now).
>   */
>  notmuch_status_t
>  notmuch_message_tags_to_maildir_flags (notmuch_message_t *message);
>=20=20
> +/* Set the 'safe' setting on the given flag
> + *
> + * The flag is the one-character IMAP flag, for example for Trashed
> + * messages, it's the char 'T'.
> + */
> +void
> +notmuch_message_set_flag_safety(char flag, notmuch_bool_t safe);
> +
>  /* Freeze the current state of 'message' within the database.
>   *
>   * This means that changes to the message state, (via
> diff --git a/notmuch-new.c b/notmuch-new.c
> index 7d17793..1502a70 100644
> --- a/notmuch-new.c
> +++ b/notmuch-new.c
> @@ -801,6 +801,7 @@ notmuch_new_command (void *ctx, int argc, char *argv[=
])
>=20=20
>      add_files_state.new_tags =3D notmuch_config_get_new_tags (config, &a=
dd_files_state.new_tags_length);
>      add_files_state.synchronize_flags =3D notmuch_config_get_maildir_syn=
chronize_flags (config);
> +    notmuch_message_set_flag_safety('T', notmuch_config_get_maildir_reck=
less_trash (config));
>      add_files_state.message_ids_to_sync =3D _filename_list_create (ctx);
>      db_path =3D notmuch_config_get_database_path (config);
>=20=20
> diff --git a/notmuch-restore.c b/notmuch-restore.c
> index f095f64..1f3622e 100644
> --- a/notmuch-restore.c
> +++ b/notmuch-restore.c
> @@ -43,6 +43,7 @@ notmuch_restore_command (unused (void *ctx), int argc, =
char *argv[])
>  	return 1;
>=20=20
>      synchronize_flags =3D notmuch_config_get_maildir_synchronize_flags (=
config);
> +    notmuch_message_set_flag_safety('T', notmuch_config_get_maildir_reck=
less_trash (config));
>=20=20
>      if (argc) {
>  	input =3D fopen (argv[0], "r");
> diff --git a/notmuch-tag.c b/notmuch-tag.c
> index 6204ae3..e2f7cb8 100644
> --- a/notmuch-tag.c
> +++ b/notmuch-tag.c
> @@ -101,6 +101,7 @@ notmuch_tag_command (void *ctx, unused (int argc), un=
used (char *argv[]))
>  	return 1;
>=20=20
>      synchronize_flags =3D notmuch_config_get_maildir_synchronize_flags (=
config);
> +    notmuch_message_set_flag_safety('T', notmuch_config_get_maildir_reck=
less_trash (config));
>=20=20
>      query =3D notmuch_query_create (notmuch, query_string);
>      if (query =3D=3D NULL) {
> --=20
> 1.7.2.5
>=20
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch