asclines commented 7 years ago

Okay guys, yes the semester is coming to a close, but that is no excuse for laziness in our work. While I will only be talking about one commit from one pull-request, there may be others out there. Also, this should merely serve as an example for future problems so we can prevent them.

It seems that as this semester has gone on, and the end is approaching, we may be losing our focus a little bit, letting our guard down, and in doing so,

WE messed up.

I will be discussing Pull Request #126 Updates to filler_strategy.

Let's start with merging.

Merging

For a refresher: Documentation for git-merge

For those who don't want to read the documentation, want an easier way to understand it, or for some reason haven't learned how git merging/branching works, Atlassian provides a nice, easy to read, tutorial on it.

tl;dr No. Go read at least one of those articles. I'm not explaining how merging works here.

To quote Atlassian "When you encounter a merge conflict, running the git status command shows you which files need to be resolved."

This was clearly not done after commit 2cd633ae261da0a3b41dbc8eff2a461d19aab290 . As there is now a file, titled test_filler_strategy.py~HEAD. This is the result of their being merge conflicts after a git-merge.

The conflicts should have been resolved and then pushed. In fact, in the commit message that is generated by git-merge, it even lists the file by name!

Instead, for whatever reason, there were not, resulting in commit 9d899b70abd4d6f5d40dbe9e3efb832dbb45c372 titled "stupid master things". I am assuming that this commit is the result of the author later realizing the merge conflict issues and attempting to fix them. I hope they were fixed. But still, test_filler_strategy.py~HEAD, has not been deleted. I honestly do not understand why this file still exists but nevertheless, it exists and we need to learn from this.

The lesson that I believe we should get from this is:

Check every file that you commit and push to a branch.

This is public stuff, with your name attached, that others are relying on. Please check the files that you are pushing. This is where git tools can screw things up. It's way to easy to "select all" and push. But seriously, check your files, before you wreck your files.

Reviewing

It's one thing that these problems happened, were committed and then were pushed to a feature branch. Its an entirely other thing that they made it into the master branch. And that is where WE really messed up.

The third point under Pull Request Process in our Contributing Guide states:

"You may merge the Pull Request in once you have the sign-off of two other developers, or if you do not have permission to do that, you may request the second reviewer to merge it for you."

Pull request #126 was only reviewed by one person.

We did not hold up to our own standards. A second person should have signed off. I really would like to think that after the author and two other developers had gone through the PR, that somebody would have noticed the file.

Takeaways

In the future, we need to really pay attention when we handle merging and we need to be constant in our diligence in reviewing each other's code. I do not believe that this time, any serious harm was caused, but when it comes to merging and pulling into master, there is always a possibility of seriously breaking things.

isaac-gs commented 7 years ago

Yeah this is a really good point and I definitely should have noticed that file sneaking in. I think for me two takeaways are,

Never approve something until commits are finished.
If something is approved and can be merged, just merge it rather than adding more stuff.

ZakeryFyke commented 7 years ago

Yeah, my mistake. I've checked the file and it was empty, and I'm fairly certain this was either a superficial merge error or something I fixed in a later commit. But agreed, I'll make sure to watch out for things like this in the future.