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

Justin Tobler <[email protected]> writes:

> 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.

> 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.