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

>
> 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" &&

>     ++
>      +  # 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