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

[Toon Claes <toon@iotcl.com>]

Tian Yuchen <cat@malon.dev> writes:

> 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?

The comment is a bit misleading. A user cannot set the field `contained`
in the struct when it passed `--ref`. But if a bug is made somewhere,
but both fields can be set, that's what the comment is saying.

I can add a check to verify in replay.c that not both are set, and
update the comment they are incompatible?

>> @@ -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

That's interesting, I didn't think of that.

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

Let me look into that how we can deal with that.

-- 
Cheers,
Toon