16

Recently I've had an argument with a teammate that I wanted to have a reverted commit on the master branch from where releases are made as a standalone commit and not merged with future changes.

  • My point of view was that I wanted explicitly show that I reverted another's teammates work and then on top of that I introduced new behavior. For me it was more important to show exactly that I reverted a certain commit.
  • His point of view was that the git history doesn't make sense at that reverted standalone commit because in the commit message I need to reason that I reverted that work in order to introduce a new work in a future commit. Especially that that "future" commit reasoning he didn't like. His view was that I should combine my new work with the reverted commit because only then the commit as a whole makes sense.

Are there best practices when reverting commits?

Should they be shown as separate commits or merged with other (new) meaningful changes and I'm especially interested in the why you would do one or the other?

If this is subjective feel free to close it, never the less I'm really interested how others handle reverted commits in the git history.

Improve this question
7
  • It depends. If the revert was on some feature branch, nobody cares. If the revert was on the branch from which releases are made, then a revert should be equivalent to a merge, with proper reasoning, reviews, stand alone and not bundled with other stuff. Commented Nov 16, 2023 at 7:43
  • Edited the question. The revert was on the master branch form where releases are made. Thanks for your view. Can you point out why it should be equivalent to a merge and not bundled with other stuff? Commented Nov 16, 2023 at 7:46
  • git revert will always create a revert commit. So what are you actually doing that not creating a revert commit is on the table? Commented Nov 17, 2023 at 11:59
  • 3
    IMHO having separate reverts is a best-practice... It doesn't make sense if you are developing something locally and so you end up reverting your own changes, but once something is published in main then I believe it does make a lot of sense to let people know "this commit specifically was reverted for reason X" and keep it as standalone change that can be discussed. This is especially useful if you need to deploy fast and you know commit X broke main, but fixing it properly would take too much time, you can just revert and let the dev know they need to propose a new version that works Commented Nov 18, 2023 at 14:44
  • 1
    My reasoning is that there might be a bug in the code that you committed after the revert. Now if they want to revert your commit with the new code, they will also have to revert the revert to get back to normal functionality if the original code was beneficial on its own. Commented Nov 19, 2023 at 4:30

5 Answers 5

Reset to default
46

Creating a revert-commit sends a message that the original commit was flawed and should not have been made.

Sometimes, that message is justified, for example when a commit introduced a serious bug, but there could be other reasons for a commit to be flawed.

At other times, the code that you backed out was fine and functioned properly until you got a change in the requirements. In that case, a separate revert-commit sends the wrong message. There was nothing inherently wrong with that commit, it is just that a change in requirements made its content obsolete. In that case, just make a normal commit with the changes you made for your new feature (including both additions, deletions and modifications as appropriate). The fact that the deleted and modified code was originally written by someone else should not matter in any way.

Improve this answer
11
  • 5
    @BrunoBieri, yes that sounds like your colleague was "right" and you were just doing ongoing development that happened to include the removal of some existing code. Commented Nov 16, 2023 at 8:18
  • 6
    This answer is fine, but let me add there can be more reasons for a commit to be flawed than just a serious bug. Commented Nov 16, 2023 at 9:25
  • 5
    @BrunoBieri: well, I am sure you have stumbled across enough examples by yourself where a certain code change did not have the quality it should have. It could be a change which increased the risk for future errors, unnecessary code duplication, an overcomplicated solution, a "too simple" solution only fixing part of what was intended, or "fixing" a bug by masking it. Sometimes, these changes can still be improved gradually. But sometimes, it is better to throw them out of the code base and rework them from scratch. Commented Nov 16, 2023 at 10:07
  • 6
    As someone who writes a lot of code that gets thrown away I just want to say that how it gets thrown away is far less important than that it is thrown away. Easily. Without any need to pass judgement on the wisdom of it's creation. Code doesn't need any help making it difficult to change. Commented Nov 16, 2023 at 14:40
  • 3
    I'd agree with this - the purpose of a "revert" is to undo the previous commit completely, as if it had never happened... usually because that previous commit has broken things. I wouldn't expect to see them used for a case where you've decided to change the solution instead... in that situation, I'd expect to see a commit which replaced the old with the new. Commented Nov 16, 2023 at 21:05
27

I think there's a lot more thought about blame and pride going into this than is necessary.

When I see a revert I only think one thing: I can ignore the reverted code because it doesn't feed into anything.

Why it got created, why it got removed can be explained in the commit messages. But really, who cares? I throw away a lot more code than I commit. If what I commit makes it into the main branch, hooray. If not, oh well.

Assume both the person who wrote the unused code and the person who removed it are dead (or won the lottery) and don't care about this. What makes life easier for those who are left who must maintain this?

This attitude is called egoless coding1,2. If your team isn't there yet then work to get them there.

Improve this answer
0
6

TL;DR: Pull Requests and Commits are different things.

Let's talk about workflow:

  • A Commit is a unit of change. It is focused, to make review easy.
  • A Pull Request is a unit of "deployment". It encompasses anything that is necessary for the application to pass acceptance tests.

For example, it is usual to do some refactoring prior to introducing a new functionality, and then throw in some clean-up once the new functionality is in. This may be done in a single Pull Request -- to leave the application in a good state -- but is better done in several commits: X refactorings, Y pieces of functionality, Z clean-ups.

So, when should you revert on master?

First of all, you should generally revert a full Pull Request, not just one commit in the middle. You revert, in reverse order, the commits of the pull requests. Sometimes, further adjustments will be necessary, in which case those should be in a separate commit to clearly identify them -- though ideally it may be easier to just revert the commits introducing those adjustments.

You should revert a Pull Request if it's unfit for the branch. Maybe it's deeply flawed. Maybe the solution "works" but is terrible. Your organization, your rules.

And that's it.

From what I understand of your situation, however, the reverted commit is NOT unfit, it just turns out that you need a different piece of functionality, or maybe to do it differently.

In such case, I would not "revert as a PR", as the revert is not really "stand-alone". Instead, I would argue for a Pull Request where the revert will be one of the "preparation" commits, alongside refactorings. This will let anyone who inspects the PR (or history) clearly see that the "old" functionality was wiped clean first, and then see what the "new" functionality is without interference.

I would advise AGAINST squashing the "new" functionality with the revert of the "old" one, as it makes reviewing the diff later more difficult.

Improve this answer
5
  • Thanks for your contribution. If I understand your point correctly you would support my view point. I prepared a new PR in which I reverted 1 commit from the past and wanted to have this as a separate commit. Then building on top of that reverted commit my changes. As my teammate pointed out and also others point out the commit message in that reverted commit will then need to explain why it was reverted and here my teammate just doesn't agree that there the reason is that "a future" commit "will" introduce changes. It just doesn't make sense for him to refer to the future. Commented Nov 17, 2023 at 13:56
  • What is your suggestion to write in a message of such a reverted commit? Commented Nov 17, 2023 at 13:57
  • 2
    @BrunoBieri: Then I disagree with your teammate. Any "preparation" commit is always done in preparation for a future commit, and they're essential to focused commits. The justification -- and thus the error message -- IS that preparations are being made for future work. You can explain why the specific preparations are necessary -- adding X to make it possible to customize Y -- without explicitly referencing a future commit, but it's merely following the letter without following the spirit. Seen another way, you have to build the foundations before you can build the house on top. Commented Nov 17, 2023 at 14:12
  • I would agree with Matthieu and argue that you can create a commit that removes the functionality (what you would like to revert) and name it something other than revert. As many people have written, a revert commit looks like a mistake. Revert in a separate commit to document the changes and give it a sensible name to make it clear why. Commented Nov 18, 2023 at 19:12
  • 2
    "Remove functionality from email service" or even "Revert to a simpler time before feature X" describes what you're changing in the code and can even describe why. "Revert 'Commit by BradleyCooper'" describes what your changing in the history. Commented Nov 18, 2023 at 19:18
2

When you call out a revert (as a separate commit) you are basically saying:

The potential downside of the original commit outweighs the potential upside.

I threw the word "potential" in there as there could be doubt as to whether a particular commit either causes (or fixes) a critical bug. I agree with your co-worker that you should ALWAYS have a reason for doing a revert (even if the reason is just to test an assertion).

I think Bart's answer sights a bad example - we delete code from applications when we feel it is cluttering the code base / making it more difficult to read - in such cases it has negative value with respect to the current/future requirements. In that case I think the revert should still be called out - though it's perfectly fine to comment in the revert - that the revert is being done because of a change in requirements.

When a commit is mostly unnecessary (or clearly wrong) it make sense to revert it. When a commit is almost right, it makes sense to do a bug fix. The difficult situation is when a commit has 50% needed code and 50% bad/unneeded - in this case I would be tempted to revert the commit and then create a new commit with the code I actually wanted. However I think I have to concede personal preference as the reason for that.

Improve this answer
0

Excuse the language, but...

About 80% of the time when a revert is appropriate, it is used to signal "Something is fucked up". It may be a major fuckup or a minor fuckup, could be an almost harmless fuckup, but generally it's a fuckup. Not a bug, not unclear requirements, a fuckup. "This shit should not have been done".

I've mainly encountered it as trivial, as long as it is fixed. Checked stuff into the wrong branch or completed a pull request before it was ready. No harm no foul, reading the history, you see that it wasn't supposed to be done, and so was undone.

I'm not saying that it was fucked to cast blame on the person that did it or to doubt their skills or any kind of negative implication for the PERSON that did it, I've fucked up, most people have. But it's not merely an oversight or bad math or whatever.

I can't say whether you should revert or not, but I do have a question for you: was it a fuckup to make that commit? Not did it not work, not did it contain a bug, but "that change should not have been made".

Improve this answer

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.