Should I tell someone that their commit caused a regression?

Question

When you track down and fix a regression—i.e. a bug that caused previously working code to stop working—version control makes it entirely possible to look up who committed the change that broke it.

Is it worth doing this? Is it constructive to point this out to the person that made the commit? Does the nature of the mistake (on the scale of simple inattentiveness to fundamental misunderstanding of the code they changed) change whether or not it's a good idea?

If it is a good idea to tell them, what are good ways to do it without causing offense or causing them to get defensive?

Assume, for the sake of argument, that the bug is sufficiently subtle that the CI server's automated tests can't pick it up.

Answer 1

If you just approach them to tell them about a mistake they made then unless you are the best diplomat in the world its going to be difficult for it not to just sound like "Ha! - look at this mistake you made!". We are all human and criticism is difficult to take.

On the other hand unless the change is completely trivial and obviously wrong I normally find it benificial to talk to the person who committed the original change as part of my investigation just to make sure that I fully understand whats going on, hence the way I usually end up handling these situation is by going over to said person and having a conversation that goes a little like this:

Me: I'm working on this bug where ... summary of bug ... and I think I've tracked down the problem to a change you made. Can you remember what this change was for? / have you got some time to explain this change?

Then either:

Them: Sure, thats to handle ... situation I wasn't aware of ...

Or something along the lines of:

Them: Nope sorry I don't remember, looks wrong to me.

By going and investigating the change / bug together the original committer gets to learn from their mistakes without just feeling like they are being criticised*, and there is also a pretty good chance that you will learn something too.

If the original committer isn't around or is busy then you can alwasy just slog through and figure it all out yourself, I just normally find that talking to the person who originally made the change is quicker.

* Of course this is only going to work if you are genuinely interested in the other persons help. If you are just using this as a thinly disguised method of telling someone about a mistake they made then this is probably worse than just being open about it.

Answer 2

Be assertive not aggressive. Always favour saying something akin to "this piece of code is not working" vs "your code is not working". Criticise the code, not the person who wrote the code.

Better yet, if you can think of a solution, fix it and push to them -- assuming you have a distributed version control system. Then ask them if your fix is valid for the bug they were fixing. Overall, try to increase both your and their knowledge of programming. But do it without your ego getting in the way.

Of course, you should be willing to listen to other developers coming to you with the same problem and act how you would have wished they did.

Answer 3

Yes, always. As a programmer, it's your job to learn from mistakes.

Letting them know mistakes they make will help them become a better coder and reduce their chance of making mistakes in future. BUT do be polite and don't make a big deal of it, we all create bugs every so often. I find a polite email is a very non-confrontational way of letting people know.

Answer 4

The constructive way is to find the bug, fix it and take actions to avoid similar bugs to arise in the future.

If it involves explaining people how not to introduce bugs, go for it.

Once, I worked in a team where the project manager never told a particular developer that he made a mistake: he organised a meeting with the whole team where he explained that a mistake was made and that a new process has been defined in order to suppress that kind of mistakes. That way, nobody was stigmatized.

Answer 5

In general, yes.

Nobody should get defensive if you're tactful about it. An easy way to handle it is to ask them to double-check your change before you commit it back to the trunk (or whatever is relevant for your version control system). People will appreciate it if you save them a few minutes by fixing obvious errors, but they won't appreciate it if you fix something that wasn't broken and end up breaking their code. Giving them a chance to review your change tells them that you don't want to step on their toes and gives them an opportunity to object to your changes.

If it's a big change rather than just a typo, it's a good idea to give the author a heads up before you dig into trying to fix it. "Joe, I was merging my own stuff yesterday and found something that I'm not sure I understand. It looks like a bug, but I wanted to run it by you before I go messing with your code. Would you take a look with me?"

Your relationship with the author is a big factor. If you wouldn't mind the author fixing your code without telling you, and if you're pretty sure the feeling is mutual, then it might not be worth mentioning. If it's someone with more experience/seniority/status, you'll want to let them know that you're going to change their code. If it's someone with less, then consider whether it's the sort of thing they need to hear to avoid repeating the mistake or it might embarrass them needlessly.

Always remember that if you can find out who checked in the "bug", they can as easily find out who "fixed" their code. If you think they'd be upset/annoyed/embarrassed at finding out about your change after the fact, by all means tell them beforehand.

Also, fixing the bug isn't your only option. You can always just report the bug in your issue tracker. Tact is again required here -- reporting the bug makes it more visible to the entire team, but it also gives the author a chance to fix his or her own mistake. Reporting is the best option if you're not certain about the best way to fix the problem or if you just don't have time to fix it.

Answer 6

If I make a commit that includes a bug, you had better tell me. If I find a commit of yours that includes a bug, I will surely tell you.

We only improve when we comprehend our errors. That's how we produce better code in the future.

Answer 7

You're getting excellent answers here.

I could only add a technique I learned from a manager once when I would make a mistake.

I was the middle-aged consultant with the Ph.D. and she was the young manager without, so there could have been a perceived prestige gradient. At any rate, she had clearly had experience with this situation and knew how to handle it.

She mentioned to me in an almost apologetic tone that there seemed to be a problem, and would I have time to look into it?

Often enough, the error was mine, and she knew it. That is skill.

Answer 8

I think there's a deeper issue underlying this question. Yes, the submitter should certainly be made aware of the consequences of their change, so that they can understand what happened and not do the same thing again. However, the context of your question indicates that you prepared and submitted a fix without the original submitter's knowledge that they even caused a problem. Therein lies the deeper issue: why doesn't the submitter already know about the regression and why didn't they fix it themselves? The situation you described may indicate a lack of accountability or vigilance on the part of the original submitter, which is a potential concern with respect to their overall performance and motivation.

My software engineering experience has taught me to own all of my code changes, not just projects I'm responsible for, all the way to production, which includes being aware of their impact, including on your build system and (obviously) product behavior.

If someone's change has caused a problem, it doesn't mean the person is a bad engineer, but usually they should be responsible for and involved in fixing whatever has gone wrong. Even if they are not "at fault," e.g. their code exposed an underlying bug that has existed in the codebase for years, they should be one of the first people to be aware of a problem with their change. Even if the original submitter isn't the right person to fix the bug, they should be closely connected to the life cycle of their change.

Answer 9

Good traction on your question! Everyone's told you what do. Should you tell? YES! Anytime the question asks "should I communicate more?", the answer is almost always YES!

But to add something different: your premise is flawed.

A co-worker made a commit that didn't break CI, but lead you to discover a problem.

Congrats! You found a new bug, not a regression. Seriously, do you manually test every scenario and line of code not covered by automated (or standardised manual) testing when you commit?

By all means, get your colleague involved in the fix, with tests to ensure it can't happen again. You are both heroes! But if you let slip any blame in word or action, you are responsible for perpetuating one of the worst organisational diseases: accountability without responsibility.

If you really need to find a villian, think about the guy who committed the original code that broke, and left a trap for your unsuspecting buddy (obviously without sufficient test coverage). Hopefully that wasn't you!

Answer 10

Always regard the other person as someone better than you , always see others good characteristics and always know that I can make mistakes too.

Tell them when its only the two of you.

Answer 11

If someone gets offended when you said him he made a mistake, it means he thinks he is the wisest on the earth and makes no mistake, and when criticized, he gets the feeling, as we said in Poland, that 'crown is falling from his head'.

So you shouldn't hesitate to say that someone has made a mistake. It is normal. Everyone makes mistakes, even the best! Only those who make nothing make no mistakes ;)

Answer 12

In addition to what others have said, make sure it's ACTUALLY their commit that caused a bug. Certainly don't blame someone else for your own mistake. No matter how tactfully you approach them, you're still going to piss them off if you've blamed them for something they didn't do. (Speaking as someone who has been blamed for other peoples' bugs constantly; one time someone came up to me and said I did something utterly stupid and I brought up the commit log and found that the last person to touch that line of code was the person who was blaming me. Somehow he still seemed to think it was my fault because I'd written the line originally.)

Answer 13

Why don't I see a single answer here that reflects the top voted comment on the question??

Yes, absolutely tell them about it, but don't do it in front of the entire team

Approach the developer 1:1 and point out the bug. Don't make a big deal of it. I always thought that pointing out the error in front of the entire team was a bad idea. It might work for some developers, but its not for everyone and can have a negative effect. Remember, we've all been in their shoes at some point or another, and as the 2nd top voted answer says, you learn from your mistakes

I usually find it works best when you start with a compliment and then get to the error... something like "the fix you implemented works great, BUT it seems to have broken x,y,z", or "thanks for doing a,b,c, BUT it seems to be causing x,y,z"

Answer 14

Simple answer: Yes.

Longer answer: My last job was at an Agile company that used TDD with CI tools to ensure that what was in our SVN repo was good, working code at all times. When something was committed, our TeamCity server got a copy, compiled, and ran unit tests. It also ran integration tests hourly. If something was committed that caused the CI to fail, everyone got an e-mail stating the build had broken based on a commit by a particular person.

That didn't always catch everything; woe to us, we didn't enforce code coverage, and even if something was covered by unit or integration tests, they might not exercise that code sufficiently. When that happened, whoever got the task of fixing the known issue (if QA caught it) or defect (if, dun-dun-dun, the clients did), would run a "blame" (shows who last modified each line of a code file) and determine the culprit.

Calling someone out for checking in broken code is not necessarily a bad thing. They have failed to do their job properly, and either they or someone else had to go back and fix the mistake. This happens all the time; how big a deal it should be depends on how easy the fix was, whether the mistake indicates the person didn't even compile or run the code in question, and the overall corporate culture. What's important in the whole thing is that something is learned by the person who made the mistake; if the build breaks due to the same guy over and over again, there's a deeper issue with that person which must be addressed. Builds breaking all the time indicate an issue with the team's communication or knowledge of the process.

Answer 15

Yes. Ask the person to review the fix you made to the code. Sometimes I have found that someone else's bug was actually a tricky part of the code with some other unseen consequences if the bug was simply fixed.

Answer 16

There are a lot of factors at play.

• How severe is the bug?
• What's the seniority relationship between you and the breaker?
• How busy/stressed out is the team?
• Was the breaker working in their part of the codebase or yours?
• How certain are you that it was real a bug, and how certain are you that your fix is correct?

If the problem was minor - a typo/thinko/cut & paste bug - and the breaker is a busy peer, and you're confident in your assessment of the problem, you probably don't need to bring it to their attention. (e.g. foo.x = bar.x; foo.y = bar.y, foo.z = bar.y).

In most other cases, it's a good idea to mention the problem. In non-serious cases, you don't need to interrupt what they're doing; wait and do it over lunch or when running into them in the break room.

If the nature of the error indicates a serious misunderstanding (of the implementation platform, the local policies, or the project spec), though, bring it up ASAP.

If you aren't certain of your assessment, ask for them to review your fix, especially if it's not in code that you're very familiar with. (I strongly recommend your dev team adopt a 'code buddy' policy where all changes are reviewed by one other person prior to checkin, anyway.)

Answer 17

What happens if you don't tell them?

The cons

They may make the same mistake in other places because they don't understand that it is causing a problem. Not only that but there will be extra unnecessary time to repeatedly fix the same mistake. You can't learn from mistakes you are unaware you nade.

Second, they think they are doing a better job than they are. When people are not made aware of their problems they can hardly be blamed for thinking they are doing well when they aren't. Even when the problem is a careless mistake, people make fewer of them when they are aware that the mistakes are noticed.

Next if someone doesn't look up who did it, how will you know if you have a particular problem employee who is either always careless or has basic misunderstandings of the product? Would a responsible person want that to continue in a team he or she is associated on?

If you fix and move on without discussing it are you sure, you fixed it correctly? Sometimes it is tests that need to change when a requirement changes. If it is something other than a fairly minor typo, can you really be sure either one of you has the correct solution? You might be breaking his code in return without consulting.

The pros

People don't get embarrassed or annoyed with you for pointing out their mistakes.

I guess I come down strongly on the side of telling them but doing it nicely and privately. There is no need for public humiliation. If the person repeatedly makes the same mistakes or is making critical mistakes that show a lack of understanding, then the supervisor needs to be made aware as well.