views:

559

answers:

13

There are minor coding style changes that I often want to commit to the source control, but now the change log is full of those changes that do not affect code functionality.

What should I do next time I have to fix minor things like:

  • Remove and sort usings (in .NET, imports in python, includes in c++)
  • Correct indentation, spacing and line breaks
A: 

Are these "minor" things fixes? If yes, commit them. If no, don't.

Really, it depends on what you and your team consider important.

A: 

Commit them with the next big change as sidenote. At least that's what I'd do.

Bobby
-1 - that'll make working out what's actually changed in your big change impossible. Whitespace/cosmetic changes should always be separate, so actual changes are easier to see.
Dominic Rodger
But then perhaps merging a simple bugfix to another branch, a released version, might gets noisy because the changeset contains lots of unrelated changes.
Lasse V. Karlsen
@Dominic, Lasse: You guys are right...I totally missed that there's also the changeset for each commit, which would be crowded with the whitespace changes...sorry.
Bobby
+17  A: 

If you are changing the code file, I don't really see why you wouldn't want to commit and share those changes. If you don't you run the risk that someone else will fix them and then collide with yours.

If they aren't changes that other users want in the codebase, perhaps you should ask yourself why you are spending time writing them.

Alex Brown
Good point . . .
Jader Dias
And if you're not just going to annoy everyone with endless whitespace/cosmetic diffs.
Dominic Rodger
@Dominic now you are saying that I shouldn't..
Jader Dias
No - I frequently submit whitespace/cosmetic changes, and tend to hope I'm not annoying people.
Dominic Rodger
Since you've got version control, if someone doesn't like your cosmetic changes, they can revert them out. No big deal.
Mike DeSimone
@Mike - yes, but since a lot of people get notified every time I submit changes, these changes can end up creating a lot of noise.
Dominic Rodger
+9  A: 

Don't commit them together with unrelated fixes.

I would commit them, but add some predefined keyword to the commit message. Messages with this keyword could then be ignored when generating change logs.

You could use a prefix like [cleanup] for instance.

[cleanup] Removed some whitespace
[cleanup] Changed format
Fixed some major bug.
[cleanup] Corrected indentation
Peter Lang
+4  A: 

On projects where I'm the only developer I tend to do these kind of fix ups along with other code changes.

On projects where there's a team of us I tend to try and commit these kind of changes on their own so that they don't obscure the 'real work'.

I feel that it's important to fix everything that's 'wrong' with a codebase even if it's purely minor things like indentation.

Len Holgate
+12  A: 

Do commit them, with the commit comment flagged appropriately to make it easier to ignore when skimming through a list of changes.

Don't commit them in the same operation as a change to functionality. That way, if you do break something, it is easier to narrow down what broke it and is easy to revert just the refactoring if necessary.

moonshadow
This is really important. Commiting whitespace changes together with functional changes is dangerous. It makes it hard for people to figure out what really changed.And, like everybody else said, make sure the cosmetic changes are easily recognisable from their commit messages. (You do use commit messages on every commit, right?)
mcv
A: 

i like to commit often. Certainly any time there's an appreciable change. It's easy that way. If you start to compartmentalize code snippets and try and commit some sooner rather than later you will eventually forget to commit something very important.

In short: Commit often and ALWAYS document the change. When there's a HUGE change, tag it.

Paul Sasik
+4  A: 

I think this depends on your work environment and how others working on the same project want to deal with that which is likely to differ.

So, my general suggestion would be to ask the people working with the same code and coming up with a guideline for cases like that. You might find that people don't mind check-ins due to cosmetic changes or that they would rather live with a bit of "unprettiness" rather than dealing with cluttered change logs.

A definitive guideline that is transparent to everyone is the best way to deal with these questions and avoid confusion in the future.

Personally, I like tidied up code and wouldn't mind check-ins due to purely cosmetic changes. However, if it is just a bit of spacing and line breaks, I would probably just let it be and only change it if I was working on the same code file anyway. I often remove and sort usings because I find it confusing if there are a whole bunch of usings that don't make sense, but that's just me.

Anne Schuessler
+2  A: 

Definitely commit them. If you commit them along with real code changes and you have to roll back those changes, then you lose your cosmetic fixes.

Ideally, commits should be like database transactions: a chunk of related working code that can be rolled back without affecting the rest of the system.

Jamie Ide
+3  A: 

There's a couple of issues.

First, don't do changes to the code because you're bored and doesn't have enough real tasks. If this is the case, go talk to your project manager and get some real tasks assigned to you, something with value.

In other words, don't go changing the code for the sake of the change. Always add some value to the code in the process.

Now, if those changes are contributing to making the code easier to handle, by you, and others, then do them. Things like ensuring naming standards are followed, refactor crufty code, etc. But get a task for it, so that your project manager can say "Yes, this is good, spend 2 hours on this and get back to me."

Commit the changes when you're done with them. Don't lop them together with whatever real task you finished just before them, or the next one, it will make merging bugfixes between branches, code reviews, and just general code browsing, hard to follow.

"Ok, so you fixed bug 7711, and also changed about 100 other files. Nice, so what is actually the bugfix here?"

Lasse V. Karlsen
+2  A: 

I think when you’ve got a team of developers working on the same code, the most important thing is to agree a cosmetic style for code. So, your first task is to try to get your entire team to agree on a coding style.

Good luck.

Once you’ve done that, commit cosmetic changes as often as you want, to remind people to stick to the style.

There’s a great section in Code Complete about the merits of different coding styles. If you can get your team to read the section before your coding style meeting, it might help to get you all out of the meeting alive focus the discussion.

Paul D. Waite
+1  A: 

If the changes are to things which might be in any way controversial (position of brackets for example) then make sure you've agreed a code style with the rest of your team. Don't just change it to your own preferred style then check it in. Otherwise someone else might then change it back and check their changes in, then you change it back to your way...

MTEd
A: 

Don't commit just for the sake of commiting. I usually add those for the code I'm working on. For example if I'm fixing a bug in method A I make sure to also make all cosmetic changes as well.

IMO you guys are missing coding tools like PMD, JIndent, etc that take care of this issues as you code. Some IDE's like Netebeans show this "issues" as warnings. So its not a random/personal change is following standards.

javydreamercsw