Re: [PATCH v8 24/44] fetch.c: use a single ref transaction for all ref updates

[Jonathan Nieder <jrnieder@gmail.com>]

Ronnie Sahlberg wrote:

> Change store_updated_refs to use a single ref transaction for all refs that
> are updated during the fetch. This makes the fetch more atomic when update
> failures occur.

Fun.

[...]
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
[...]
> @@ -373,27 +374,13 @@ static int s_update_ref(const char *action,
>  			struct ref *ref,
>  			int check_old)
>  {
> -	char msg[1024];
> -	char *rla = getenv("GIT_REFLOG_ACTION");
> -	struct ref_transaction *transaction;

Previously fetch respected GIT_REFLOG_ACTION, and this is dropping
that support.  Intended?

[...]
> +	if (ref_transaction_update(transaction, ref->name, ref->new_sha1,
> +			       ref->old_sha1, 0, check_old))
> +		return STORE_REF_ERROR_OTHER;

Should pass a strbuf in to get a message back.

[...]
> +
>  	return 0;
>  }
>  
> @@ -565,6 +552,13 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
>  		goto abort;
>  	}
>  
> +	errno = 0;
> +	transaction = ref_transaction_begin();
> +	if (!transaction) {
> +		rc = error(_("cannot start ref transaction\n"));
> +		goto abort;

error() appends a newline on its own, so the message shouldn't
end with newline.

More importantly, this message isn't helpful without more detail about
why _transaction_begin() failed.  The user doesn't know what

	error: cannot start ref transaction

means.

	error: cannot connect to mysqld for ref storage: [etc]

would tell what they need to know.  That is:

	transaction = ref_transaction_begin(err);
	if (!transaction) {
		rc = error("%s", err.buf);
		goto abort;
	}

errno is not used here, so no need to set errno = 0.

[...]
> @@ -676,6 +670,10 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
>  			}
>  		}
>  	}
> +	if ((ret = ref_transaction_commit(transaction, NULL)))
> +		rc |= ret == -2 ? STORE_REF_ERROR_DF_CONFLICT :
> +				STORE_REF_ERROR_OTHER;

(I cheated and stole the newer code from your repo.)

Yay!  Style nit: git avoids assignments in "if" conditionals.

	ret = ref_tr...
	if (ret == -2)
		rc |= ...
	else if (ret)
		rc |= ...

Does _update need the too as futureproofing for backends that can
detect the name conflict sooner?

Thanks,
Jonathan