From: Siddharth Asthana Date: Wed, 15 Oct 2025 05:01:35 GMT Subject: Re: [PATCH v3 2/3] replay: make atomic ref updates the default behavior Message-ID: <1065906e-01d2-4b1d-9b43-ce53c5ea9d1b@gmail.com> In-Reply-To: On 14/10/25 03:35, Junio C Hamano wrote: > Siddharth Asthana writes: > >> For users needing the traditional pipeline workflow, add a new >> `--update-refs=` 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 :: >> 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 while the > is checked out (hence the branch advances as the result of acquiring > these commits from )? Let me see if I understood you by > attempting to rephrase. > > The history is replayed on top of the and 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 and 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 ", i.e. merely specifying the > starting point without 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. >