From: Siddharth Asthana <siddharthasthana31@gmail.com>
Date: Thu, 27 Nov 2025 19:23:46 GMT
Subject: Re: [PATCH 1/1] replay: add --revert option to reverse commit changes
Message-ID: <78e31e24-5eab-44d8-a7e0-d3efae6ca6cf@gmail.com>
In-Reply-To: <xmqq8qfsmraa.fsf@gitster.g>


On 27/11/25 02:43, Junio C Hamano wrote:
> Siddharth Asthana <siddharthasthana31@gmail.com> writes:
>
>>>> +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.


Right - it's the changes that need to exist in the target tree, not the 
commits themselves. Cherry-picked commits can be reverted even without 
topological ancestry. I will fix the documentation.


>    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'.
>
>>>> +		/* 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.,


Will do.


>
> 		merge_opt->branch2 = xstrfmt("parent of %s",
> 					short_commit_name(repo, pickme));
>
>>>> -	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.


This is the only place in replay.c. The reason we only NULL ancestor and 
branch2 is that those are the ones allocated with xstrfmt() - branch1 
points to short_commit_name() which doesn't need freeing.

A helper would make the intent clearer. I will look into adding one.

Thanks,
Siddharth