Re: [PATCH 1/1] replay: add --revert option to reverse commit changes

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 26, 2025, 21:13 UTC
Message-ID: <xmqq8qfsmraa.fsf@gitster.g>
In-Reply-To: <706e2875-a3f9-447f-9f43-690990a2342d@gmail.com>

Siddharth Asthana <siddharthasthana31@gmail.com> writes:

Show 11 quoted lines

>>> +This creates new commits on top of 'main' that reverse the changes introduced
>>> +by the last three commits on 'feature'. The 'feature' branch is updated to
>>> +point at the last of these revert commits. The 'main' branch is not updated
>>> +in this case.
>> Is there any topological requirement between 'main' and 'feature'
>> branches?
>
> Yes, and I failed to explain this. For reverts to produce meaningful 
> non-empty commits, the commits being reverted should already be in the 
> target branch's history. I will clarify the examples to show this 
> topology requirement explicitly.

We need to be a bit careful, though. Strictly speaking, what we have is not a requirement on the shape of the history. If a topic that was merged to the development branch gets cherry-picked to the master branch, and then it turns out to be faulty and needs to be reverted, we can still "revert" the original topic out of the master branch, even though topologically, the original topic is *not* in 'master'.

Show 8 quoted lines

>>> +		/* For revert: swap base and pickme to reverse the diff */
>>> +		merge_opt->branch1 = short_commit_name(repo, replayed_base);
>>> +		merge_opt->branch2 = xstrfmt("parent of %s", short_commit_name(repo, pickme));
>> That is an overly long line (sorry, I notice these things when a
>> line does not even fit in 92-col terminal).
>
>
> Fixed in my local tree by introducing a `pickme_name` variable.

Just a line-folding at an appropriate column may be sufficient, e.g.,

		merge_opt->branch2 = xstrfmt("parent of %s",
					short_commit_name(repo, pickme));

Show 10 quoted lines

>>> -	free((char*)merge_opt->ancestor);
>>>   	merge_opt->ancestor = NULL;
>>> +	merge_opt->branch2 = NULL;
>> Not a new problem, but what is the point of setting these two (but
>> not branch1) to NULL?
>
>
> You're right, this is inconsistent. The intent is to prevent 
> use-after-free, but setting only some fields to NULL is incomplete. I 
> will either set all three to NULL or add a comment explaining the rationale.

Is this the only place that resets a subset of merge_opt members for reuse? If not, are these multiple places want to reset the same subset of the members? Perhaps we can use a helper function to clarify in such a case.