Subject: Re: Transaction patch series overview

From: Junio C Hamano <[email-masked]>
Date: Tue, 26 Aug 2014 14:01:29 -0700

Jonathan Nieder <[email-masked]> writes:

>>  https://code-review.googlesource.com/#/q/topic:ref-transaction-1
>
> Outcome of the experiment: patches gained some minor changes and then
>
>  1-12
> 	Reviewed-by: Jonathan Nieder <[email-masked]>
>
>  13
> 	Reviewed-by: Michael Haggerty <[email-masked]>
> 	Reviewed-by: Jonathan Nieder <[email-masked]>
>
>  14
> 	Reviewed-by: Jonathan Nieder <[email-masked]>
>
>  15-16
> 	Reviewed-by: Michael Haggerty <[email-masked]>
> 	Reviewed-by: Jonathan Nieder <[email-masked]>
>
>  17
> 	Reviewed-by: Michael Haggerty <[email-masked]>
>
>  18
> 	Reviewed-by: Michael Haggerty <[email-masked]>
> 	Reviewed-by: Jonathan Nieder <[email-masked]>
>
>  19
> 	Reviewed-by: Jonathan Nieder <[email-masked]>
>
>  20
> 	Reviewed-by: Michael Haggerty <[email-masked]>
> 	Reviewed-by: Jonathan Nieder <[email-masked]>
>
> I found the web UI helpful in seeing how each patch evolved since I
> last looked at it.  Interdiff relative to the version in pu is below.

Thanks for the interdiff, it really helps to be able to take a
glance without having to click around.

It seems that I can hold the topic in 'pu' a bit longer and expect a
reroll from this effort before merging it to 'next'?

> The next series from Ronnie's collection is available at
> https://code-review.googlesource.com/#/q/topic:ref-transaction in case
> someone wants a fresh series to look at.

Thanks.

> diff --git a/builtin/commit.c b/builtin/commit.c
> index 668fa6a..9bf1003 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1765,8 +1765,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  	transaction = ref_transaction_begin(&err);
>  	if (!transaction ||
>  	    ref_transaction_update(transaction, "HEAD", sha1,
> -				   current_head ?
> -				   current_head->object.sha1 : NULL,
> +				   current_head
> +				   ? current_head->object.sha1 : NULL,
>  				   0, !!current_head, &err) ||
>  	    ref_transaction_commit(transaction, sb.buf, &err)) {
>  		rollback_index_files();

Perhaps this is nicer, but probably most people would not care.

> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 91099ad..224fadc 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -580,18 +580,18 @@ static char *update(struct command *cmd, struct shallow_info *si)
>  
>  		if (shallow_update && si->shallow_ref[cmd->index] &&
>  		    update_shallow_ref(cmd, si))
> -			return xstrdup("shallow error");
> +			return "shallow error";
>  
>  		transaction = ref_transaction_begin(&err);
>  		if (!transaction ||
>  		    ref_transaction_update(transaction, namespaced_name,
>  					   new_sha1, old_sha1, 0, 1, &err) ||
>  		    ref_transaction_commit(transaction, "push", &err)) {
> -			char *str = strbuf_detach(&err, NULL);
>  			ref_transaction_free(transaction);
>  
> -			rp_error("%s", str);
> -			return str;
> +			rp_error("%s", err.buf);
> +			strbuf_release(&err);
> +			return "failed to update ref";
>  		}

I am guessing that the purpose of this change is to make sure that
cmd->error_string is taken from a fixed set of strings, not an
arbitrary collection of errors from the transaction subsystem, but
what is the significance of doing so?  Do the other side i18n while
running print_ref_status() or something?

> diff --git a/refs.h b/refs.h
> index aad846c..69ef28c 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -180,8 +180,7 @@ extern int peel_ref(const char *refname, unsigned char *sha1);
>   */
>  #define REF_NODEREF	0x01
>  /*
> - * Locks any ref (for 'HEAD' type refs) and sets errno to something
> - * meaningful on failure.
> + * This function sets errno to something meaningful on failure.
>   */
>  extern struct ref_lock *lock_any_ref_for_update(const char *refname,
>  						const unsigned char *old_sha1,

To contrast this function with lock_ref_sha1() that only allowed
refs under refs/ hierarchy, the comment may have made sense, but now
that other function is gone, losing the comment makes sense.

I removed from the above the interdiff hunks I did not have any
comments or questions on.

Thanks again.