Date: Tue, 1 Apr 2014 23:35:42 -0400
From: Austin Clements <amdragon@MIT.EDU>
To: David Bremner <david@tethera.net>
Subject: Re: [Patch v5 5/6] notmuch-new: backup tags before database upgrade
Message-ID: <20140402033542.GC25677@mit.edu>
References: <1396401381-18128-1-git-send-email-david@tethera.net>
	<1396401381-18128-6-git-send-email-david@tethera.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1396401381-18128-6-git-send-email-david@tethera.net>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

Quoth David Bremner on Apr 01 at 10:16 pm:
> All we do here is calculate the backup filename, and call the existing
> dump routine.
> 
> Also take the opportity to add a message about being safe to
> interrupt.
> ---
>  notmuch-new.c        | 29 ++++++++++++++++++++++++++++-
>  test/T530-upgrade.sh |  4 +++-
>  2 files changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/notmuch-new.c b/notmuch-new.c
> index 82acf69..e0431c6 100644
> --- a/notmuch-new.c
> +++ b/notmuch-new.c
> @@ -989,8 +989,35 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
>  	    return EXIT_FAILURE;
>  
>  	if (notmuch_database_needs_upgrade (notmuch)) {
> -	    if (add_files_state.verbosity >= VERBOSITY_NORMAL)
> +	    time_t now = time (NULL);
> +	    struct tm *gm_time = gmtime (&now);
> +
> +	    /* since dump files are written atomically, the amount of
> +	     * harm from overwriting one within a second seems
> +	     * relatively small. */
> +
> +	    const char *backup_name =
> +		talloc_asprintf (notmuch, "%s/backup-%04d-%02d-%02d-%02d:%02d:%02d.gz",

Maybe "dump" instead of "backup" so it's more obvious that it's a
notmuch dump?

This would be ISO 8601 compatible if you put a 'T' instead of a '-'
between the date and the time.

Colons tend to get file names into trouble (some file systems don't
support them, tools like scp think they demarcate host names, etc).
How about compact ISO 8601: dump-%04d%02d%02dT%02d%02d%02d.gz?

> +				 dot_notmuch_path,
> +				 gm_time->tm_year + 1900,
> +				 gm_time->tm_mon + 1,
> +				 gm_time->tm_mday,
> +				 gm_time->tm_hour,
> +				 gm_time->tm_min,
> +				 gm_time->tm_sec);
> +
> +	    if (add_files_state.verbosity >= VERBOSITY_NORMAL) {
>  		printf ("Welcome to a new version of notmuch! Your database will now be upgraded.\n");
> +		printf ("This process is safe to interrupt.\n");
> +		printf ("Backing up tags to %s\n", backup_name);

Add "..." to indicate a running process?  Even better would be a
progress report, but we can add that to TODO.

> +	    }
> +
> +	    if (notmuch_database_dump (notmuch, backup_name, "",
> +				       DUMP_FORMAT_BATCH_TAG, TRUE)) {
> +		fprintf (stderr, "Backup failed. Aborting upgrade");

Add a "." at the end for consistency.

> +		return EXIT_FAILURE;
> +	    }
> +
>  	    gettimeofday (&add_files_state.tv_start, NULL);
>  	    notmuch_database_upgrade (notmuch,
>  				      add_files_state.verbosity >= VERBOSITY_NORMAL ? upgrade_print_progress : NULL,
> diff --git a/test/T530-upgrade.sh b/test/T530-upgrade.sh
> index 67bbf31..d46e3d1 100755
> --- a/test/T530-upgrade.sh
> +++ b/test/T530-upgrade.sh
> @@ -26,9 +26,11 @@ output=$(notmuch search path:foo)
>  test_expect_equal "$output" ""
>  
>  test_begin_subtest "database upgrade from format version 1"
> -output=$(notmuch new)
> +output=$(notmuch new | sed -e 's/^Backing up tags to .*$/Backing up tags to FILENAME/')
>  test_expect_equal "$output" "\
>  Welcome to a new version of notmuch! Your database will now be upgraded.
> +This process is safe to interrupt.
> +Backing up tags to FILENAME
>  Your notmuch database has now been upgraded to database format version 2.
>  No new mail."
>