From: Siddharth Asthana <siddharthasthana31@gmail•com>
To: Patrick Steinhardt <ps@pks•im>
Cc: git@vger•kernel.org, Junio C Hamano <gitster@pobox•com>,
Christian Couder <christian.couder@gmail•com>,
Karthik Nayak <karthik.188@gmail•com>,
Justin Tobler <jltobler@gmail•com>,
Elijah Newren <newren@gmail•com>, Toon Claes <toon@iotcl•com>,
John Cai <johncai86@gmail•com>,
Johannes Schindelin <Johannes.Schindelin@gmx•de>
Subject: Re: [PATCH 1/2] replay: add --update-refs option
Date: Tue, 9 Sep 2025 12:28:29 +0530 [thread overview]
Message-ID: <c7615356-04cc-47e2-a894-4d24e416e4ad@gmail.com> (raw)
In-Reply-To: <aL6n8KEHSDii5Wd1@pks.im>
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[=<mode>]`,
> where `<mode>` 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
next prev parent reply other threads:[~2025-09-09 6:58 UTC|newest]
Thread overview: 125+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-09-08 4:36 [PATCH 0/2] replay: add --update-refs option Siddharth Asthana
2025-09-08 4:36 ` [PATCH 1/2] " Siddharth Asthana
2025-09-08 9:54 ` Patrick Steinhardt
2025-09-09 6:58 ` Siddharth Asthana [this message]
2025-09-09 9:00 ` Patrick Steinhardt
2025-09-09 7:32 ` Elijah Newren
2025-09-10 17:58 ` Siddharth Asthana
2025-09-08 4:36 ` [PATCH 2/2] replay: document --update-refs and --batch options Siddharth Asthana
2025-09-08 6:00 ` Christian Couder
2025-09-09 6:36 ` Siddharth Asthana
2025-09-09 7:26 ` Christian Couder
2025-09-10 20:26 ` Siddharth Asthana
2025-09-08 14:40 ` Kristoffer Haugsbakk
2025-09-09 7:06 ` Siddharth Asthana
2025-09-09 19:20 ` Andrei Rybak
2025-09-10 20:28 ` Siddharth Asthana
2025-09-08 6:07 ` [PATCH 0/2] replay: add --update-refs option Christian Couder
2025-09-09 6:36 ` Siddharth Asthana
2025-09-08 14:33 ` Kristoffer Haugsbakk
2025-09-09 7:04 ` Siddharth Asthana
2025-09-09 7:13 ` Elijah Newren
2025-09-09 7:47 ` Christian Couder
2025-09-09 9:19 ` Elijah Newren
2025-09-09 16:44 ` Junio C Hamano
2025-09-09 19:52 ` Elijah Newren
2025-09-26 23:08 ` [PATCH v2 0/1] replay: make atomic ref updates the default behavior Siddharth Asthana
2025-09-26 23:08 ` [PATCH v2 1/1] " Siddharth Asthana
2025-09-30 8:23 ` Christian Couder
2025-10-02 22:16 ` Siddharth Asthana
2025-10-03 7:30 ` Christian Couder
2025-10-02 22:55 ` Elijah Newren
2025-10-03 7:05 ` Christian Couder
2025-09-30 10:05 ` Phillip Wood
2025-10-02 10:00 ` Karthik Nayak
2025-10-02 22:20 ` Siddharth Asthana
2025-10-02 22:20 ` Siddharth Asthana
2025-10-08 14:01 ` Phillip Wood
2025-10-08 20:09 ` Siddharth Asthana
2025-10-08 20:59 ` Elijah Newren
2025-10-08 21:16 ` Siddharth Asthana
2025-10-09 9:40 ` Phillip Wood
2025-10-02 16:32 ` Elijah Newren
2025-10-02 18:27 ` Junio C Hamano
2025-10-02 23:42 ` Siddharth Asthana
2025-10-02 23:27 ` Siddharth Asthana
2025-10-03 7:59 ` Christian Couder
2025-10-08 19:59 ` Siddharth Asthana
2025-10-03 19:48 ` Elijah Newren
2025-10-03 20:32 ` Junio C Hamano
2025-10-08 20:06 ` Siddharth Asthana
2025-10-08 20:59 ` Junio C Hamano
2025-10-08 21:10 ` Siddharth Asthana
2025-10-08 21:30 ` Elijah Newren
2025-10-08 20:05 ` Siddharth Asthana
2025-10-02 17:14 ` [PATCH v2 0/1] " Kristoffer Haugsbakk
2025-10-02 23:36 ` Siddharth Asthana
2025-10-03 19:05 ` Kristoffer Haugsbakk
2025-10-08 20:02 ` Siddharth Asthana
2025-10-08 20:56 ` Elijah Newren
2025-10-08 21:16 ` Kristoffer Haugsbakk
2025-10-08 21:18 ` Siddharth Asthana
2025-10-13 18:33 ` [PATCH v3 0/3] replay: make atomic ref updates the default Siddharth Asthana
2025-10-13 18:33 ` [PATCH v3 1/3] replay: use die_for_incompatible_opt2() for option validation Siddharth Asthana
2025-10-13 18:33 ` [PATCH v3 2/3] replay: make atomic ref updates the default behavior Siddharth Asthana
2025-10-13 22:05 ` Junio C Hamano
2025-10-15 5:01 ` Siddharth Asthana
2025-10-13 18:33 ` [PATCH v3 3/3] replay: add replay.defaultAction config option Siddharth Asthana
2025-10-13 19:39 ` [PATCH v3 0/3] replay: make atomic ref updates the default Junio C Hamano
2025-10-15 4:57 ` Siddharth Asthana
2025-10-15 10:33 ` Christian Couder
2025-10-15 14:45 ` Junio C Hamano
2025-10-22 18:50 ` [PATCH v4 " Siddharth Asthana
2025-10-22 18:50 ` [PATCH v4 1/3] replay: use die_for_incompatible_opt2() for option validation Siddharth Asthana
2025-10-22 18:50 ` [PATCH v4 2/3] replay: make atomic ref updates the default behavior Siddharth Asthana
2025-10-22 21:19 ` Junio C Hamano
2025-10-28 19:03 ` Siddharth Asthana
2025-10-24 10:37 ` Christian Couder
2025-10-24 15:23 ` Junio C Hamano
2025-10-28 20:18 ` Siddharth Asthana
2025-10-28 19:39 ` Siddharth Asthana
2025-10-22 18:50 ` [PATCH v4 3/3] replay: add replay.refAction config option Siddharth Asthana
2025-10-24 11:01 ` Christian Couder
2025-10-24 15:30 ` Junio C Hamano
2025-10-28 20:08 ` Siddharth Asthana
2025-10-28 19:26 ` Siddharth Asthana
2025-10-24 13:28 ` Phillip Wood
2025-10-24 13:36 ` Phillip Wood
2025-10-28 19:47 ` Siddharth Asthana
2025-10-28 19:46 ` Siddharth Asthana
2025-10-23 18:47 ` [PATCH v4 0/3] replay: make atomic ref updates the default Junio C Hamano
2025-10-25 16:57 ` Junio C Hamano
2025-10-28 20:19 ` Siddharth Asthana
2025-10-24 9:39 ` Christian Couder
2025-10-28 21:46 ` [PATCH v5 " Siddharth Asthana
2025-10-28 21:46 ` [PATCH v5 1/3] replay: use die_for_incompatible_opt2() for option validation Siddharth Asthana
2025-10-28 21:46 ` [PATCH v5 2/3] replay: make atomic ref updates the default behavior Siddharth Asthana
2025-10-28 21:46 ` [PATCH v5 3/3] replay: add replay.refAction config option Siddharth Asthana
2025-10-29 16:19 ` Christian Couder
2025-10-29 17:00 ` Siddharth Asthana
2025-10-30 19:19 ` [PATCH v6 0/3] replay: make atomic ref updates the default Siddharth Asthana
2025-10-30 19:19 ` [PATCH v6 1/3] replay: use die_for_incompatible_opt2() for option validation Siddharth Asthana
2025-10-31 18:47 ` Elijah Newren
2025-11-05 18:39 ` Siddharth Asthana
2025-10-30 19:19 ` [PATCH v6 2/3] replay: make atomic ref updates the default behavior Siddharth Asthana
2025-10-31 18:49 ` Elijah Newren
2025-10-31 19:59 ` Junio C Hamano
2025-11-05 19:07 ` Siddharth Asthana
2025-11-03 16:25 ` Phillip Wood
2025-11-03 19:32 ` Siddharth Asthana
2025-11-04 16:15 ` Phillip Wood
2025-10-30 19:19 ` [PATCH v6 3/3] replay: add replay.refAction config option Siddharth Asthana
2025-10-31 7:08 ` Christian Couder
2025-11-05 19:03 ` Siddharth Asthana
2025-10-31 18:49 ` Elijah Newren
2025-11-05 19:10 ` Siddharth Asthana
2025-10-31 18:51 ` [PATCH v6 0/3] replay: make atomic ref updates the default Elijah Newren
2025-11-05 19:15 ` [PATCH v7 " Siddharth Asthana
2025-11-05 19:15 ` [PATCH v7 1/3] replay: use die_for_incompatible_opt2() for option validation Siddharth Asthana
2025-11-05 19:16 ` [PATCH v7 2/3] replay: make atomic ref updates the default behavior Siddharth Asthana
2025-11-05 19:16 ` [PATCH v7 3/3] replay: add replay.refAction config option Siddharth Asthana
2025-11-06 19:32 ` [PATCH v7 0/3] replay: make atomic ref updates the default Elijah Newren
2025-11-08 13:22 ` Siddharth Asthana
2025-11-08 17:11 ` Elijah Newren
2025-11-07 15:48 ` Phillip Wood
2025-11-08 13:23 ` Siddharth Asthana
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c7615356-04cc-47e2-a894-4d24e416e4ad@gmail.com \
--to=siddharthasthana31@gmail$(echo .)com \
--cc=Johannes.Schindelin@gmx$(echo .)de \
--cc=christian.couder@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=jltobler@gmail$(echo .)com \
--cc=johncai86@gmail$(echo .)com \
--cc=karthik.188@gmail$(echo .)com \
--cc=newren@gmail$(echo .)com \
--cc=ps@pks$(echo .)im \
--cc=toon@iotcl$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox