From: Siddharth Asthana Date: Tue, 09 Sep 2025 06:58:29 GMT Subject: Re: [PATCH 1/2] replay: add --update-refs option Message-ID: In-Reply-To: On 08/09/25 15:24, Patrick Steinhardt wrote: > 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); >> +} Hi Patrick, Thanks for the detailed review > 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. You are absolutely right about the performance concern. My thinking was to provide a simple mode that exactly mimics "git replay | git update-ref --stdin" behavior, but I see that's not worth the performance cost. I will remove the individual update function and only use batched transactions with REF_TRANSACTION_ALLOW_FAILURE when needed. > 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[=]`, > where `` could be "allow-failures". That make sense. Would you prefer `--update-refs` with `--allow-failures` as a separate flag? I am leaning toward that since it's clearer than the parameter syntax. > >> @@ -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. Thanks, I will use those. > >> @@ -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. Will remove the redundant comments. > >> @@ -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. Will remove the redundant comments here too. Thank, Siddharth > >> + 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