Re: [PATCH v7 0/3] replay: make atomic ref updates the default

> On Wed, Nov 5, 2025 at 11:17 AM Siddharth Asthana
> <siddharthasthana31@gmail.com> wrote:
>> This is v7 of the git-replay atomic updates series.
>>
>> This version addresses all feedback from v6 reviews. Thanks to Elijah,
>> Christian, and Phillip for the thorough reviews that helped refine the
>> implementation to Git standards.
>>
>> ## Changes in v7
>>
>> **Improved commit message clarity**
>>
>> Per Elijah's feedback, simplified commit messages by removing redundant
>> sections:
>>    - Removed "Implementation details" section (details visible in diff)
>>    - Shortened "Test suite changes" to focus on what's tested
>>    - Removed command-line precedence paragraph (obvious from code)
>>    - Removed "Examples" and configuration precedence sections
>>
>> **Fixed test cleanup and isolation**
>>
>> Following Elijah's suggestions:
>>    - Used test_when_finished with proper state restoration in atomic tests
>>    - Created separate test-atomic branch to avoid contaminating topic2
>>    - Fixed bare repository test to use START variable for cleanup
>>    - Improved test reliability by rebuilding expectations independently
>>
>> **Extracted parse_ref_action_mode() to appropriate commit**
>>
>> Per Christian's observation, moved the parse_ref_action_mode() helper
>> function from Commit 3 to Commit 2 where it's first used. This makes
>> the patch progression more logical.
>>
>> **Fixed parameter naming consistency**
>>
>> Following Christian's feedback, used consistent naming throughout:
>>    - ref_action (string parameter for command-line/config value)
>>    - ref_mode (enum variable for internal mode)
>> This eliminates confusion and improves code readability.
>>
>> **Moved config reference to correct commit**
>>
>> Per Elijah's note, moved the sentence about replay.refAction config
>> from Commit 2's documentation to Commit 3 where the config is actually
>> introduced.
>>
>> **Enhanced reflog messages**
>>
>> Following Phillip's suggestions for better user experience:
>>    - --advance mode: "replay --advance <branch-name>" (uses user input)
>>    - --onto mode: "replay --onto <commit-sha>" (precise commit reference)
>> Added comprehensive reflog testing to verify messages.
>>
>> **Fixed indentation in Commit 3**
>>
>> Corrected indentation within the while (decoration) loop per CI
>> feedback, adding proper tabs to nested if statements.
>>
>> **Fixed coding style**
>>
>> Per CI check-style feedback, removed braces from single-statement
>> if-else blocks following Git's CodingGuidelines.
>>
>> **Split config tests for clarity**
>>
>> Separated the replay.refAction config test into two distinct tests:
>>    - replay.refAction=print config option
>>    - replay.refAction=update config option
>> This improves test clarity and makes failures easier to diagnose.
>>
>> ## Technical Implementation
>>
>> The atomic ref updates leverage Git's ref transaction API:
>>    - ref_store_transaction_begin() with default atomic behavior
>>    - ref_transaction_update() to stage each update
>>    - ref_transaction_commit() for atomic application
>>
>> The helper functions provide clean separation:
>>    - parse_ref_action_mode(): Validates strings and converts to enum
>>    - get_ref_action_mode(): Implements command-line > config > default precedence
>>    - handle_ref_update(): Uses type-safe enum with switch statement
>>
>> Reflog messages are constructed dynamically based on replay mode and
>> include either the branch name (--advance) or commit SHA (--onto) for
>> clear audit trails.
>>
>> ## Testing
>>
>> All tests pass:
>>    - t3650-replay-basics.sh (22 tests pass)
>>    - Config tests verify proper precedence and error handling
>>    - Atomic behavior tests verify direct ref updates
>>    - Reflog tests verify descriptive messages
>>    - Backward compatibility maintained for pipeline workflow
>>
>> CI results: https://gitlab.com/gitlab-org/git/-/pipelines/2140425748
>>
>> Siddharth Asthana (3):
>>    replay: use die_for_incompatible_opt2() for option validation
>>    replay: make atomic ref updates the default behavior
>>    replay: add replay.refAction config option
>>
>>   Documentation/config/replay.adoc |  11 +++
>>   Documentation/git-replay.adoc    |  63 ++++++++++-----
>>   builtin/replay.c                 | 133 ++++++++++++++++++++++++++++---
>>   t/t3650-replay-basics.sh         | 113 ++++++++++++++++++++++++--
>>   4 files changed, 277 insertions(+), 43 deletions(-)
>>   create mode 100644 Documentation/config/replay.adoc
>>
>> Range-diff against v6:
>> 1:  1f0fad0cac = 1:  9e4eab2df2 replay: use die_for_incompatible_opt2() for option validation
>> 2:  bfc6188234 ! 2:  1602f6097e replay: make atomic ref updates the default behavior
>>      @@ Commit message
>>            * update (default): Update refs directly using an atomic transaction
>>            * print: Output update-ref commands for pipeline use
>>
>>      -    Implementation details:
>>      -
>>      -    The atomic ref updates are implemented using Git's ref transaction API.
>>      -    In cmd_replay(), when not in `print` mode, we initialize a transaction
>>      -    using ref_store_transaction_begin() with the default atomic behavior.
>>      -    As commits are replayed, ref updates are staged into the transaction
>>      -    using ref_transaction_update(). Finally, ref_transaction_commit()
>>      -    applies all updates atomically—either all updates succeed or none do.
>>      -
>>      -    To avoid code duplication between the 'print' and 'update' modes, this
>>      -    commit extracts a handle_ref_update() helper function. This function
>>      -    takes the mode (as an enum) and either prints the update command or
>>      -    stages it into the transaction. Using an enum rather than passing the
>>      -    string around provides type safety and allows the compiler to catch
>>      -    typos. The switch statement makes it easy to add future modes.
>>      -
>>      -    The helper function signature:
>>      -
>>      -      static int handle_ref_update(enum ref_action_mode mode,
>>      -                                    struct ref_transaction *transaction,
>>      -                                    const char *refname,
>>      -                                    const struct object_id *new_oid,
>>      -                                    const struct object_id *old_oid,
>>      -                                    struct strbuf *err)
>>      -
>>      -    The enum is defined as:
>>      -
>>      -      enum ref_action_mode {
>>      -          REF_ACTION_UPDATE,
>>      -          REF_ACTION_PRINT
>>      -      };
>>      -
>>      -    The mode string is converted to enum immediately after parse_options()
>>      -    to avoid string comparisons throughout the codebase and provide compiler
>>      -    protection against typos.
>>      -
>>           Test suite changes:
>>
>>           All existing tests that expected command output now use
>>      @@ Commit message
>>            - Equivalence between traditional pipeline and atomic updates
>>            - Real atomicity using a lock file to verify all-or-nothing guarantee
>>            - Test isolation using test_when_finished to clean up state
>>      -
>>      -    The bare repository tests were fixed to rebuild their expectations
>>      -    independently rather than comparing to previous test output, improving
>>      -    test reliability and isolation.
>>      +      - Reflog messages include replay mode and target
>>
>>           A following commit will add a replay.refAction configuration
>>           option for users who prefer the traditional pipeline output as their
>>      @@ Documentation/git-replay.adoc: OPTIONS
>>       -commits, similar to the way how `git rebase --update-refs` updates
>>       -multiple branches in the affected range.
>>       +When `--onto` is specified, the branch(es) in the revision range will be
>>      -+updated to point at the new commits (or update commands will be printed
>>      -+if `--ref-action=print` is used), similar to the way `git rebase --update-refs`
>>      ++updated to point at the new commits, similar to the way `git rebase --update-refs`
>>       +updates multiple branches in the affected range.
>>
>>        --advance <branch>::
>>      @@ Documentation/git-replay.adoc: OPTIONS
>>       -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).
>>       +The history is replayed on top of the <branch> and <branch> is updated to
>>      -+point at the tip of the resulting history (or an update command will be
>>      -+printed if `--ref-action=print` is used). This is different from `--onto`,
>>      ++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.
>>       +
>>       +--ref-action[=<mode>]::
>>      @@ Documentation/git-replay.adoc: OPTIONS
>>       +  * `print`: Output update-ref commands for pipeline use. This is the
>>       +    traditional behavior where output can be piped to `git update-ref --stdin`.
>>       +--
>>      -++
>>      -+The default mode can be configured via the `replay.refAction` configuration variable.
>>
>>        <revision-range>::
>>          Range of commits to replay. More than one <revision-range> can
>>      @@ builtin/replay.c: static struct commit *pick_regular_commit(struct repository *r
>>          return create_commit(repo, result->tree, pickme, replayed_base);
>>        }
>>
>>      ++static enum ref_action_mode parse_ref_action_mode(const char *ref_action, const char *source)
>>      ++{
>>      ++  if (!ref_action || !strcmp(ref_action, "update"))
>>      ++          return REF_ACTION_UPDATE;
>>      ++  if (!strcmp(ref_action, "print"))
>>      ++          return REF_ACTION_PRINT;
>>      ++  die(_("invalid %s value: '%s'"), source, ref_action);
>>      ++}
>>      ++
>>       +static int handle_ref_update(enum ref_action_mode mode,
>>       +                       struct ref_transaction *transaction,
>>       +                       const char *refname,
>>       +                       const struct object_id *new_oid,
>>       +                       const struct object_id *old_oid,
>>      ++                       const char *reflog_msg,
>>       +                       struct strbuf *err)
>>       +{
>>       +  switch (mode) {
>>      @@ builtin/replay.c: static struct commit *pick_regular_commit(struct repository *r
>>       +          return 0;
>>       +  case REF_ACTION_UPDATE:
>>       +          return ref_transaction_update(transaction, refname, new_oid, old_oid,
>>      -+                                        NULL, NULL, 0, "git replay", err);
>>      ++                                        NULL, NULL, 0, reflog_msg, err);
>>       +  default:
>>       +          BUG("unknown ref_action_mode %d", mode);
>>       +  }
>>      @@ builtin/replay.c: int cmd_replay(int argc,
>>          struct commit *onto = NULL;
>>          const char *onto_name = NULL;
>>          int contained = 0;
>>      -+  const char *ref_action_str = NULL;
>>      -+  enum ref_action_mode ref_action = REF_ACTION_UPDATE;
>>      ++  const char *ref_action = NULL;
>>      ++  enum ref_action_mode ref_mode = REF_ACTION_UPDATE;
>>
>>          struct rev_info revs;
>>          struct commit *last_commit = NULL;
>>      @@ builtin/replay.c: int cmd_replay(int argc,
>>          kh_oid_map_t *replayed_commits;
>>       +  struct ref_transaction *transaction = NULL;
>>       +  struct strbuf transaction_err = STRBUF_INIT;
>>      ++  struct strbuf reflog_msg = STRBUF_INIT;
>>          int ret = 0;
>>
>>       -  const char * const replay_usage[] = {
>>      @@ builtin/replay.c: int cmd_replay(int argc,
>>                             N_("replay onto given commit")),
>>                  OPT_BOOL(0, "contained", &contained,
>>                           N_("advance all branches contained in revision-range")),
>>      -+          OPT_STRING(0, "ref-action", &ref_action_str,
>>      ++          OPT_STRING(0, "ref-action", &ref_action,
>>       +                     N_("mode"),
>>       +                     N_("control ref update behavior (update|print)")),
>>                  OPT_END()
>>      @@ builtin/replay.c: int cmd_replay(int argc,
>>          die_for_incompatible_opt2(!!advance_name_opt, "--advance",
>>                                    contained, "--contained");
>>
>>      -+  /* Default to update mode if not specified */
>>      -+  if (!ref_action_str)
>>      -+          ref_action_str = "update";
>>      -+
>>      -+  /* Validate ref-action mode */
>>      -+  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);
>>      ++  /* Parse ref action mode */
>>      ++  if (ref_action)
>>      ++          ref_mode = parse_ref_action_mode(ref_action, "--ref-action");
>>       +
>>          advance_name = xstrdup_or_null(advance_name_opt);
>>
>>      @@ builtin/replay.c: int cmd_replay(int argc,
>>          determine_replay_mode(repo, &revs.cmdline, onto_name, &advance_name,
>>                                &onto, &update_refs);
>>
>>      ++  /* Build reflog message */
>>      ++  if (advance_name_opt)
>>      ++          strbuf_addf(&reflog_msg, "replay --advance %s", advance_name_opt);
>>      ++  else
>>      ++          strbuf_addf(&reflog_msg, "replay --onto %s",
>>      ++                      oid_to_hex(&onto->object.oid));
>>      ++
>>       +  /* Initialize ref transaction if using update mode */
>>      -+  if (ref_action == REF_ACTION_UPDATE) {
>>      ++  if (ref_mode == REF_ACTION_UPDATE) {
>>       +          transaction = ref_store_transaction_begin(get_main_ref_store(repo),
>>       +                                                    0, &transaction_err);
>>       +          if (!transaction) {
>>      @@ builtin/replay.c: int cmd_replay(int argc,
>>       -                                 decoration->name,
>>       -                                 oid_to_hex(&last_commit->object.oid),
>>       -                                 oid_to_hex(&commit->object.oid));
>>      -+                          if (handle_ref_update(ref_action, transaction,
>>      ++                          if (handle_ref_update(ref_mode, transaction,
>>       +                                                decoration->name,
>>       +                                                &last_commit->object.oid,
>>       +                                                &commit->object.oid,
>>      ++                                                reflog_msg.buf,
>>       +                                                &transaction_err) < 0) {
>>       +                                  ret = error(_("failed to update ref '%s': %s"),
>>       +                                              decoration->name, transaction_err.buf);
>>      @@ builtin/replay.c: int cmd_replay(int argc,
>>       -                 advance_name,
>>       -                 oid_to_hex(&last_commit->object.oid),
>>       -                 oid_to_hex(&onto->object.oid));
>>      -+          if (handle_ref_update(ref_action, transaction, advance_name,
>>      ++          if (handle_ref_update(ref_mode, transaction, advance_name,
>>       +                                &last_commit->object.oid,
>>       +                                &onto->object.oid,
>>      ++                                reflog_msg.buf,
>>       +                                &transaction_err) < 0) {
>>       +                  ret = error(_("failed to update ref '%s': %s"),
>>       +                              advance_name, transaction_err.buf);
>>      @@ builtin/replay.c: int cmd_replay(int argc,
>>       +  if (transaction)
>>       +          ref_transaction_free(transaction);
>>       +  strbuf_release(&transaction_err);
>>      ++  strbuf_release(&reflog_msg);
>>          release_revisions(&revs);
>>          free(advance_name);
>>
>>      @@ t/t3650-replay-basics.sh: test_expect_success 'merge.directoryRenames=false' '
>>        '
>>
>>       +test_expect_success 'default atomic behavior updates refs directly' '
>>      -+  # Store original state for cleanup
>>      -+  test_when_finished "git branch -f topic2 topic1" &&
>>      ++  # Use a separate branch to avoid contaminating topic2 for later tests
>>      ++  git branch test-atomic topic2 &&
>>      ++  test_when_finished "git branch -D test-atomic" &&

>
>>       +
>>       +  # Test default atomic behavior (no output, refs updated)
>>      -+  git replay --onto main topic1..topic2 >output &&
>>      ++  git replay --onto main topic1..test-atomic >output &&
>>       +  test_must_be_empty output &&
>>       +
>>       +  # Verify ref was updated
>>      -+  git log --format=%s topic2 >actual &&
>>      ++  git log --format=%s test-atomic >actual &&
>>       +  test_write_lines E D M L B A >expect &&
>>      -+  test_cmp expect actual
>>      ++  test_cmp expect actual &&
>>      ++
>>      ++  # Verify reflog message includes SHA of onto commit
>>      ++  git reflog test-atomic -1 --format=%gs >reflog-msg &&
>>      ++  ONTO_SHA=$(git rev-parse main) &&
>>      ++  echo "replay --onto $ONTO_SHA" >expect-reflog &&
>>      ++  test_cmp expect-reflog reflog-msg
>>       +'
>>       +
>>       +test_expect_success 'atomic behavior in bare repository' '
>>      ++  # Store original state for cleanup
>>      ++  START=$(git -C bare rev-parse topic2) &&
>>      ++  test_when_finished "git -C bare update-ref refs/heads/topic2 $START" &&
> ...just saving the location for topic2 in this test (and similarly
> just saving the location for main in the next test).  It appears you
> weren't doing anything special with the test-atomic branch, so I'm
> curious why you didn't just use the same idiom for all three tests.
>
>>      ++
>>       +  # Test atomic updates work in bare repo
>>       +  git -C bare replay --onto main topic1..topic2 >output &&
>>       +  test_must_be_empty output &&
>>      @@ t/t3650-replay-basics.sh: test_expect_success 'merge.directoryRenames=false' '
>>       +  # Verify ref was updated in bare repo
>>       +  git -C bare log --format=%s topic2 >actual &&
>>       +  test_write_lines E D M L B A >expect &&
>>      -+  test_cmp expect actual &&
>>      ++  test_cmp expect actual
>>      ++'
>>      ++
>>      ++test_expect_success 'reflog message for --advance mode' '
>>      ++  # Store original state
>>      ++  START=$(git rev-parse main) &&
>>      ++  test_when_finished "git update-ref refs/heads/main $START" &&
>>      ++
>>      ++  # Test --advance mode reflog message
>>      ++  git replay --advance main topic1..topic2 >output &&
>>      ++  test_must_be_empty output &&
>>       +
>>      -+  # Reset for other tests
>>      -+  git -C bare update-ref refs/heads/topic2 $(git -C bare rev-parse topic1)
>>      ++  # Verify reflog message includes --advance and branch name
>>      ++  git reflog main -1 --format=%gs >reflog-msg &&
>>      ++  echo "replay --advance main" >expect-reflog &&
>>      ++  test_cmp expect-reflog reflog-msg
>>       +'
>>       +
>>        test_done
>> -:  ---------- > 3:  b7ebe1f534 replay: add replay.refAction config option
> There was a third patch in v6, but it doesn't show up in your
> range-diff?  Did you specify the range incorrectly by chance when you
> generated this?

>
> Anyway, I looked over the patches as well as the range-diff.  There is
> the slight surprise I had in your lack of consistency with idiom
> choice in the tests, but that's pretty minor and probably doesn't
> merit a re-roll.  This version looks good to me; thanks for working on
> it!