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

[Michael Haggerty <mhagger@alum.mit.edu>]

On 05/16/2014 07:37 PM, 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.
> 
> Since ref update failures will now no longer occur in the code path for
> updating a single ref in s_update_ref, we no longer have as detailed error
> message logging the exact reference and the ref log action as in the old cod

s/cod/code/ ?

> Instead since we fail the entire transaction we log a much more generic
> message. But since we commit the transaction using MSG_ON_ERR we will log
> an error containing the ref name if either locking of writing the ref would

s/of/or/ ?
s/would/would fail,/ ?

> so the regression in the log message is minor.
> 
> This will also change the order in which errors are checked for and logged
> which may alter which error will be logged if there are multiple errors
> occuring during a fetch.

s/occuring/occurring/

> 
> For example, assume we have a fetch for two refs that both would fail.
> Where the first ref would fail with ENOTDIR due to a directory in the ref
> path not existing, and the second ref in the fetch would fail due to
> the check in update_logical_ref():
>         if (current_branch &&
>             !strcmp(ref->name, current_branch->name) &&
>             !(update_head_ok || is_bare_repository()) &&
>             !is_null_sha1(ref->old_sha1)) {
>                 /*
>                  * If this is the head, and it's not okay to update
>                  * the head, and the old value of the head isn't empty...
>                  */
> 
> In the old code since we would update the refs one ref at a time we would
> first fail the ENOTDIR and then fail the second update of HEAD as well.
> But since the first ref failed with ENOTDIR we would eventually fail the who

s/who/whole/

> fetch with STORE_REF_ERROR_DF_CONFLICT
> 
> In the new code, since we defer committing the transaction until all refs
> have been processed, we would now detect that the second ref was bad and
> rollback the transaction before we would even try start writing the update t

s/try/try to/
s/t$/to/

> disk and thus we would not return STORE_REF_ERROR_DF_CONFLICT for this case.
> 
> I think this new behaviour is more correct, since if there was a problem
> we would not even try to commit the transaction but need to highlight this
> change in how/what errors are reported.
> This change in what error is returned only occurs if there are multiple
> refs that fail to update and only some, but not all, of them fail due to
> ENOTDIR.

Thanks for the detailed explanation.  The change in behavior seems
reasonable to me.

> 
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
>  builtin/fetch.c | 34 ++++++++++++++++------------------
>  1 file changed, 16 insertions(+), 18 deletions(-)
> 
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 8cf70cd..5b0cc31 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -45,6 +45,7 @@ static struct transport *gsecondary;
>  static const char *submodule_prefix = "";
>  static const char *recurse_submodules_default;
>  static int shown_url;
> +static struct ref_transaction *transaction;
>  
>  static int option_parse_recurse_submodules(const struct option *opt,
>  				   const char *arg, int unset)
> @@ -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;
> -
>  	if (dry_run)
>  		return 0;
> -	if (!rla)
> -		rla = default_rla.buf;
> -	snprintf(msg, sizeof(msg), "%s: %s", rla, action);
>  
> -	errno = 0;
> -	transaction = ref_transaction_begin();
> -	if (!transaction ||
> -	    ref_transaction_update(transaction, ref->name, ref->new_sha1,
> -				   ref->old_sha1, 0, check_old, NULL) ||
> -	    ref_transaction_commit(transaction, msg, NULL)) {
> -		ref_transaction_free(transaction);
> -		return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
> -					  STORE_REF_ERROR_OTHER;
> -	}
> -	ref_transaction_free(transaction);
> +	if (ref_transaction_update(transaction, ref->name, ref->new_sha1,
> +				   ref->old_sha1, 0, check_old, NULL))
> +		return STORE_REF_ERROR_OTHER;
> +
>  	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;
> +	}
> +
>  	/*
>  	 * We do a pass for each fetch_head_status type in their enum order, so
>  	 * merged entries are written before not-for-merge. That lets readers
> @@ -676,6 +670,10 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
>  			}
>  		}
>  	}
> +	if (ref_transaction_commit(transaction, "fetch_ref transaction", NULL))
> +		rc |= errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
> +		  STORE_REF_ERROR_OTHER;
> +	ref_transaction_free(transaction);
>  
>  	if (rc & STORE_REF_ERROR_DF_CONFLICT)
>  		error(_("some local refs could not be updated; try running\n"
> 


-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/