Subject: Re: [PATCH v6 2/3] replay: make atomic ref updates the default behavior

From: Phillip Wood <[email-masked]>
Date: Mon, 3 Nov 2025 16:25:23 +0000

Hi Siddharth

On 30/10/2025 19:19, Siddharth Asthana wrote:

> +	case REF_ACTION_UPDATE:
> +		return ref_transaction_update(transaction, refname, new_oid, old_oid,
> +					      NULL, NULL, 0, "git replay", err);

I wonder if we should use a more descriptive reflog message here that 
says what git replay was doing. For example "git replay --onto 
<new-base> <revs>" could include the new base in the reflog message like 
"git rebase" does. For "git replay --advance" we could include the 
commits that have been picked. It would be helpful to test the reflog 
message in the new tests as well.

Thanks

Phillip

> +	default:
> +		BUG("unknown ref_action_mode %d", mode);
> +	}
> +}
> +
>   int cmd_replay(int argc,
>   	       const char **argv,
>   	       const char *prefix,
> @@ -294,6 +321,8 @@ 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;
>   
>   	struct rev_info revs;
>   	struct commit *last_commit = NULL;
> @@ -302,12 +331,14 @@ int cmd_replay(int argc,
>   	struct merge_result result;
>   	struct strset *update_refs = NULL;
>   	kh_oid_map_t *replayed_commits;
> +	struct ref_transaction *transaction = NULL;
> +	struct strbuf transaction_err = STRBUF_INIT;
>   	int ret = 0;
>   
> -	const char * const replay_usage[] = {
> +	const char *const replay_usage[] = {
>   		N_("(EXPERIMENTAL!) git replay "
>   		   "([--contained] --onto <newbase> | --advance <branch>) "
> -		   "<revision-range>..."),
> +		   "[--ref-action[=<mode>]] <revision-range>..."),
>   		NULL
>   	};
>   	struct option replay_options[] = {
> @@ -319,6 +350,9 @@ 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,
> +			   N_("mode"),
> +			   N_("control ref update behavior (update|print)")),
>   		OPT_END()
>   	};
>   
> @@ -333,6 +367,18 @@ 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";
> +
> +	/* Parse 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);
> +
>   	advance_name = xstrdup_or_null(advance_name_opt);
>   
>   	repo_init_revisions(repo, &revs, prefix);
> @@ -389,6 +435,17 @@ int cmd_replay(int argc,
>   	determine_replay_mode(repo, &revs.cmdline, onto_name, &advance_name,
>   			      &onto, &update_refs);
>   
> +	/* Initialize ref transaction if using update mode */
> +	if (ref_action == REF_ACTION_UPDATE) {
> +		transaction = ref_store_transaction_begin(get_main_ref_store(repo),
> +							  0, &transaction_err);
> +		if (!transaction) {
> +			ret = error(_("failed to begin ref transaction: %s"),
> +				    transaction_err.buf);
> +			goto cleanup;
> +		}
> +	}
> +
>   	if (!onto) /* FIXME: Should handle replaying down to root commit */
>   		die("Replaying down to root commit is not supported yet!");
>   
> @@ -434,10 +491,15 @@ int cmd_replay(int argc,
>   			if (decoration->type == DECORATION_REF_LOCAL &&
>   			    (contained || strset_contains(update_refs,
>   							  decoration->name))) {
> -				printf("update %s %s %s\n",
> -				       decoration->name,
> -				       oid_to_hex(&last_commit->object.oid),
> -				       oid_to_hex(&commit->object.oid));
> +				if (handle_ref_update(ref_action, transaction,
> +						      decoration->name,
> +						      &last_commit->object.oid,
> +						      &commit->object.oid,
> +						      &transaction_err) < 0) {
> +					ret = error(_("failed to update ref '%s': %s"),
> +						    decoration->name, transaction_err.buf);
> +					goto cleanup;
> +				}
>   			}
>   			decoration = decoration->next;
>   		}
> @@ -445,10 +507,23 @@ int cmd_replay(int argc,
>   
>   	/* In --advance mode, advance the target ref */
>   	if (result.clean == 1 && advance_name) {
> -		printf("update %s %s %s\n",
> -		       advance_name,
> -		       oid_to_hex(&last_commit->object.oid),
> -		       oid_to_hex(&onto->object.oid));
> +		if (handle_ref_update(ref_action, transaction, advance_name,
> +				      &last_commit->object.oid,
> +				      &onto->object.oid,
> +				      &transaction_err) < 0) {
> +			ret = error(_("failed to update ref '%s': %s"),
> +				    advance_name, transaction_err.buf);
> +			goto cleanup;
> +		}
> +	}
> +
> +	/* Commit the ref transaction if we have one */
> +	if (transaction && result.clean == 1) {
> +		if (ref_transaction_commit(transaction, &transaction_err)) {
> +			ret = error(_("failed to commit ref transaction: %s"),
> +				    transaction_err.buf);
> +			goto cleanup;
> +		}
>   	}
>   
>   	merge_finalize(&merge_opt, &result);
> @@ -460,6 +535,9 @@ int cmd_replay(int argc,
>   	ret = result.clean;
>   
>   cleanup:
> +	if (transaction)
> +		ref_transaction_free(transaction);
> +	strbuf_release(&transaction_err);
>   	release_revisions(&revs);
>   	free(advance_name);
>   
> diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh
> index 58b3759935..123734b49f 100755
> --- a/t/t3650-replay-basics.sh
> +++ b/t/t3650-replay-basics.sh
> @@ -52,7 +52,7 @@ test_expect_success 'setup bare' '
>   '
>   
>   test_expect_success 'using replay to rebase two branches, one on top of other' '
> -	git replay --onto main topic1..topic2 >result &&
> +	git replay --ref-action=print --onto main topic1..topic2 >result &&
>   
>   	test_line_count = 1 result &&
>   
> @@ -68,7 +68,7 @@ test_expect_success 'using replay to rebase two branches, one on top of other' '
>   '
>   
>   test_expect_success 'using replay on bare repo to rebase two branches, one on top of other' '
> -	git -C bare replay --onto main topic1..topic2 >result-bare &&
> +	git -C bare replay --ref-action=print --onto main topic1..topic2 >result-bare &&
>   	test_cmp expect result-bare
>   '
>   
> @@ -86,7 +86,7 @@ test_expect_success 'using replay to perform basic cherry-pick' '
>   	# 2nd field of result is refs/heads/main vs. refs/heads/topic2
>   	# 4th field of result is hash for main instead of hash for topic2
>   
> -	git replay --advance main topic1..topic2 >result &&
> +	git replay --ref-action=print --advance main topic1..topic2 >result &&
>   
>   	test_line_count = 1 result &&
>   
> @@ -102,7 +102,7 @@ test_expect_success 'using replay to perform basic cherry-pick' '
>   '
>   
>   test_expect_success 'using replay on bare repo to perform basic cherry-pick' '
> -	git -C bare replay --advance main topic1..topic2 >result-bare &&
> +	git -C bare replay --ref-action=print --advance main topic1..topic2 >result-bare &&
>   	test_cmp expect result-bare
>   '
>   
> @@ -115,7 +115,7 @@ test_expect_success 'replay fails when both --advance and --onto are omitted' '
>   '
>   
>   test_expect_success 'using replay to also rebase a contained branch' '
> -	git replay --contained --onto main main..topic3 >result &&
> +	git replay --ref-action=print --contained --onto main main..topic3 >result &&
>   
>   	test_line_count = 2 result &&
>   	cut -f 3 -d " " result >new-branch-tips &&
> @@ -139,12 +139,12 @@ test_expect_success 'using replay to also rebase a contained branch' '
>   '
>   
>   test_expect_success 'using replay on bare repo to also rebase a contained branch' '
> -	git -C bare replay --contained --onto main main..topic3 >result-bare &&
> +	git -C bare replay --ref-action=print --contained --onto main main..topic3 >result-bare &&
>   	test_cmp expect result-bare
>   '
>   
>   test_expect_success 'using replay to rebase multiple divergent branches' '
> -	git replay --onto main ^topic1 topic2 topic4 >result &&
> +	git replay --ref-action=print --onto main ^topic1 topic2 topic4 >result &&
>   
>   	test_line_count = 2 result &&
>   	cut -f 3 -d " " result >new-branch-tips &&
> @@ -168,7 +168,7 @@ test_expect_success 'using replay to rebase multiple divergent branches' '
>   '
>   
>   test_expect_success 'using replay on bare repo to rebase multiple divergent branches, including contained ones' '
> -	git -C bare replay --contained --onto main ^main topic2 topic3 topic4 >result &&
> +	git -C bare replay --ref-action=print --contained --onto main ^main topic2 topic3 topic4 >result &&
>   
>   	test_line_count = 4 result &&
>   	cut -f 3 -d " " result >new-branch-tips &&
> @@ -217,4 +217,32 @@ test_expect_success 'merge.directoryRenames=false' '
>   		--onto rename-onto rename-onto..rename-from
>   '
>   
> +test_expect_success 'default atomic behavior updates refs directly' '
> +	# Store original state for cleanup
> +	test_when_finished "git branch -f topic2 topic1" &&
> +
> +	# Test default atomic behavior (no output, refs updated)
> +	git replay --onto main topic1..topic2 >output &&
> +	test_must_be_empty output &&
> +
> +	# Verify ref was updated
> +	git log --format=%s topic2 >actual &&
> +	test_write_lines E D M L B A >expect &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'atomic behavior in bare repository' '
> +	# Test atomic updates work in bare repo
> +	git -C bare replay --onto main topic1..topic2 >output &&
> +	test_must_be_empty output &&
> +
> +	# 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 &&
> +
> +	# Reset for other tests
> +	git -C bare update-ref refs/heads/topic2 $(git -C bare rev-parse topic1)
> +'
> +
>   test_done