From: Tomi Ollila <tomi.ollila@iki.fi>
To: Ben Gamari <bgamari.foss@gmail.com>, notmuch@notmuchmail.org
Subject: Re: [PATCH 3/3] compact: Provide user with more error feedback
In-Reply-To: <1381799585-16274-4-git-send-email-bgamari.foss@gmail.com>
References: <1381799585-16274-1-git-send-email-bgamari.foss@gmail.com>
	<1381799585-16274-4-git-send-email-bgamari.foss@gmail.com>
User-Agent: Notmuch/0.16+112~g46b74be (http://notmuchmail.org) Emacs/24.3.1
	(x86_64-unknown-linux-gnu)
Date: Sun, 20 Oct 2013 21:13:16 +0300
Message-ID: <m2d2mz7qv7.fsf@guru.guru-group.fi>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: list

On Tue, Oct 15 2013, Ben Gamari <bgamari.foss@gmail.com> wrote:

> Provide instructions on what to do when we couldn't move the compacted
> database into place.
>
> Signed-off-by: Ben Gamari <bgamari.foss@gmail.com>
> ---
>  lib/database.cc | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/lib/database.cc b/lib/database.cc
> index 57c2292..6f9fed1 100644
> --- a/lib/database.cc
> +++ b/lib/database.cc
> @@ -938,7 +938,23 @@ notmuch_database_compact (const char* path,
>      }
>  
>      if (rename(compact_xapian_path, xapian_path)) {
> -	fprintf (stderr, "Error moving compacted database\n");
> +	fprintf (stderr, "Error moving compacted database into place: %s\n", strerror(errno));
> +	fprintf (stderr, "\n");
> +	fprintf (stderr, "Encountered error %s while moving the compacted database,\n",
> +		 strerror(errno));

At this point 'errno' may have changed, so the above error string might not
be the same. Maybe removing the '%s ' part altogether as inserting the part
looks a bit strange...

> +	fprintf (stderr, "\n");
> +	fprintf (stderr, "    %s\n", compact_xapian_path);
> +	fprintf (stderr, "\n");
> +	fprintf (stderr, "to\n");
> +	fprintf (stderr, "\n");
> +	fprintf (stderr, "    %s\n", xapian_path);
> +	fprintf (stderr, "\n");
> +	fprintf (stderr, "Please identify the reason for this and move the compacted database into place manually.\n");
> +	if (backup_path != NULL) {
> +	    fprintf (stderr, "Otherwise, you can revert to the backup database located at,\n");

the commas (,) here and a few lines above looks a bit strange to me. 

> +	    fprintf (stderr, "\n");
> +	    fprintf (stderr, "    %s\n", backup_path);

Maybe saying something how to do this revert would be enlightening to the
user... (i.e. mentioning that backup_path needs to be moved as xapian_path).


> +	}

>  	ret = NOTMUCH_STATUS_FILE_ERROR;
>  	goto DONE;
>      }
> -- 

Tomi