From: Siddharth Asthana <siddharthasthana31@gmail.com>
Date: Wed, 10 Sep 2025 17:58:16 GMT
Subject: Re: [PATCH 1/2] replay: add --update-refs option
Message-ID: <aa4d5d0d-3c13-4381-8194-2b0c178b3cca@gmail.com>
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