Re: [PATCH v2 7/7] odb/transaction: add transaction interface to write packfiles

[Justin Tobler <jltobler@gmail.com>]

On 26/08/09 09:02PM, Junio C Hamano wrote:
> Justin Tobler <jltobler@gmail.com> writes:
> 
> > .... Note that a packfile
> > written via git-index-pack(1) is kept in place by a ".keep" lockfile
> > that must be retained until references are updated. To faciliate this in
> > an ODB backend agnostic manner, the "files" transaction backend takes
> > ownership of these lockfiles and removes them post-commit through its
> > release callback.
> 
> The above is confusing and I am lost.  Care to explain a bit more?

It should say "finalize callback" instead of "release callback". The
idea here though is that ".keep" files are likely an implementation
detail of the "files" backend, but we still need a way to clean them up
after a transaction is committed and reference updates have been
performed via the generic ODB transaction interface.

So after `odb_transaction_commit()`, the ".keep" files are tracked by
the "files" transaction and only removed once
`odb_transaction_finalize()` is invoked. It would be up to callers to
ensure that reference updates are performed as required prior to
finalize being invoked.

I'll try to explain this better in the next version.

> > +		status = start_command(&child);
> > +		if (status) {
> > +			strbuf_addstr(err_msg, "index-pack fork failed");
> > +			return -1;
> > +		}
> > +
> > +		lockfile = index_pack_lockfile(repo, child.out, NULL);
> > +		if (lockfile) {
> > +			ALLOC_GROW(transaction->pack_lockfiles,
> > +				   transaction->pack_lockfiles_nr + 1,
> > +				   transaction->pack_lockfiles_alloc);
> > +			transaction->pack_lockfiles[transaction->pack_lockfiles_nr++] =
> > +				register_tempfile(lockfile);
> > +			free(lockfile);
> > +		}
> 
> Here we add the .keep file to the list of lockfiles.  We have
> finalization step laer in odb_transaction_files_finalize() that
> deletes the tempfiles when we are done, which comes after
> the transaction is committed.
> 
> But isn't the odb_transaction_files_commit() where the migration of
> tmp_objdir_migrate() happens?  Everything in the quarantine directory
> including these .keep files are "migrated" (either link-to-the-new
> followed by unlink-of-the-old, or rename-old-to-new) there.
> 
> And then ...
> 
> > +static int odb_transaction_files_finalize(struct odb_transaction *base)
> > +{
> > +	struct odb_transaction_files *transaction =
> > +		container_of(base, struct odb_transaction_files, base);
> > +	int ret = 0;
> > +
> > +	for (size_t i = 0; i < transaction->pack_lockfiles_nr; i++)
> > +		ret |= delete_tempfile(&transaction->pack_lockfiles[i]);
> > +
> > +	free(transaction->pack_lockfiles);
> > +
> > +	return ret;
> > +}
> 
> ... we do the deletion of tempfile but has anybody migrated the path
> to these files recorded in the lockfile structure?  How are we
> removing the .keep files that were "migrated" when the transaction
> was committed?

The filepath recorded by the ".keep" tempfile structure is _supposed_ to
be the final path of the ".keep" file post-commit that way it knows its
location after its been migrated and can remove it. The path is
generated by `index_pack_lockfile()` and is supposed to use the real ODB
source path and not the transaction ODB source path... but this is not
happening anymore now that we are using ODB transactions in
git-receive-pack(1) which reorders the ODB source list order when the
transaction is applied...

This is a bug and needs to be fixed. The fix itself should be fairly
straightforward, we just need to tell `index_pack_lockfile()` the
correct ODB source it should be using. I'll send a patch later today
correct this.

Thanks,
-Justin