Re: [PATCH 05/11] tag.c: use ref transactions when doing updates

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

On 04/17/2014 09:46 PM, Ronnie Sahlberg wrote:
> Change tag.c to use ref transactions for all ref updates.
> 
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
>  builtin/tag.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 40356e3..dbeacc5 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -488,7 +488,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  	struct strbuf ref = STRBUF_INIT;
>  	unsigned char object[20], prev[20];
>  	const char *object_ref, *tag;
> -	struct ref_lock *lock;
>  	struct create_tag_options opt;
>  	char *cleanup_arg = NULL;
>  	int annotate = 0, force = 0, lines = -1;
> @@ -496,6 +495,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  	const char *msgfile = NULL, *keyid = NULL;
>  	struct msg_arg msg = { 0, STRBUF_INIT };
>  	struct commit_list *with_commit = NULL;
> +	struct ref_transaction *transaction;
>  	struct option options[] = {
>  		OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
>  		{ OPTION_INTEGER, 'n', NULL, &lines, N_("n"),
> @@ -641,11 +641,14 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  	if (annotate)
>  		create_tag(object, tag, &buf, &opt, prev, object);
>  
> -	lock = lock_any_ref_for_update(ref.buf, prev, 0, NULL);
> -	if (!lock)
> -		die(_("%s: cannot lock the ref"), ref.buf);
> -	if (write_ref_sha1(lock, object, NULL) < 0)
> -		die(_("%s: cannot update the ref"), ref.buf);
> +	transaction = ref_transaction_begin();
> +	if (!transaction)
> +		die(_("%s: cannot start transaction"), ref.buf);
> +	if (ref_transaction_update(transaction, ref.buf, object, prev,
> +				   0, !is_null_sha1(prev)))
> +		die(_("%s: cannot update transaction"), ref.buf);
> +	if (ref_transaction_commit(transaction, NULL, UPDATE_REFS_DIE_ON_ERR))
> +		die(_("%s: cannot commit transaction"), ref.buf);

I wonder whether these error messages are meaningful to the user.  To
the user, it's all just a failure to create the tag, right?  Does it
help to tell the user that the error was in the "start transaction",
"update transaction", or "commit transaction" phase?

In other words, maybe it would be just as good to do

if (!transaction ||
    ref_transaction_update(transaction, ref.buf, object, prev,
			   0, !is_null_sha1(prev)) ||
    ref_transaction_commit(transaction, NULL, UPDATE_REFS_DIE_ON_ERR))
	die(_("%s: cannot update the ref"), ref.buf);

But then I would also hope that somebody (probably
ref_transaction_commit() now, but later maybe ref_transaction_update())
tells the user whether the problem was the inability to lock the
reference, or the consistency check of old_sha1, or whatever.

The next question is whether the generic error messages that the refs
code generates is good enough, or whether the caller would rather learn
about the reason for an error and generate its own, more specific error
message.  Please pay attention to this as you rewrite callers: by
relying more on centralized code, are we losing out unacceptably on
error message specificity?

Oh and by the way, given that you pass UPDATE_REFS_DIE_ON_ERR,
ref_transaction_commit() should never return a nonzero value, should it?

>  	if (force && !is_null_sha1(prev) && hashcmp(prev, object))
>  		printf(_("Updated tag '%s' (was %s)\n"), tag, find_unique_abbrev(prev, DEFAULT_ABBREV));
>  
> 

Michael

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