Re: [PATCH v3 11/11] builtin/receive-pack: stage incoming objects via ODB transactions

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

Justin Tobler <jltobler@gmail.com> writes:

> @@ -2027,6 +2031,7 @@ static void execute_commands_atomic(struct command *commands,
>  static void execute_commands(struct command *commands,
>  			     const char *unpacker_error,
>  			     struct shallow_info *si,
> +			     struct odb_transaction *transaction,
>  			     const struct string_list *push_options)
>  {
>  	struct check_connected_options opt = CHECK_CONNECTED_INIT;
> ...

Hidden in the context early in this function is an error return.
When unpacker_error string is non NULL, we mark all the commands in
the linked commands list as failed, and return early from this
function.

> @@ -2105,14 +2115,13 @@ static void execute_commands(struct command *commands,
>  	 * Now we'll start writing out refs, which means the objects need
>  	 * to be in their final positions so that other processes can see them.
>  	 */
> -	if (tmp_objdir_migrate(tmp_objdir) < 0) {
> +	if (odb_transaction_commit(transaction)) {
>  		for (cmd = commands; cmd; cmd = cmd->next) {
>  			if (!cmd->error_string)
>  				cmd->error_string = "unable to migrate objects to permanent storage";
>  		}
>  		return;
>  	}
> -	tmp_objdir = NULL;
>  
>  	check_aliased_updates(commands);

In the "happy case", execute_commands() would commit the transaction
before going on to do the execute_commands_{atomic,nonatomic}() that
appears later in it.

> @@ -2706,11 +2705,14 @@ int cmd_receive_pack(int argc,
>  		if (!si.nr_ours && !si.nr_theirs)
>  			shallow_update = 0;
>  		if (!delete_only(commands)) {
> -			unpack_status = unpack_with_sideband(&si);
> +			if (odb_transaction_begin(the_repository->objects, &transaction, ODB_TRANSACTION_RECEIVE))

In the "main" program, we start a transaction here, and

> +				unpack_status = "unable to start object transaction";
> +			else
> +				unpack_status = unpack_with_sideband(&si, transaction);

then call unpack_with_sideband().  It may fail.

>  			update_shallow_info(commands, &si, &ref);
>  		}
>  		use_keepalive = KEEPALIVE_ALWAYS;
> -		execute_commands(commands, unpack_status, &si,
> +		execute_commands(commands, unpack_status, &si, transaction,
>  				 &push_options);

And in such a case, execute_commands() returns without committing
the transaction.  Is there a need to add and make an
odb_transaction_abort() call or something in such a case?
Everything should be cleaned up upon process exit, and on file based
backends, we probably let the tempfile/lockfile API do their thing
to clean up, but are there other things we may want to clean up?

>  		delete_tempfile(&pack_lockfile);
>  		sigchain_push(SIGPIPE, SIG_IGN);
> @@ -2719,7 +2721,7 @@ int cmd_receive_pack(int argc,
>  		else if (report_status)
>  			report(commands, unpack_status);
>  		sigchain_pop(SIGPIPE);
> -		run_receive_hook(commands, "post-receive", 1,
> +		run_receive_hook(commands, "post-receive", 1, NULL,
>  				 &push_options);
>  		run_update_post_hook(commands);
>  		free_commands(commands);