Re: [PATCH v2 2/4] bulk-checkin: remove global transaction state

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

Justin Tobler <jltobler@gmail.com> writes:

> diff --git a/bulk-checkin.c b/bulk-checkin.c
> index 82a73da79e8..53a20a2d92f 100644
> --- a/bulk-checkin.c
> +++ b/bulk-checkin.c
> @@ -30,11 +30,13 @@ struct bulk_checkin_packfile {
>  	uint32_t nr_written;
>  };
>  
> -static struct odb_transaction {
> +struct odb_transaction {
> +	struct object_database *odb;
> +
>  	int nesting;
>  	struct tmp_objdir *objdir;
>  	struct bulk_checkin_packfile packfile;
> -} transaction;
> +};

Very nice to see that this singleton instance is now gone.  

The singleton was really handy way to allow calls into this
subsystem to lazily initialize bulk_checkin_packfile in the call
chain starting from deflate_blob_to_pack() -> prepare_to_stream().

We now need to be a lot more careful to make sure that everybody has
access to a valid bulk_checkin_packfile struct, which makes the
implementation of index_blob_bulk_checkin() a bit awkward and we
need to invent one bulk_checkin_packfile instance right there.
Luckily it goes away in the next step, I guess?

> +int index_blob_bulk_checkin(struct odb_transaction *transaction,
> +			    struct object_id *oid, int fd, size_t size,
>  			    const char *path, unsigned flags)
>  {
> -	int status = deflate_blob_to_pack(&transaction.packfile, oid, fd, size,
> -					  path, flags);
> -	if (!transaction.nesting)
> -		flush_bulk_checkin_packfile(&transaction.packfile);
> +	int status;
> +
> +	if (transaction) {
> +		status = deflate_blob_to_pack(&transaction->packfile, oid, fd,
> +					      size, path, flags);
> +	} else {
> +		struct bulk_checkin_packfile state = { 0 };
> +
> +		status = deflate_blob_to_pack(&state, oid, fd, size, path, flags);
> +		flush_bulk_checkin_packfile(&state);
> +	}
> +
>  	return status;
>  }

OK.  If we do not have a transaction (i.e. if nobody called 
begin_odb_transaction() more times than end_odb_transaction()
got called), we let the underlying machinery do the right thing on
an empty state and clean up after ourselves; otherwise we follow
exactly the same code path as we used to.

> diff --git a/cache-tree.c b/cache-tree.c
> index 66ef2becbe0..d225554eedd 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -474,6 +474,7 @@ static int update_one(struct cache_tree *it,
>  
>  int cache_tree_update(struct index_state *istate, int flags)
>  {
> +	struct odb_transaction *transaction;
>  	int skip, i;

OK, doing the odb-transaction here would cover both this one and
write_index_as_tree and its friends, that are public interfaces into
the cache-tree subsystem.  

> diff --git a/odb.h b/odb.h
> index 3dfc66d75a3..a89b2143909 100644
> --- a/odb.h
> +++ b/odb.h
> @@ -84,6 +84,7 @@ struct odb_source {
>  
>  struct packed_git;
>  struct cached_object_entry;
> +struct odb_transaction;
>  
>  /*
>   * The object database encapsulates access to objects in a repository. It
> @@ -94,6 +95,13 @@ struct object_database {
>  	/* Repository that owns this database. */
>  	struct repository *repo;
>  
> +	/*
> +	 * State of current current object database transaction. Only one
> +	 * transaction may be pending at a time. Is NULL when no transaction is
> +	 * configured.
> +	 */
> +	struct odb_transaction *transaction;

Once dust from this topic settles, we may want to rename the
bulk-checkin.[ch] to have "odb" somewhere in its name, perhaps?  I
usually do not like renaming file for the sake of renaming to make
the result look "pretty" (people may use "consistent naming" ),
though.  I dunno.