Re: [PATCH 1/2] replay: add --update-refs option
- From
Elijah Newren <newren@gmail.com>
- Date
- Sep 9, 2025, 07:32 UTC
- Message-ID
- <CABPp-BEmOor3CLAY6y50DuGR1K7WYu+PVsXXWOOaofXzJpavMg@mail.gmail.com>
- In-Reply-To
- <20250908043620.57848-2-siddharthasthana31@gmail.com>
On Sun, Sep 7, 2025 at 9:36 PM Siddharth Asthana <siddharthasthana31@gmail.com> wrote:
>
[...]
> 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.
[...]
> + 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?
Show 6 quoted lines
> + 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?
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.
Show 5 quoted lines
> @@ -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?
Show 25 quoted lines
> - 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?