Code practices

[MerlijnWajer]

I would love it if we can standardize on one setting.

Personally, I often use indent -linux on all my projects. I am fine with another standard too.

We could run this command pre-commit and all C, C++ source and header files.

Every existing repository, we have to do one cleanup commit, that we can ignore with git blame, to still keep the right blame lines.

So:

1. Any other downsides to doing this ? (Maybe git revert won't work as well)
2. Are we OK with indent -linux ? - Or would we like another standard.

I don't suggest we do this to all the packages that we work on right now, but maybe we can try it out with one now.

[MerlijnWajer]

cc @spinal84 @freemangordon @parazyd

[spinal84]

Is it supposed that git blame will ignore that commit automatically? If yes, I'm fine. Not sure if indent -linux would be acceptable in our situation.

Do you have a demo repository with your proposal implemented?

[MerlijnWajer]

No, git blame will not ignore it automatically. There is no way to do that.

Feel free to run indent -linux yourself and see if you like the result.

I can set up a demo, yeah.

[parazyd]

ACK

It is also possible to rewrite the entire git history and reindent while keeping the correct git-blame. There should be some examples on the Internet.

[spinal84]

No, git blame will not ignore it automatically. There is no way to do that.

If we can't automate this, I'd rather say "no" to this proposal. It will be useless complication to "git blame" command which is used pretty often. When you develop something it's undesirable to be distracted by something that's not directly connected to the issue you are trying to fix. And the need to ignore such “magic” commits is likely to distract developers. Not to mention extra time.

[MerlijnWajer]

I am constantly and immensely distracted by the complete mix of honestly garbage code indentation and formatting in the repos. It costs me a lot of time to fix mixed tabs and spaces left by others long before any of us worked on this project. Is it really a problem to type something like git blame --ignore-revs-file .gitblameignore ?

[MerlijnWajer]

Plus, if we end up fixing indentation manually, that also pollutes the git-blame history, so it's even less clean in a way.

The only real reason I can think of for not doing is it that it may make it harder to revert commits, or cherry-pick code from specific sources. Like, we clearly won't do it for gtk+.

[spinal84]

I just checked git-blame man page and found the "-w" option. Isn't that what you want?

       -w
           Ignore whitespace when comparing the parent’s version and the
           child’s to find where the lines came from.

[spinal84]

I agree with your proposal. git blame -w -M -C is my new friend. Thanks for the direction!

[MerlijnWajer]

That's useful. One catch: indent -linux will also move function start/end brackets if they aren't on the right line, so the whitespace-flag won't be sufficient there. Are you against a standard file, standard filename, with a list of git commit revs to ignore?

[spinal84]

Could you please make a demo repository with such commit(s), so we could see how your proposal works in practice?

[freemangordon]

TBH I am not sure 'indent -linux' is best coding style for the source code we maintain. Indentation more than 2 spaces is an overkill IMO. Also (again, my own preference), but having curly brackets on the same line as if(), while(), etc, makes code less readable. I just find kernel coding style wasting too much of the space in a row.

An example of the coding style I am used to is (randomly taken) https://github.com/maemo-leste/connui-cellular/blob/master/wizard/gprs.c. This is the coding style I use in my job too, and no, I am too old to change it already :)

What I think we have problem with is not a particular coding style, but a lack of such or a mixture of several coding styles. So I think a better approach would be - instead of applying a particular coding style en masse, lets everyone creating a fix (and those reviewing) make sure they stick to the coding style of the file they are fixing. And if one hits a file that worth code style fixing, to do it in a separate commit (named something like "foo.c: Fix coding style", before the real code change she is on) , by using either the prevailing coding style for the project/package or her preferred coding style.

[dderby]

Plus, if we end up fixing indentation manually, that also pollutes the git-blame history, so it's even less clean in a way.

The only real reason I can think of for not doing is it that it may make it harder to revert commits, or cherry-pick code from specific sources. Like, we clearly won't do it for gtk+.

Speaking of gtk+, https://github.com/maemo-leste/gtk/ is missing the entire history from before it was imported. Any particular reason it was imported like this? Does this GTK come from Nokia or upstream? It's not very easy to tell without the history.

[MerlijnWajer]

TBH I am not sure 'indent -linux' is best coding style for the source code we maintain. Indentation more than 2 spaces is an overkill IMO. Also (again, my own preference), but having curly brackets on the same line as if(), while(), etc, makes code less readable. I just find kernel coding style wasting too much of the space in a row.

I always use four spaces but don't mind using two spaces. Keep in mind, if/when we decide on a standard for a repo, we can just run indent -whatever-flags-you-want pre-commit. So I can even work in my preferred four spaces format, pre-commit run indent to have it be whatever it actually is in the repo, and there will be no problem. No history problem, everyone happy.

Please fiddle a bit with indent flags and come up with something that works for you.

An example of the coding style I am used to is (randomly taken) https://github.com/maemo-leste/connui-cellular/blob/master/wizard/gprs.c. This is the coding style I use in my job too, and no, I am too old to change it already :)

What I think we have problem with is not a particular coding style, but a lack of such or a mixture of several coding styles. So I think a better approach would be - instead of applying a particular coding style en masse, lets everyone creating a fix (and those reviewing) make sure they stick to the coding style of the file they are fixing. And if one hits a file that worth code style fixing, to do it in a separate commit (named something like "foo.c: Fix coding style", before the real code change she is on) , by using either the prevailing coding style for the project/package or her preferred coding style.

I am fine with different standards for a repository, as long as:

1. It's consistent in the repo
2. Consistency changes are easily ignored in git-blame
3. The format is documented and enforced, preferably with a pre-commit hook.

This way I can always run indent -linux on a local repo, and before I commit/push, indent it back to whatever it needs to be.

[MerlijnWajer]

Did it for the PDF viewer:

https://github.com/maemo-leste/osso-pdf-viewer/commit/17f9b8459dee044365e1f972ad99dad25149ea77 https://github.com/maemo-leste/osso-pdf-viewer/commit/7ba681b45b0cd79b64ef4d997707c58d165629d1

[spinal84]

The initial formatting of the function header was better.

static void
pdf_mime_open_cb(ComappSystemData * cas)

is better that this: static void pdf_mime_open_cb(ComappSystemData * cas)

It's because your choice makes function title line longer and if there's a complex return type and/or long function name with long argument names / long type of arguments names, you will have to think how to not go past 80 chars on a single line if you want a nice function header with indentations.

[MerlijnWajer]

All of the indentation is done by tools, so you just need to configure the tools to fit your liking. Again, I just used indent -linux and astyle without arguments. They support a lot of arguments.

[spinal84]

Sometimes, it may look like this: static ConboyActionContainer *conboy_note_window_get_action_container (ConboyNoteWindow *window) This line is 97 chars wide.

[spinal84]

... compare to:

static ConboyActionContainer *
conboy_note_window_get_action_container (ConboyNoteWindow *window)
{
  ...

[spinal84]

All of the indentation is done by tools, so you just need to configure the tools to fit your liking. Again, I just used indent -linux and astyle without arguments. They support a lot of arguments.

Good to know.

[MerlijnWajer]

Like I said, I have no particular strong opinions on what arguments to use for these tools, I just don't want to spend my time manually cleaning up code indentation issues that I didn't even cause in the first place.

And of course, since it can be easily automated, one can run indent with whatever arguments he/she prefers after git pull, write code, and before commit, format back to whatever the repo uses.

[spinal84]

I suggest to pick something that we could use as a good standard (or several standards) for our repos. Maybe a note should be added to HACKING file in the sources. With the tips on how to setup a working environment for a particular package and what indent command should be used there to have a nice result.

[IMbackK]

ACK on indent -linux

but i think 80 cols is a bit to extreme on modern displays and 120 col limit is more sane. even the kernel codeing style has been changed to 120 lines so i would suggest indent -linux -l 120

[parazyd]

ACK on indent -linux -l 120

Another argument is that lots of our code often exceeds 80 cols because of the "object-oriented" glib.

[MerlijnWajer]

I am OK with following kernel practices