Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 2 commits into from

Conversation

Jafaral
Copy link
Member

@Jafaral Jafaral commented Aug 23, 2024

Replaces #16587

…ddress"

This reverts commit b3600d8, reversing
changes made to 5111982.
@Cellebyte
Copy link

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

@Jafaral
Copy link
Member Author

Jafaral commented Aug 23, 2024

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

@ton31337
Copy link
Member

ton31337 commented Aug 24, 2024

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.

@Jafaral
Copy link
Member Author

Jafaral commented Aug 26, 2024

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.

@ton31337
Copy link
Member

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

@Cellebyte
Copy link

@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?

@ton31337
Copy link
Member

#16685

@Jafaral
Copy link
Member Author

Jafaral commented Aug 29, 2024

closed in favor of #16587

@Jafaral Jafaral closed this Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants