From: Siddharth Asthana <siddharthasthana31@gmail•com>
To: Elijah Newren <newren@gmail•com>
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>,
Patrick Steinhardt <ps@pks•im>, 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: Wed, 10 Sep 2025 23:28:16 +0530 [thread overview]
Message-ID: <aa4d5d0d-3c13-4381-8194-2b0c178b3cca@gmail.com> (raw)
In-Reply-To: <CABPp-BEmOor3CLAY6y50DuGR1K7WYu+PVsXXWOOaofXzJpavMg@mail.gmail.com>
On 09/09/25 13:02, Elijah Newren wrote:
> On Sun, Sep 7, 2025 at 9:36 PM Siddharth Asthana
> <siddharthasthana31@gmail•com> wrote:
> [...]
Hi Elijah,
>> Option validation ensures --update-refs cannot be used with the existing
>> --update option, and --batch can only be used with --update-refs.
> There is no existing --update option.
You're right, poor wording in my commit message. Both options are new in
this series.
>
> [...]
>> + int update_directly = 0;
>> + int update_refs_flag = 0;
>> + int batch_mode = 0;
> Why are we adding three kinds of updates? You covered two in the
> commit message, but mostly only motivated one, and then added three?
That's fair criticism. I was trying to cover all bases but ended up with
confusing options. The three were:
- --update: individual ref updates (like piping to update-ref --stdin)
- --update-refs: atomic transactions
- --batch: allow partial failures with --update-refs
But as Patrick pointed out, individual updates are inefficient and
everyone seems to prefer simpler options.
>
>> + 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")),
> Three modes and I can't figure out how update_directly differs from
> the others from the description. Is it different?
Yes, update_directly calls refs_update_ref() for each ref individually,
while update_refs_flag uses ref transactions. But Patrick's performance
concerns make me think we should drop the individual approach entirely.
>
> Also, --batch seems like a funny name since update-refs is also
> updating refs in a batch. I'd suggest coming up with a new name...but
> is there clamor for it? You mostly motivated the atomic updates, and
> I think it might be better to just implement those and then add more
> flags later if needed.
>
>> @@ -399,6 +461,7 @@ int cmd_replay(int argc,
>>
>> init_basic_merge_options(&merge_opt, repo);
>> memset(&result, 0, sizeof(result));
>> + result.clean = 1; /* Assume clean until proven otherwise */
> I don't understand why this change is needed or helpful. I don't
> think it changes behavior looking at the existing code, but to me,
> result is supposed to be the result of a merge operation, not an
> input, and should not be set other than being cleared initially by the
> caller. The comment feels slightly misleading to me, as well. So,
> I'm surprised by this change and would like to hear the motivation
> behind it; could you clarify? Did I miss something about how you
> depend on this being set even if the list of commits to replay is
> empty or something?
You caught an error in my logic. I was trying to handle the case where
no commits are replayed (empty range), but you are right - result should
only be set by merge operations. The existing code already handles empty
ranges correctly by never entering the replay loop. I will remove this
line in v2.
>
>> - printf("update %s %s %s\n",
>> - decoration->name,
>> - oid_to_hex(&last_commit->object.oid),
>> - oid_to_hex(&commit->object.oid));
>> + if (update_directly) {
>> + if (update_ref_direct(repo, decoration->name,
>> + &last_commit->object.oid,
>> + &commit->object.oid) < 0) {
>> + ret = -1;
>> + goto cleanup;
>> + }
>> + } else if (transaction) {
>> + if (add_ref_to_transaction(transaction, decoration->name,
>> + &last_commit->object.oid,
>> + &commit->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",
>> + decoration->name,
>> + oid_to_hex(&last_commit->object.oid),
>> + oid_to_hex(&commit->object.oid));
>> + }
> Who would want the update_ref_direct() branch of code here? Can we
> just toss it?
Given the performance concerns and the confusion it is causing, yes.
Let's toss it and focus on the transaction-based approach.
Based on all the feedback, I am thinking of simplifying to:
- Default: update refs atomically using transactions
- --output-commands: print update commands (for the traditional pipeline
workflow)
- --allow-partial: allow some ref updates to succeed while others fail
This addresses your point about making the better behavior default while
still supporting existing workflows.
Thanks,
Siddharth
next prev parent reply other threads:[~2025-09-10 17: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
2025-09-09 9:00 ` Patrick Steinhardt
2025-09-09 7:32 ` Elijah Newren
2025-09-10 17:58 ` Siddharth Asthana [this message]
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=aa4d5d0d-3c13-4381-8194-2b0c178b3cca@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