Re: [PATCH 3/3] replay: allow to specify a ref with option --ref

[Tian Yuchen <cat@malon.dev>]

Hi Toon,

On 3/24/26 00:09, Toon Claes wrote:


> +	die_for_incompatible_opt2(!!opts.ref, "--ref",
> +				  !!opts.contained, "--contained");

> +	/*
> +	 * Reference to update with the result of the replay. This stops replay
> +	 * from taking refs from `onto`, `advance`, and `revert`. Ignores
> +	 * `contained`.
> +	 */
> +	const char *ref;
> +

Do the comment and the code implementation match up here?

> @@ -427,13 +446,9 @@ int replay_revisions(struct rev_info *revs,
>  		goto out;
>  	}
>  
> -	/* In --advance or --revert mode, update the target ref */
> -	if (advance || revert) {
> -		const char *ref = advance ? advance : revert;
> -		replay_result_queue_update(out, ref,
> -					   &onto->object.oid,
> +	if (ref)
> +		replay_result_queue_update(out, ref, &old_oid,
>  					   &last_commit->object.oid);
> -	}
>  
>  	ret = 0;

It doesn't seem like there's anything here to prevent users from 
inputting multiple branches? I think this will result in at least one 
branch becoming orphaned, without any error being reported.

Consider this command:

	git replay --onto main --ref refs/heads/target topic1 topic2

In replay.c we have:

> if (onto_name) {
> 		*onto = peel_committish(repo, onto_name, "--onto");
> 		if (rinfo.positive_refexprs <
> 		    strset_get_size(&rinfo.positive_refs))
> 			die(_("all positive revisions given must be references"));
> 		*update_refs = xcalloc(1, sizeof(**update_refs));
> 		**update_refs = rinfo.positive_refs;
> 		memset(&rinfo.positive_refs, 0, sizeof(**update_refs));


After topic1 and topic2 are put into update_refs, only one ref 
(refs/heads/target) is updated, since what is written in the patch is:

 > +	if (ref)
 > +		replay_result_queue_update(out, ref, &old_oid,
 >  					   &last_commit->object.oid);

I believe this is not what we expect :P

Regards, Yuchen