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

On 26/08/09 08:38PM, Junio C Hamano wrote:

Justin Tobler [off-list ref] writes:

quoted

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.

In practice, only call sites that invoke `odb_transaction_write_pack()`
(only git-receive-pack(1) for now) actually need to be concerned about
any deferred clean up outside of just freeing some memory. Conceptually
this is a bit messy though and callers shouldn't ideally have to be
aware of such specifics.

It may make sense to align the clean-up as you suggested above. I will
explore in the next version.

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

quoted

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?

In version 1 I did keep `odb_transaction_finalize()` void, but decided
to at least provide the option for callers to check for errors if they
wished. The existing callers don't, but there isn't a reason most of the
couldn't be more strict here. In the next version, similar to
`odb_transaction_begin_or_die()`, I may add an
`odb_transaction_finalize_or_die()` helper and adapt some of the
existing callers.

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

There is one `odb_transaction_commit()` caller in
"builtin/receive-pack.c" that does check for errors, but ya all other
callers simply ignore them. For the same reasons mentioned above, I
opted to follow the existing behavior of ignoring temporary directory
related errors, but include error reporting as part of the interface in
case callers wanted to check. I could also add an
`odb_transaction_commit_or_die()` helper here too and adapt callers
where it is reasonable to be more strict.

-Justin