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

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

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

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