Re: [PATCH v8 26/44] fast-import.c: use a ref transaction when dumping tags

[Jonathan Nieder <jrnieder@gmail.com>]

Ronnie Sahlberg wrote:

> [Subject: fast-import.c: use a ref transaction when dumping tags]

This seems like an odd thing to do: either it would make sense to have
a single transaction for all imported refs so all fail or succeed
together, or there would be separate transactions for each ref.

That said, I don't mind, particularly if it's a step on the way to
using a single transaction for everything being dumped.

[...]
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -1736,15 +1736,22 @@ static void dump_tags(void)
>  {
>  	static const char *msg = "fast-import";
>  	struct tag *t;
> -	struct ref_lock *lock;
>  	char ref_name[PATH_MAX];
> +	struct strbuf err = STRBUF_INIT;
> +	struct ref_transaction *transaction;
>  
> +	transaction = ref_transaction_begin();
>  	for (t = first_tag; t; t = t->next_tag) {
> -		sprintf(ref_name, "tags/%s", t->name);
> +		sprintf(ref_name, "refs/tags/%s", t->name);

Can this overflow the buffer?

> -		lock = lock_ref_sha1(ref_name, NULL);
> -		if (!lock || write_ref_sha1(lock, t->sha1, msg) < 0)
> -			failure |= error("Unable to update %s", ref_name);
> +
> +		if (ref_transaction_update(transaction, ref_name, t->sha1,
> +					   NULL, 0, 0, &err)) {
> +			failure |= 1;
> +			break;
> +		}
>  	}
> +	if (failure || ref_transaction_commit(transaction, msg, &err))
> +		failure |= error("Unable to update %s", err.buf);

This 'if (failure || ...) failure |=' idiom seems strange.
Is err.buf always set if failure is nonzero?

Elsewhere failure is 0 or -1 but this introduces the possibility for
it to be temporarily 1.

dump_branches could have caused failure to be -1 before dump_tags
is called.  I think the intent is

		if (ref_transaction_update(...)) {
			failure |= error(...);
			goto out;
		}
	}
	if (ref_transaction_commit(...))
		failure |= error(...);
 out:
	ref_transaction_free(transaction);
	...

Thanks,
Jonathan