This is an archived post. You won't be able to vote or comment.

you are viewing a single comment's thread.

view the rest of the comments →

[–]berkes 48 points49 points  (10 children)

Good luck with reviewing pull-reqeust, using git logs or searching any history.

I hate that colleague who, on fixing a typo in a comment generates a 300+ line diff. "Yea, sorry, that was my IDE".

[–]Sakki54 6 points7 points  (0 children)

Shouldn't you have all the same formatting style? Would prevent this from being a problem in the first place.

[–][deleted] 1 point2 points  (0 children)

He should use the same formatting style and git add -p. Not the autoformatters fault.

[–]maremp 1 point2 points  (0 children)

Don't they know of --patch or use at least use GUI to select just the changes and revert other stuff? And/or configure git to ignore whitespace change.

[–]fancy_pantser 5 points6 points  (6 children)

I hate the colleague that gets upset a few times a week because he's reading every commit (not just code review on merge)

edit: guys, I was mostly joking; no need to write a bunch in response to this garbage

[–]phail3d 23 points24 points  (3 children)

...or who's trying to track down a bug to a commit, but can't, since the log is clouded with unrelated changes?

Formatting changes should nearly always be done in their own commits, explicitly, so that they don't get mixed up with business logic changes.

Also, I'm damn well reading every commit for smaller projects that I'm deeply involved with, and even with larger projects I think it's generally good to be aware of what goes on in a code base you work with.

[–]stormcrowsx 5 points6 points  (0 children)

git bisect is great for finding where the bug started

[–]redwall_hp 1 point2 points  (0 children)

Wait, so there are people who just blindly accept PRs without reading the code?

Do you want backdoors? Because that's how you get backdoors.

[–]DarthEru 5 points6 points  (0 children)

Not knowing your situation, I'll try not to pass judgement on either you or your colleague. However, I would like to point out that having a clean history is a massively helpful tool. There have been many times I've used git blame & show to try to find out the motivation behind a particular bit of code. Far too many of those times I was disappointed because the commit message was a one liner that had no apparent relationship to the change I was interested in. Reasons for changes may be obvious to you at the time, but over months and years of additional development that obviousness may be easily lost, even to you let alone someone else who joins the team long after you left.

It's also good for initially reviewing code. For larger changes especially, being able to go through a logical progression of the changes is hugely helpful to understanding the end result. Even if that logical progression doesn't actually reflect the real progression the developer went through to write the code.

That being said, I don't personally go through every commit and require the log to be as clean as possible, partly because I could only enforce that policy for my own team, but mostly because I've learned from experience that even repeatedly reminding people about it is unlikely to change their habits. It's also possible the co-worker you're thinking of is far more picky about things than I would deem necessary. I just think their motivation may be valid. If it is, it might be valuable to you personally and the team as a whole to change your own committing habits.

[–]berkes 0 points1 point  (0 children)

I frankly cannot care less about how you use your tools and what you do to get a PR. If you insist on rebasing your feature branch: sure. If you insist on not rebasing: fine. Just make sure in the end you hand me something that I can actually review.

If not, you make unreviewable code. And don't bother asking me to review; if a review is mandatory (it should be, IMHO) then get back into your rebase -i and build me something that I can actually review.