Re: [PATCH 1/2] replay: add --update-refs option
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Sep 8, 2025, 09:54 UTC
- Message-ID
- <aL6n8KEHSDii5Wd1@pks.im>
- In-Reply-To
- <20250908043620.57848-2-siddharthasthana31@gmail.com>
On Mon, Sep 08, 2025 at 10:06:19AM +0530, Siddharth Asthana wrote:
Show 16 quoted lines
> 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.
Show 13 quoted lines
> @@ -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".
Show 15 quoted lines
> @@ -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.
Show 5 quoted lines
> @@ -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.
Show 32 quoted lines
> @@ -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.
Show 16 quoted lines
> + 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