From: Carl Worth <cworth@cworth.org>
To: Austin Clements <amdragon@mit.edu>, notmuch@notmuchmail.org
Subject: Re: [PATCH 00/10] Fix 'notmuch new' atomicity issues
In-Reply-To: <BANLkTimcVvQjSF7E65gO1KSY4KQSXsVQNw@mail.gmail.com>
References: <1298015940-31986-1-git-send-email-amdragon@mit.edu>
	<BANLkTikOVqK7p1G2SmgJoac+n8p19_XsdQ@mail.gmail.com>
	<BANLkTiktPcf9Y4fF8nP1+66GpDP6eGY2Vw@mail.gmail.com>
	<BANLkTimcVvQjSF7E65gO1KSY4KQSXsVQNw@mail.gmail.com>
User-Agent: Notmuch/0.5 (http://notmuchmail.org) Emacs/23.3.1
	(i486-pc-linux-gnu)
Date: Wed, 08 Jun 2011 15:05:14 -0700
Message-ID: <87ei34rnc5.fsf@yoom.home.cworth.org>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="=-=-=";
	micalg=pgp-sha1; protocol="application/pgp-signature"
Precedence: list

--=-=-=
Content-Transfer-Encoding: quoted-printable

On Sat, 28 May 2011 22:51:10 -0400, Austin Clements <amdragon@mit.edu> wrot=
e:
> Rebased to current master (cb8418) as atomic-new-v4 (aka
> for-review/atomic-new-v4).

Hi Austin,

Thanks so much for sending this series (and 4 times, even!).

I *really* like the new robustness provided by this series, and I
especially like the exhaustive testing here. Thanks so much!

Having just gone through the for-review/atomic-new-v4 series, I have a
few comments. Some are very minor and I'll be glad to implement them
myself:

	1. Two commits have "lose" misspelled as "loose". These are "ew:
	   don't loose messages on SIGINT" and "new: Wrap adding a
	   message in an atomic section".

	2. The commit with summary of "lib: Make _notmuch_message_sync
	   capable of deleting a message." is missing the rest of its
	   commit message with a complete explanation. For example, this
	   commit message should describe that a message document is
	   deleted from the database (if the deleted field is set when
	   _sync is called). And the commit message should also mention
	   that this functionality is not currently used, but prepares
	   for a subsequent use.

	3. While reviewing the commit "lib: Indicate if there are more
	   filenames after removal" the "if (status =3D=3D
	   NOTMUCH_STATUS_SUCCESS)" looked out of place to me. Indeed,
	   if status is any other value at this point in the code, then
	   the function should have returned earlier. I intend to follow
	   up with a commit that adds the missing early return and
	   removes this condition.

	4. I really don't like that the final state of the code has two
	   different functions named notmuch_message_remove_filename and
	   _notmuch_message_remove_filename. If the semantics of these
	   functions are identical, then there should be only one
	   function. If the semantics are different, then they need to
	   have noticeably distinct names, (and a single underscore
	   doesn't count).

	5. The final code has a function inside of notmuch-new.c named
	   "remove_file", but this function isn't removing a
	   file---instead it's removing a message document from the
	   database. So it needs a more accurate name.

Like I said, those are all pretty minor and I would just implement all
of those and push the series myself, but for one remaining issue that is
a bit more significant.

The last issue has to do with the addition of the
notmuch_database_find_message_by_filename and
notmuch_message_remove_filename functions. In the series as it stands,
notmuch-new.c is updated to call these two functions instead of calling
the existing notmuch_database_remove_message function (which itself also
calls the same functions).

That sets off a red flag in my mind. If our program is avoiding a
library function and substituting its own implementation, how are other
users of the library going to get things right? Should we deprecate
notmuch_database_remove_message? Should we add more documentation to it
describing the situation in which a user might prefer not to call it? It
seems the library is harder to use than it should be in this area.

Meanwhile, I'm not very satisfied by the existence of
notmuch_message_remove_filename in the public API. It would have a
natural pairing with notmuch_message_add_filename, but the series isn't
exporting that functionality. So things feel more asymmetric than they
should be as well.

Now, why is notmuch-new going through all this effort to reimplement an
existing library function (and requiring two new library functions in
the process)? What it wants to do is to wrap the functionality of
database_remove_message in freeze/thaw and while the message is frozen
call notmuch_message_maildir_flags_to_tags.

So, how to fix my complaints above?

  * Do we want to allow database_remove_message to optionally call
    maildir_flags_to_tags?

    This seems a little messy in requiring some additional information
    to the library so it can know whether to do the maildir
    synchronization here. And it's also asymmetric unless we would also
    support similar synchronization support in the library for simlar
    operations.

  * Do we want to expose notmuch_message_add_filename as well as
    remove_filename for better symmetry?

    I'm not sure I like that. It still feels like we're exposing too
    many internals and not making it obvious to the user how to do
    things. Having just the existing add_message/remove_message
    functions definitely makes the interface easier.

  * Can we fix the remove case without this new library API by simply
    adding calls to begin_atomic and end_atomic?

    I think this is probably the solution I would prefer to see.

What do you think, Austin?

=2DCarl

=2D-=20
carl.d.worth@intel.com

--=-=-=
Content-Type: application/pgp-signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)

iEYEARECAAYFAk3v8hoACgkQ6JDdNq8qSWh7lQCfa5lAnUui+EsioKxW0vd4hF6n
e5oAn2GiW8CXQUSLhpIcLJYuCWiwarQj
=tG4z
-----END PGP SIGNATURE-----
--=-=-=--