Re: [PATCH v2 1/7] odb/transaction: add transaction finalize interface

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 10, 2026, 03:38 UTC
Message-ID: <xmqq33wm8x20.fsf@gitster.g>
In-Reply-To: <20260809190106.1565882-2-jltobler@gmail.com>

Justin Tobler <jltobler@gmail.com> writes:

Show 20 quoted lines

> diff --git a/builtin/add.c b/builtin/add.c
> index 60ffbede2b..501e114ed5 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);
>  
> @@ -610,5 +610,6 @@ int cmd_add(int argc,
>  	free(ps_matched);
>  	dir_clear(&dir);
>  	clear_pathspec(&pathspec);
> +	odb_transaction_finalize(transaction);
>  	return exit_status;
>  }

There is only one non-local exit between transation-begin and transaction-finalize, which is a call ot report_path_error() followed by exit(128). Will _finalize() stay to be just freeing memory and nothing else? It may be conceptually cleaner to jump to the bottom to make sure the clean-up sequence will always happen.

The same comment applies to other codepaths to which this patch adds _finalize() calls.

Show 14 quoted lines

> 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)
> +{

Curiously no callers added by this patch checks the return value of this function. Intended or just sloppy? If the former, perhaps this wants to return void instead?

The same can be said for _commit(), by the way.