Re: [PATCH 5/8] update-ref: handle rejections while adding updates

[Patrick Steinhardt <ps@pks.im>]

On Mon, Apr 20, 2026 at 12:12:03PM +0200, Karthik Nayak wrote:
> When using git-update-ref(1) with the '--batch-updates' flag, updates
> rejected by the reference backend are displayed to the user while other
> updates are applied. This only applies during the committing of the

Nit: s/committing/commit phase/

> transaction.
> 
> In the following commits, we'll also extend `ref_transaction_update()`
> to reject updates before even a transaction is prepared/committed. In

The "even" reads like it's at the wrong position.

> diff --git a/builtin/update-ref.c b/builtin/update-ref.c
> index 5259cc7226..d1980c60c4 100644
> --- a/builtin/update-ref.c
> +++ b/builtin/update-ref.c
> @@ -268,11 +277,13 @@ static void print_rejected_refs(const char *refname,
>   */
>  
>  static void parse_cmd_update(struct ref_transaction *transaction,
> -			     const char *next, const char *end)
> +			     const char *next, const char *end,
> +			     struct command_options *opts)

Here you follow my earlier suggestion of using `_options` for the struct
name, but `opts` for the variable :)

> @@ -289,11 +300,18 @@ static void parse_cmd_update(struct ref_transaction *transaction,
>  	if (*next != line_termination)
>  		die("update %s: extra input: %s", refname, next);
>  
> -	if (ref_transaction_update(transaction, refname,
> -				   &new_oid, have_old ? &old_oid : NULL,
> -				   NULL, NULL,
> -				   update_flags | create_reflog_flag,
> -				   msg, &err))
> +	tx_err = ref_transaction_update(transaction, refname,
> +					&new_oid, have_old ? &old_oid : NULL,
> +					NULL, NULL,
> +					update_flags | create_reflog_flag,
> +					msg, &err);
> +
> +	if (tx_err && tx_err != REF_TRANSACTION_ERROR_GENERIC &&
> +	    opts->allow_update_failures)
> +		print_rejected_refs(refname, have_old ? &old_oid : NULL,
> +				    &new_oid, NULL, NULL, tx_err, err.buf,
> +				    NULL);
> +	else if (tx_err)
>  		die("%s", err.buf);
>  
>  	update_flags = default_flags;

I think this could use some curly braces to become easier to read, even
if it's only single-line statements.

Does this change have an impact on the ordering the user sees for
printed errors? If so, it might make sense to point that out in the
commit message.

> @@ -341,12 +361,19 @@ static void parse_cmd_symref_update(struct ref_transaction *transaction,
>  	if (*next != line_termination)
>  		die("symref-update %s: extra input: %s", refname, next);
>  
> -	if (ref_transaction_update(transaction, refname, NULL,
> -				   have_old_oid ? &old_oid : NULL,
> -				   new_target,
> -				   have_old_oid ? NULL : old_target,
> -				   update_flags | create_reflog_flag,
> -				   msg, &err))
> +	tx_err = ref_transaction_update(transaction, refname, NULL,
> +					have_old_oid ? &old_oid : NULL,
> +					new_target,
> +					have_old_oid ? NULL : old_target,
> +					update_flags | create_reflog_flag,
> +					msg, &err);
> +
> +	if (tx_err && tx_err != REF_TRANSACTION_ERROR_GENERIC &&
> +	    opts->allow_update_failures)
> +		print_rejected_refs(refname, have_old_oid ? &old_oid : NULL,
> +				    NULL, have_old_oid ? NULL : old_target,
> +				    new_target, tx_err, err.buf, NULL);
> +	else if (tx_err)
>  		die("%s", err.buf);
>  
>  	update_flags = default_flags;

Same here, curly braces might help readability.

> @@ -644,6 +680,10 @@ static void update_refs_stdin(unsigned int flags)
>  	struct ref_transaction *transaction;
>  	int i, j;
>  
> +	struct command_options opts = {
> +		.allow_update_failures = flags & REF_TRANSACTION_ALLOW_FAILURE,
> +	};
> +
>  	transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
>  						  flags, &err);
>  	if (!transaction)

Suggestion, please feel free to ignore: we could also pass the flags
directly instead.

Patrick