Re: [PATCH 2/6] object-file: propagate files transaction errors

[Junio C Hamano <gitster@pobox.com>]

Justin Tobler <jltobler@gmail.com> writes:

> The "files" transaction backend may encounter errors related to managing
> the temporary directory used to stage objects, but silently ignores
> these errors. Instead return errors encountered in the
> `odb_transaction_files_{prepare,begin,commit}()` interfaces to allow
> callers to handle as needed.

"handle them as needed", perhaps.

> -static void odb_transaction_files_prepare(struct odb_transaction *base)
> +static int odb_transaction_files_prepare(struct odb_transaction *base)
>  {
>  	struct odb_transaction_files *transaction =
>  		container_of_or_null(base, struct odb_transaction_files, base);
> @@ -511,11 +511,15 @@ static void odb_transaction_files_prepare(struct odb_transaction *base)
>  	 * added at the time they call odb_transaction_files_begin.
>  	 */
>  	if (!transaction || transaction->objdir)
> -		return;
> +		return 0;
>  
>  	transaction->objdir = tmp_objdir_create(base->source->odb->repo, "bulk-fsync");

If this fails, NULL is returned, and ...

> -	if (transaction->objdir)
> -		tmp_objdir_replace_primary_odb(transaction->objdir, 0);
> +	if (!transaction->objdir)
> +		return -1;

... we return -1 from here to signal an error now.

But callers of this function in write_loose_object(), and
odb_source_loose_write_stream() are not prepared to react to such an
error.

I guess this is nothing new.  The callers ignored such an error from here
in the original and proceeded writing the primary ODB anyway, and we
continue to do so after this step.

> @@ -542,13 +546,13 @@ static void fsync_loose_object_transaction(struct odb_transaction *base,
>  /*
>   * Cleanup after batch-mode fsync_object_files.
>   */
> -static void flush_loose_object_transaction(struct odb_transaction_files *transaction)
> +static int flush_loose_object_transaction(struct odb_transaction_files *transaction)
>  {
>  	struct strbuf temp_path = STRBUF_INIT;
>  	struct tempfile *temp;
>  
>  	if (!transaction->objdir)
> -		return;
> +		return 0;
>  
>  	/*
>  	 * Issue a full hardware flush against a temporary file to ensure
> @@ -570,8 +574,12 @@ static void flush_loose_object_transaction(struct odb_transaction_files *transac
>  	 * Make the object files visible in the primary ODB after their data is
>  	 * fully durable.
>  	 */
> -	tmp_objdir_migrate(transaction->objdir);
> +	if (tmp_objdir_migrate(transaction->objdir))
> +		return -1;
> +
>  	transaction->objdir = NULL;
> +
> +	return 0;
>  }

The caller of this function does react to a failure of it, ...

> @@ -1670,27 +1678,34 @@ int read_loose_object(struct repository *repo,
>  	return ret;
>  }
>  
> -static void odb_transaction_files_commit(struct odb_transaction *base)
> +static int odb_transaction_files_commit(struct odb_transaction *base)
>  {
>  	struct odb_transaction_files *transaction =
>  		container_of(base, struct odb_transaction_files, base);
>  
> -	flush_loose_object_transaction(transaction);
> +	if (flush_loose_object_transaction(transaction))
> +		return -1;
>  	flush_packfile_transaction(transaction);
> +
> +	return 0;
>  }

... like this, which is good.  Do we need an explicit "abort-transaction",
or is that implicit?

Thanks.