Re: [PATCH v3 2/9] odb/transaction: add transaction finalize interface

> When committing an ODB transaction via `odb_transaction_commit()`, the
> staged objects are made visible and the underlying transaction is freed
> at the same time. Coupling these two steps does not leave room for any
> post-commit transaction operations to be introduced though. Such a
> capability is useful if an ODB transaction backend needs to hold on to
> lockfiles after transaction commit until references are updated, as is
> the case with the existing "files" backend in git-receive-pack(1).
> 
> Stop freeing the transaction in `odb_transaction_commit()` and introduce
> `odb_transaction_finalize()` to explicitly clean up the transaction
> accordingly. Note that the finalize interface also provides an optional
> callback for any backend-specific deferred cleanup. In a subsequent
> commit, the "files" transaction backend will use this to remove ".keep"
> files generated for packfiles received via git-receive-pack(1) after
> references have been updated. In preparation for this, the
> `odb_transaction_finalize()` call site in git-receive-pack(1) is made
> after the reference updates are finished.
> 
> All other callers commit a transaction and immediately finalize it with
> no work in between and cannot meaningfully recover should either step
> fail, so introduce an `odb_transaction_commit_and_finalize_or_die()`

> helper that performs both and dies on error. Call sites are updated
> accordingly.
> 
> Signed-off-by: Justin Tobler <jltobler@gmail.com>
> ---
>  builtin/add.c            |  4 ++--
>  builtin/receive-pack.c   |  1 +
>  builtin/unpack-objects.c |  2 +-
>  builtin/update-index.c   |  4 ++--
>  cache-tree.c             |  2 +-
>  object-file.c            |  2 +-
>  odb/transaction.c        | 14 ++++++++++++++
>  odb/transaction.h        | 23 +++++++++++++++++++++++
>  read-cache.c             |  2 +-
>  9 files changed, 46 insertions(+), 8 deletions(-)
> 
> diff --git a/builtin/add.c b/builtin/add.c
> index 60ffbede2b..ad418a5952 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -393,7 +393,7 @@ int cmd_add(int argc,
>  	char *seen = NULL;
>  	char *ps_matched = NULL;
>  	struct lock_file lock_file = LOCK_INIT;
> -	struct odb_transaction *transaction;
> +	struct odb_transaction *transaction = NULL;
>  
>  	repo_config(repo, add_config, NULL);
>  
> @@ -600,7 +600,7 @@ int cmd_add(int argc,
>  
>  	if (chmod_arg && pathspec.nr)
>  		exit_status |= chmod_pathspec(repo, &pathspec, chmod_arg[0], show_only);
> -	odb_transaction_commit(transaction);
> +	odb_transaction_commit_and_finalize_or_die(transaction);
>  
>  finish:
>  	if (write_locked_index(repo->index, &lock_file,
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index d74b787148..ed1edcbe93 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -2720,6 +2720,7 @@ int cmd_receive_pack(int argc,
>  		use_keepalive = KEEPALIVE_ALWAYS;
>  		execute_commands(commands, unpack_status, &si, transaction,
>  				 &push_options);
> +		odb_transaction_finalize(transaction);
>  		delete_tempfile(&pack_lockfile);
>  		sigchain_push(SIGPIPE, SIG_IGN);
>  		if (report_status_v2)
> diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
> index 4263edfbec..d6a2d616d9 100644
> --- a/builtin/unpack-objects.c
> +++ b/builtin/unpack-objects.c
> @@ -603,7 +603,7 @@ static void unpack_all(void)
>  		unpack_one(i);
>  		display_progress(progress, i + 1);
>  	}
> -	odb_transaction_commit(transaction);
> +	odb_transaction_commit_and_finalize_or_die(transaction);
>  	stop_progress(&progress);
>  
>  	if (delta_list)
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index 241abd4332..b25d4ecb10 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -1156,7 +1156,7 @@ int cmd_update_index(int argc,
>  			 * a transaction.
>  			 */
>  			if (transaction && verbose) {
> -				odb_transaction_commit(transaction);
> +				odb_transaction_commit_and_finalize_or_die(transaction);
>  				transaction = NULL;
>  			}
>  
> @@ -1224,7 +1224,7 @@ int cmd_update_index(int argc,
>  	/*
>  	 * By now we have added all of the new objects
>  	 */
> -	odb_transaction_commit(transaction);
> +	odb_transaction_commit_and_finalize_or_die(transaction);
>  
>  	if (split_index > 0) {
>  		if (repo_config_get_split_index(the_repository) == 0)
> diff --git a/cache-tree.c b/cache-tree.c
> index d92f513286..a220372a42 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -538,7 +538,7 @@ int cache_tree_update(struct index_state *istate, int flags)
>  	i = update_one(istate->cache_tree, istate->cache, istate->cache_nr,
>  		       "", 0, &skip, flags);
>  	if (!inflight)
> -		odb_transaction_commit(transaction);
> +		odb_transaction_commit_and_finalize_or_die(transaction);
>  	trace2_region_leave("cache_tree", "update", istate->repo);
>  	trace_performance_leave("cache_tree_update");
>  	if (i < 0)
> diff --git a/object-file.c b/object-file.c
> index ec35c318bc..4d03c167d5 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -965,7 +965,7 @@ int index_fd(struct index_state *istate, struct object_id *oid,
>  								  xsize_t(st->st_size),
>  								  oid);
>  			if (!inflight)
> -				odb_transaction_commit(transaction);
> +				odb_transaction_commit_and_finalize_or_die(transaction);
>  		} else {
>  			ret = hash_blob_stream(&stream,
>  					       the_repository->hash_algo, oid,
> diff --git a/odb/transaction.c b/odb/transaction.c
> index dab7da6a9a..9e9a982778 100644
> --- a/odb/transaction.c
> +++ b/odb/transaction.c
> @@ -33,6 +33,20 @@ int odb_transaction_commit(struct odb_transaction *transaction)
>  
>  	ret = transaction->commit(transaction);
>  	transaction->source->odb->transaction = NULL;
> +
> +	return ret;
> +}
> +
> +int odb_transaction_finalize(struct odb_transaction *transaction)
> +{
> +	int ret = 0;
> +
> +	if (!transaction)
> +		return 0;
> +
> +	if (transaction->finalize)
> +		ret = transaction->finalize(transaction);
> +
>  	free(transaction);
>  
>  	return ret;
> diff --git a/odb/transaction.h b/odb/transaction.h
> index 4cb2eafcbf..6ed39b3d0e 100644
> --- a/odb/transaction.h
> +++ b/odb/transaction.h
> @@ -22,6 +22,13 @@ struct odb_transaction {
>  	 */
>  	int (*commit)(struct odb_transaction *transaction);
>  
> +	/*
> +	 * Optional ODB source specific callback invoked when the transaction
> +	 * needs to perform any deferred cleanup after objects have been
> +	 * committed. Returns 0 on success, a negative error code otherwise.
> +	 */
> +	int (*finalize)(struct odb_transaction *transaction);
> +
>  	/*
>  	 * This callback is expected to write the given object stream into
>  	 * the ODB transaction. Note that for now, only blobs support streaming.
> @@ -75,6 +82,22 @@ static inline void odb_transaction_begin_or_die(struct object_database *odb,
>   */
>  int odb_transaction_commit(struct odb_transaction *transaction);
>  
> +/*
> + * Finalizes an ODB transaction, performing any deferred cleanup and freeing it.
> + * Must be called for every successfully started transaction. Note that, if the
> + * specified transaction is NULL, the function is a no-op. Returns 0 on success,
> + * a negative error code otherwise.
> + */
> +int odb_transaction_finalize(struct odb_transaction *transaction);
> +
> +static inline void odb_transaction_commit_and_finalize_or_die(struct odb_transaction *transaction)
> +{
> +	if (odb_transaction_commit(transaction))
> +		die(_("failed to commit ODB transaction"));
> +	if (odb_transaction_finalize(transaction))
> +		die(_("failed to finalize ODB transaction"));
> +}
> +
>  /*
>   * Writes the object in the provided stream into the transaction. The resulting
>   * object ID is written into the out pointer. Returns 0 on success, a negative
> diff --git a/read-cache.c b/read-cache.c
> index 6c449f393d..0cd0ef85ec 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -4049,7 +4049,7 @@ int add_files_to_cache(struct repository *repo, const char *prefix,
>  		odb_transaction_begin_or_die(repo->objects, &transaction, 0);
>  	run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);
>  	if (!inflight)
> -		odb_transaction_commit(transaction);
> +		odb_transaction_commit_and_finalize_or_die(transaction);
>  
>  	release_revisions(&rev);
>  	return !!data.add_errors;
> -- 
> 2.55.0.424.g13c7afec21
>