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

[Junio C Hamano <gitster@pobox.com>]

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.