Re: [PATCH v8 17/44] fast-import.c: change update_branch to use ref transactions

From: Jonathan Nieder <jrnieder@gmail.com>
Date: May 15, 2014, 21:47 UTC
Message-ID: <20140515214745.GN26471@google.com>
In-Reply-To: <1400174999-26786-18-git-send-email-sahlberg@google.com>

Ronnie Sahlberg wrote:

Show 31 quoted lines

> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -1679,39 +1679,45 @@ found_entry:
>  static int update_branch(struct branch *b)
>  {
>  	static const char *msg = "fast-import";
> -	struct ref_lock *lock;
> +	struct ref_transaction *transaction;
>  	unsigned char old_sha1[20];
> +	struct strbuf err = STRBUF_INIT;
>  
>  	if (read_ref(b->name, old_sha1))
>  		hashclr(old_sha1);
> +
>  	if (is_null_sha1(b->sha1)) {
>  		if (b->delete)
>  			delete_ref(b->name, old_sha1, 0);
>  		return 0;
>  	}
> -	lock = lock_any_ref_for_update(b->name, old_sha1, 0, NULL);
> -	if (!lock)
> -		return error("Unable to lock %s", b->name);
>  	if (!force_update && !is_null_sha1(old_sha1)) {
>  		struct commit *old_cmit, *new_cmit;
>  
>  		old_cmit = lookup_commit_reference_gently(old_sha1, 0);
>  		new_cmit = lookup_commit_reference_gently(b->sha1, 0);
>  		if (!old_cmit || !new_cmit) {
> -			unlock_ref(lock);
>  			return error("Branch %s is missing commits.", b->name);
>  		}

(style) Now that there's only one line in the "if" body, we can drop the braces.

Show 7 quoted lines

>  
>  		if (!in_merge_bases(old_cmit, new_cmit)) {
> -			unlock_ref(lock);
>  			warning("Not updating %s"
>  				" (new tip %s does not contain %s)",
>  				b->name, sha1_to_hex(b->sha1), sha1_to_hex(old_sha1));
>  			return -1;

(not about this patch, feel free to ignore) This could return warning("...")

Show 7 quoted lines

>  	}
> -	if (write_ref_sha1(lock, b->sha1, msg) < 0)
> -		return error("Unable to update %s", b->name);
> +	transaction = ref_transaction_begin();
> +	if ((!transaction ||
> +	    ref_transaction_update(transaction, b->name, b->sha1, old_sha1,
> +				   0, 1)) ||

Would be more idiomatic to drop a layer of parentheses:

	if (!transaction ||
	    ref_transaction_update(...) ||

> +	    (ref_transaction_commit(transaction, msg, &err) &&
> +	     !(transaction = NULL))) {

Would be clearer if ref_transaction_commit didn't take care of the rollback (or in other words if patch 21 were earlier in the series).

Show 5 quoted lines

> +		ref_transaction_rollback(transaction);
> +		error("Unable to update branch %s: %s", b->name, err.buf);
> +		strbuf_release(&err);
> +		return -1;
> +	}

Example old message:

	error: Unable to lock refs/heads/master

New message:

	error: Unable to update branch refs/heads/master: Cannot lock the ref 'refs/heads/master'.

So 'error("%s", err.buf)' would probably work better.

The only call site is dump_branches:

	for (i = 0; i < branch_table_sz; i++) {
		for (b = branch_table[i]; b; b = b->table_next_branch)
			failure |= update_branch(b);
	}

Should these happen in a single transaction? I haven't thought through whether it would be a good idea, if it should be optional, or what.

Anyway, that would be a bigger behavior change, but it's interesting to think about.

Thanks, Jonathan