Re: [PATCH 1/2] replay: add --update-refs option

[Patrick Steinhardt <ps@pks.im>]

On Mon, Sep 08, 2025 at 10:06:19AM +0530, Siddharth Asthana wrote:
> diff --git a/builtin/replay.c b/builtin/replay.c
> index 6172c8aacc..a33c9887cf 100644
> --- a/builtin/replay.c
> +++ b/builtin/replay.c
> @@ -284,6 +284,37 @@ static struct commit *pick_regular_commit(struct repository *repo,
>  	return create_commit(repo, result->tree, pickme, replayed_base);
>  }
>  
> +static int update_ref_direct(struct repository *repo, const char *refname,
> +			     const struct object_id *new_oid,
> +			     const struct object_id *old_oid)
> +{
> +	const char *msg = "replay";
> +	return refs_update_ref(get_main_ref_store(repo), msg, refname,
> +			       new_oid, old_oid, 0, UPDATE_REFS_MSG_ON_ERR);
> +}

Is there a strong reason why a user would want to update refs one by
one? If not, let's not add new code to our base that does so. This is
known to be inperformant for the reftable backend, but also for the
files backend in some cases. If we really want to support the case where
only a subset of references gets committed we should be using batched
updates with the `REF_TRANSACTION_ALLOW_FAILURE` flag.

> @@ -319,6 +355,12 @@ int cmd_replay(int argc,
>  			   N_("replay onto given commit")),
>  		OPT_BOOL(0, "contained", &contained,
>  			 N_("advance all branches contained in revision-range")),
> +		OPT_BOOL(0, "update", &update_directly,
> +			 N_("update branches directly instead of outputting update commands")),
> +		OPT_BOOL(0, "update-refs", &update_refs_flag,
> +			 N_("update branches using ref transactions")),
> +		OPT_BOOL(0, "batch", &batch_mode,
> +			 N_("allow partial ref updates in batch mode")),
>  		OPT_END()
>  	};
>  

So I think we should reduce this to only accept two flags:
`--update-refs` and a flag that accepts a subset of refs failing.o

We might also want to make this something like `--update-refs[=<mode>]`,
where `<mode>` could be "allow-failures".

> @@ -333,6 +375,14 @@ int cmd_replay(int argc,
>  	if (advance_name_opt && contained)
>  		die(_("options '%s' and '%s' cannot be used together"),
>  		    "--advance", "--contained");
> +
> +	if (update_directly && update_refs_flag)
> +		die(_("options '%s' and '%s' cannot be used together"),
> +		    "--update", "--update-refs");
> +
> +	if (batch_mode && !update_refs_flag)
> +		die(_("option '%s' can only be used with '%s'"),
> +		    "--batch", "--update-refs");
>  	advance_name = xstrdup_or_null(advance_name_opt);
>  
>  	repo_init_revisions(repo, &revs, prefix);

We have the `die_for_incompatible_opt*()` helpers for this.

> @@ -389,6 +439,18 @@ int cmd_replay(int argc,
>  	determine_replay_mode(repo, &revs.cmdline, onto_name, &advance_name,
>  			      &onto, &update_refs);
>  
> +	/* Initialize ref transaction if using --update-refs */

Nit: the comment doesn't really add much context, so I'd just drop it.
It's generally discouraged to add a comment that re-states what the code
already says. Instead, comments should point out things that are easy to
miss or not obvious at all.

> @@ -445,10 +525,43 @@ int cmd_replay(int argc,
>  
>  	/* In --advance mode, advance the target ref */
>  	if (result.clean == 1 && advance_name) {
> -		printf("update %s %s %s\n",
> -		       advance_name,
> -		       oid_to_hex(&last_commit->object.oid),
> -		       oid_to_hex(&onto->object.oid));
> +		if (update_directly) {
> +			if (update_ref_direct(repo, advance_name,
> +					     &last_commit->object.oid,
> +					     &onto->object.oid) < 0) {
> +				ret = -1;
> +				goto cleanup;
> +			}
> +		} else if (transaction) {
> +			if (add_ref_to_transaction(transaction, advance_name,
> +						   &last_commit->object.oid,
> +						   &onto->object.oid,
> +						   &transaction_err) < 0) {
> +				ret = error(_("failed to add ref update to transaction: %s"), transaction_err.buf);
> +				goto cleanup;
> +			}
> +		} else {
> +			printf("update %s %s %s\n",
> +			       advance_name,
> +			       oid_to_hex(&last_commit->object.oid),
> +			       oid_to_hex(&onto->object.oid));
> +		}
> +	}
> +
> +	/* Commit the ref transaction if we have one */

Likewise here.

> +	if (transaction && result.clean == 1) {
> +		if (ref_transaction_commit(transaction, &transaction_err)) {
> +			if (batch_mode) {
> +				/* Print failed updates in batch mode */
> +				warning(_("some ref updates failed: %s"), transaction_err.buf);
> +				ref_transaction_for_each_rejected_update(transaction,
> +										 print_rejected_update, NULL);
> +			} else {
> +				/* In atomic mode, all updates failed */
> +				ret = error(_("failed to update refs: %s"), transaction_err.buf);
> +				goto cleanup;
> +			}
> +		}
>  	}
>  
>  	merge_finalize(&merge_opt, &result);

Patrick