From: Siddharth Asthana <siddharthasthana31@gmail•com>
To: Junio C Hamano <gitster@pobox•com>
Cc: git@vger•kernel.org, christian.couder@gmail•com,
phillip.wood123@gmail•com, phillip.wood@dunelm•org.uk,
newren@gmail•com, ps@pks•im, karthik.188@gmail•com,
code@khaugsbakk•name, rybak.a.v@gmail•com, jltobler@gmail•com,
toon@iotcl•com, johncai86@gmail•com, johannes.schindelin@gmx•de
Subject: Re: [PATCH v3 2/3] replay: make atomic ref updates the default behavior
Date: Wed, 15 Oct 2025 10:31:35 +0530 [thread overview]
Message-ID: <1065906e-01d2-4b1d-9b43-ce53c5ea9d1b@gmail.com> (raw)
In-Reply-To: <xmqqms5uzcd7.fsf@gitster.g>
On 14/10/25 03:35, Junio C Hamano wrote:
> Siddharth Asthana <siddharthasthana31@gmail•com> writes:
>
>> For users needing the traditional pipeline workflow, add a new
>> `--update-refs=<mode>` option that preserves the original behavior:
>>
>> git replay --update-refs=print --onto main topic1..topic2 | git update-ref --stdin
>>
>> The mode can be:
>> * `yes` (default): Update refs directly using an atomic transaction
>> * `print`: Output update-ref commands for pipeline use
> Is it only me who still finds this awkward? A question "update?"
> that gets answered "yes" is quite understandable, but it is not
> immediately obvious what it means to answer "print" to the same
> question. When the user gives the latter mode as the answer to the
> question, the question being answered is not really "do you want to
> update refs?" at all.
>
> The question the command wants the user to answer is more like "what
> action do you want to see performed on the refs?", isn't it? The
> user would answer to the question with "please update them" to get
> the default mode, while "please print them" may be the answer the
> user would give to get the useful-for-dry-run-and-development mode.
>
> Perhaps phrase it more like "--ref-action=(update|print)"? I dunno.
That's a really good point. I was thinking of it as "update refs?
yes/no" where
"print" meant "don't update", but you're right that it's actually asking a
different question entirely. The real question is "what should we do
with the
refs?" and the answer is either "update them" or "print the commands".
`--ref-action=(update|print)` is much clearer because:
- It explicitly asks "what action?"
- Both values are verbs that answer that question consistently
- It's immediately obvious what each mode does
- It aligns with the config name discussion in the cover letter thread
I will switch to `--ref-action` in the next version. This also means the
config
would naturally be `replay.refAction`, which makes the relationship obvious.
>
>> --advance <branch>::
>> Starting point at which to create the new commits; must be a
>> branch name.
>> +
>> -When `--advance` is specified, the update-ref command(s) in the output
>> -will update the branch passed as an argument to `--advance` to point at
>> -the new commits (in other words, this mimics a cherry-pick operation).
>> +When `--advance` is specified, the branch passed as an argument will be
>> +updated to point at the new commits (or an update command will be printed
>> +if `--update-refs=print` is used). This mimics a cherry-pick operation.
> I do not find it clear what the reference to cherry-pick is trying
> to convey. It is like cherry-picking <something> while the <branch>
> is checked out (hence the branch advances as the result of acquiring
> these commits from <something>)? Let me see if I understood you by
> attempting to rephrase.
>
> The history is replayed on top of the <branch> and <branch> is
> updated to point at the tip of resulting history.
Your phrasing is much better. The cherry-pick comparison was trying to
contrast
with `--onto` (which doesn't move the target branch), but it ended up
being more
confusing than helpful. I will use your wording:
The history is replayed on top of the <branch> and <branch> is
updated to point at the tip of the resulting history. This is different
from `--onto`, which uses the target only as a starting point without
updating it.
>
> But what's the significance of saying so? Did you want to contrast
> it with "rebase --onto <branch>", i.e. merely specifying the
> starting point without <branch> itself moving as the result? If so,
> it is probably a notable distinction worth pointing out, but just
> saying "mimics a cherry-pick operation" alone is probably not enough
> to get the intended audience understand what you wanted to tell
> them.
>
> Side note. I casually wrote "is updated to point" but with the
> option not to update (but show the way to update refs), we'd
> probably need to find a good phrase to express "where the
> command _wants_ to see the refs pointing at as the result",
> without referring to who/how the refs are made to point at these
> points.
>
>> -To simply rebase `mybranch` onto `target`:
>> +To simply rebase `mybranch` onto `target` (default behavior):
> "the default"?
Good catch - I was trying to emphasize that the atomic update behavior
is now
default, but in the context of showing example commands, "default behavior"
doesn't add clarity. I'll just say "To simply rebase `mybranch` onto
`target`:"
>
>> diff --git a/builtin/replay.c b/builtin/replay.c
>> index b64fc72063..457225363e 100644
>> --- a/builtin/replay.c
>> +++ b/builtin/replay.c
>> @@ -284,6 +284,26 @@ static struct commit *pick_regular_commit(struct repository *repo,
>> return create_commit(repo, result->tree, pickme, replayed_base);
>> }
>>
>> +static int handle_ref_update(const char *mode,
>> + struct ref_transaction *transaction,
>> + const char *refname,
>> + const struct object_id *new_oid,
>> + const struct object_id *old_oid,
>> + struct strbuf *err)
>> +{
>> + if (!strcmp(mode, "print")) {
>> + printf("update %s %s %s\n",
>> + refname,
>> + oid_to_hex(new_oid),
>> + oid_to_hex(old_oid));
>> + return 0;
>> + }
>> +
>> + /* mode == "yes" - update refs directly */
>> + return ref_transaction_update(transaction, refname, new_oid, old_oid,
>> + NULL, NULL, 0, "git replay", err);
>> +}
> Hmph, would it be easier to follow if the above is symmetric, i.e.,
>
> if (...) {
> what happens in the "print" mode
> } else {
> what happens in the "update ourselves" mode
> }
>
> I wonder?
>
> In any case, do not pass mode as "const char *" around in the call
> chain. Instead, reduce it down to an enum or integer (with CPP
> macro) at the earliest possible place after you saw the command line
> option. That would allow you to even do
>
> switch (ref_action) {
> case PRINT_INSN:
> printf("update ...");
> return 0;
> case UPDATE_OURSELVES:
> return ref_transaction_update(...);
> default:
> BUG("Bad ref_action %d", ref_action);
> }
>
> to future-proof for the third option.
Perfect, I will convert to an enum right after parse_options(). This
approach is
much cleaner and prevents typos like "prnit" that the compiler can't catch.
Something like:
enum ref_action_mode {
REF_ACTION_UPDATE,
REF_ACTION_PRINT
};
Then parse it early:
if (!strcmp(ref_action_str, "update"))
ref_action = REF_ACTION_UPDATE;
else if (!strcmp(ref_action_str, "print"))
ref_action = REF_ACTION_PRINT;
else
die(_("unknown --ref-action mode '%s'"), ref_action_str);
And use the switch statement in handle_ref_update(). This also makes it
trivial
to add new modes in the future without string comparison overhead throughout
the code.
Thanks for the detailed review!
Siddharth
>
>> + OPT_STRING(0, "update-refs", &update_refs_mode,
>> + N_("mode"),
>> + N_("control ref update behavior (yes|print)")),
>> OPT_END()
>> };
> This one is fine, but then immediately after parse_options()
> returns, do something like
>
> if (!strcmp(update_refs_mode, "print"))
> ref_action = PRINT_INSN;
> else if (!strcmp(update_refs_mode, "yes"))
> ref_action = UPDATE_OURSELVES;
> else
> die(_("unknown option --update-ref='%s'"),
> update_refs_mode);
>
> so that you do not have to keep strcmp() with "print", which risks
> you to mistype "prnit" and no compiler would protect against that.
>
next prev parent reply other threads:[~2025-10-15 5:01 UTC|newest]
Thread overview: 126+ 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
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 [this message]
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
-- strict thread matches above, loose matches on Subject: below --
2025-10-13 18:25 [PATCH v3 " Siddharth Asthana
2025-10-13 18:25 ` [PATCH v3 2/3] replay: make atomic ref updates the default behavior 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=1065906e-01d2-4b1d-9b43-ce53c5ea9d1b@gmail.com \
--to=siddharthasthana31@gmail$(echo .)com \
--cc=christian.couder@gmail$(echo .)com \
--cc=code@khaugsbakk$(echo .)name \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=jltobler@gmail$(echo .)com \
--cc=johannes.schindelin@gmx$(echo .)de \
--cc=johncai86@gmail$(echo .)com \
--cc=karthik.188@gmail$(echo .)com \
--cc=newren@gmail$(echo .)com \
--cc=phillip.wood123@gmail$(echo .)com \
--cc=phillip.wood@dunelm$(echo .)org.uk \
--cc=ps@pks$(echo .)im \
--cc=rybak.a.v@gmail$(echo .)com \
--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