frr icon indicating copy to clipboard operation
frr copied to clipboard

Revert ipv4-mapped ipv6 and 6vpe nexthop in BGP (backport #16615)

Open Jafaral opened this issue 1 year ago • 6 comments

Replaces #16587

Jafaral avatar Aug 23 '24 05:08 Jafaral

@Jafaral what is the difference between this PR and #16587?

Cellebyte avatar Aug 23 '24 06:08 Cellebyte

@Jafaral what is the difference between this PR and #16587?

Done slightly differently. The revert in such case should happen at the PR level and not at the commit level as it was done in 16587. This way you can track the original work for better traceability.

Jafaral avatar Aug 23 '24 15:08 Jafaral

I disagree. Here is the detailed explanation: https://mirrors.edge.kernel.org/pub/software/scm/git/docs/howto/revert-a-faulty-merge.txt.

Linus explains the situation:

Reverting a regular commit just effectively undoes what that commit
did, and is fairly straightforward. But reverting a merge commit also
undoes the _data_ that the commit changed, but it does absolutely
nothing to the effects on _history_ that the merge had.

So the merge will still exist, and it will still be seen as joining
the two branches together, and future merges will see that merge as
the last shared state - and the revert that reverted the merge brought
in will not affect that at all.

So a "revert" undoes the data changes, but it's very much _not_ an
"undo" in the sense that it doesn't undo the effects of a commit on
the repository history.

So if you think of "revert" as "undo", then you're going to always
miss this part of reverts. Yes, it undoes the data, but no, it doesn't
undo history.

For example, think about what reverting a merge (and then reverting the revert) does to bisectability. Ignore the fact that the revert of a revert is undoing it - just think of it as a "single commit that does a lot". Because that is what it does.

I think it's going to be much more messier when we need to do bisecting and/or re-reverting things. I'm pretty sure we will be screwed up at some time.

Conclusion:

So from a technical angle, there's nothing wrong with reverting a merge, but from a workflow angle it's something that you generally should try to avoid.

ton31337 avatar Aug 24 '24 08:08 ton31337

For example, think about what reverting a merge (and then reverting the revert) does to bisectability. Ignore the fact that the revert of a revert is undoing it - just think of it as a "single commit that does a lot". Because that is what it does.

I think it's going to be much more messier when we need to do bisecting and/or re-reverting things. I'm pretty sure we will be screwed up at some time.

Conclusion:

So from a technical angle, there's nothing wrong with reverting a merge, but from a workflow angle it's something that you generally should try to avoid.

This is a bit out of context. The point isn't to go revert every single commit to fix the problem, but to go and find the exact commit that cause the problem, revert/fix that. Here is another quote from that link:

If at all possible, for example, if you find a problem that got merged into the main tree, rather than revert the merge, try really hard to bisect the problem down into the branch you merged, and just fix it, or try to revert the individual commit that caused it.

So, that link doesn't say you should revert a PR by reverting every commit in the PR, it says go fix the problem (revert the offending commit ) because otherwise it is going to be a mess regardless of how you handle it.

Jafaral avatar Aug 26 '24 19:08 Jafaral

In the history, we will see ONLY a merge commit reverted, instead of every commit separately, which is weird.

ton31337 avatar Aug 27 '24 12:08 ton31337

@ton31337 @Jafaral can you agree on 1 way to revert it. (Either using Merge Commit Revert or Git History Revert) I would like to have a working frr version 10.1.1. Maybe document the decided way of reverting in a CONTRIBUTOR.md file?

Cellebyte avatar Aug 28 '24 09:08 Cellebyte

https://github.com/FRRouting/frr/pull/16685

ton31337 avatar Aug 29 '24 06:08 ton31337

closed in favor of #16587

Jafaral avatar Aug 29 '24 15:08 Jafaral