Re: [PATCH 1/2] replay: add --update-refs option

From: Siddharth Asthana <siddharthasthana31@gmail.com>
Date: Sep 9, 2025, 06:58 UTC
Message-ID: <c7615356-04cc-47e2-a894-4d24e416e4ad@gmail.com>
In-Reply-To: <aL6n8KEHSDii5Wd1@pks.im>

On 08/09/25 15:24, Patrick Steinhardt wrote:

Show 17 quoted lines

> On Mon, Sep 08, 2025 at 10:06:19AM +0530, Siddharth Asthana wrote:
>> diff --git a/builtin/replay.c b/builtin/replay.c
>> index 6172c8aacc..a33c9887cf 100644
>> --- a/builtin/replay.c
>> +++ b/builtin/replay.c
>> @@ -284,6 +284,37 @@ static struct commit *pick_regular_commit(struct repository *repo,
>>   	return create_commit(repo, result->tree, pickme, replayed_base);
>>   }
>>   
>> +static int update_ref_direct(struct repository *repo, const char *refname,
>> +			     const struct object_id *new_oid,
>> +			     const struct object_id *old_oid)
>> +{
>> +	const char *msg = "replay";
>> +	return refs_update_ref(get_main_ref_store(repo), msg, refname,
>> +			       new_oid, old_oid, 0, UPDATE_REFS_MSG_ON_ERR);
>> +}

Hi Patrick,

Thanks for the detailed review

> Is there a strong reason why a user would want to update refs one by
> one? If not, let's not add new code to our base that does so. This is
> known to be inperformant for the reftable backend, but also for the
> files backend in some cases.

You are absolutely right about the performance concern. My thinking was to provide a simple mode that exactly mimics "git replay | git update-ref --stdin" behavior, but I see that's not worth the performance cost.

I will remove the individual update function and only use batched transactions with REF_TRANSACTION_ALLOW_FAILURE when needed.

Show 22 quoted lines

> If we really want to support the case where
> only a subset of references gets committed we should be using batched
> updates with the `REF_TRANSACTION_ALLOW_FAILURE` flag.
>
>> @@ -319,6 +355,12 @@ int cmd_replay(int argc,
>>   			   N_("replay onto given commit")),
>>   		OPT_BOOL(0, "contained", &contained,
>>   			 N_("advance all branches contained in revision-range")),
>> +		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")),
>>   		OPT_END()
>>   	};
>>   
> So I think we should reduce this to only accept two flags:
> `--update-refs` and a flag that accepts a subset of refs failing.o
>
> We might also want to make this something like `--update-refs[=<mode>]`,
> where `<mode>` could be "allow-failures".

That make sense. Would you prefer `--update-refs` with `--allow-failures` as a separate flag? I am leaning toward that since it's clearer than the parameter syntax.

Show 17 quoted lines

>
>> @@ -333,6 +375,14 @@ int cmd_replay(int argc,
>>   	if (advance_name_opt && contained)
>>   		die(_("options '%s' and '%s' cannot be used together"),
>>   		    "--advance", "--contained");
>> +
>> +	if (update_directly && update_refs_flag)
>> +		die(_("options '%s' and '%s' cannot be used together"),
>> +		    "--update", "--update-refs");
>> +
>> +	if (batch_mode && !update_refs_flag)
>> +		die(_("option '%s' can only be used with '%s'"),
>> +		    "--batch", "--update-refs");
>>   	advance_name = xstrdup_or_null(advance_name_opt);
>>   
>>   	repo_init_revisions(repo, &revs, prefix);
> We have the `die_for_incompatible_opt*()` helpers for this.

Thanks, I will use those.

Show 10 quoted lines

>
>> @@ -389,6 +439,18 @@ int cmd_replay(int argc,
>>   	determine_replay_mode(repo, &revs.cmdline, onto_name, &advance_name,
>>   			      &onto, &update_refs);
>>   
>> +	/* Initialize ref transaction if using --update-refs */
> Nit: the comment doesn't really add much context, so I'd just drop it.
> It's generally discouraged to add a comment that re-states what the code
> already says. Instead, comments should point out things that are easy to
> miss or not obvious at all.

Will remove the redundant comments.

Show 34 quoted lines

>
>> @@ -445,10 +525,43 @@ 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 (update_directly) {
>> +			if (update_ref_direct(repo, advance_name,
>> +					     &last_commit->object.oid,
>> +					     &onto->object.oid) < 0) {
>> +				ret = -1;
>> +				goto cleanup;
>> +			}
>> +		} else if (transaction) {
>> +			if (add_ref_to_transaction(transaction, advance_name,
>> +						   &last_commit->object.oid,
>> +						   &onto->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",
>> +			       advance_name,
>> +			       oid_to_hex(&last_commit->object.oid),
>> +			       oid_to_hex(&onto->object.oid));
>> +		}
>> +	}
>> +
>> +	/* Commit the ref transaction if we have one */
> Likewise here.

Will remove the redundant comments here too.

Thank,

Siddharth

Show 18 quoted lines

>
>> +	if (transaction && result.clean == 1) {
>> +		if (ref_transaction_commit(transaction, &transaction_err)) {
>> +			if (batch_mode) {
>> +				/* Print failed updates in batch mode */
>> +				warning(_("some ref updates failed: %s"), transaction_err.buf);
>> +				ref_transaction_for_each_rejected_update(transaction,
>> +										 print_rejected_update, NULL);
>> +			} else {
>> +				/* In atomic mode, all updates failed */
>> +				ret = error(_("failed to update refs: %s"), transaction_err.buf);
>> +				goto cleanup;
>> +			}
>> +		}
>>   	}
>>   
>>   	merge_finalize(&merge_opt, &result);
> Patrick