Re: [PATCH 03/15] refs.c: use packed refs when deleting refs during a transaction

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

Ronnie Sahlberg <sahlberg@google.com> writes:

> commit fb5fa1d338ce113b0fea3bb955b50bbb3e827805 upstream.

Huh?

> Make the deletion of refs during a transaction more atomic.
> Start by first copying all loose refs we will be deleting to the packed
> refs file and then commit the packed refs file. Then re-lock the packed refs
> file to stop anyone else from modifying these refs and keep it locked until
> we are finished.
> Since all refs we are about to delete are now safely held in the packed refs
> file we can proceed to immediately unlink any corresponding loose refs
> and still be fully rollback-able.
>
> The exception is for refs that can not be resolved. Those refs are never
> added to the packed refs and will just be un-rollback-ably deleted during
> commit.
>
> By deleting all the loose refs at the start of the transaction we make make
> it possible to both delete one ref and then re-create a different ref in
> the same transaction even if the two refs would normally conflict.
>
> Example: rename m->m/m
>
> In that example we want to delete the file 'm' so that we make room so
> that we can create a directory with the same name in order to lock and
> write to the ref m/m and its lock-file m/m.lock.
>
> If there is a failure during the commit phase we can rollback without losing
> any refs since we have so far only deleted loose refs that that are
> guaranteed to also have a corresponding entry in the packed refs file.
> Once we have finished all updates for refs and their reflogs we can repack
> the packed refs file and remove the to-be-deleted refs from the packed refs,
> at which point all the deleted refs will disappear in one atomic rename
> operation.
>
> This also means that for an outside observer, deletion of multiple refs
> in a single transaction will look atomic instead of one ref being deleted
> at a time.
>
> In order to do all this we need to change the semantics for the
> repack_without_refs function so that we can lock the packed refs file,
> do other stuff, and later be able to call repack_without_refs with the
> lock already taken.
> This means we need some additional changes in remote.c to reflect the
> changes to the repack_without_refs semantics.
>
> Change-Id: I1e4f58050acaabc6bcfa3409fbc0c2b866bb59aa
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>


> @@ -3821,23 +3824,109 @@ int transaction_commit(struct transaction *transaction,
> ...
> +	if (need_repack) {
> +		packed = get_packed_refs(&ref_cache);;
> +		sort_ref_dir(packed);
> +		if (commit_packed_refs()){

SP between "){".

> +			strbuf_addf(err, "unable to overwrite old ref-pack "
> +				    "file");

This is bending backwards sacrificing readability; 80-col limit is
not that hard a limit.  Split at the comma instead, perhaps?

			strbuf_addf(err,
				    "unable to overwrite old ref-pack file");

> ...
> +		if (!update->lock) {
> +			int df_conflict = (errno == ENOTDIR);
> +
> +			strbuf_addf(err, "Cannot lock the ref '%s'.",
> +				    update->refname);
> +			ret = df_conflict ?
> +			  TRANSACTION_NAME_CONFLICT : 

A trailing SP here...

> @@ -3860,6 +3953,16 @@ int transaction_commit(struct transaction *transaction,
>  			update->reflog_lock = update->orig_update->reflog_lock;
>  			continue;
>  		}
> +		if (log_all_ref_updates && !reflog_exists(update->refname) &&
> +		    create_reflog(update->refname)) {
> +			ret = -1;
> +			if (err)
> +				strbuf_addf(err, "Failed to setup reflog for "
> +					    "%s", update->refname);

Split after comma, perhaps?