meta: add clang-format formatting configuration

[martinpaljak]

This allows for voluntary formatting with clang-format (or
IDE tools like clang-format plugin for VS Code).

Once a suitable formatting configuration is found, automatic formatting
can be configured

This formatting is used to format new files in esteid-2018 branch.
Change-Id: I9021a98fe90c5ef9f21ce7459fdf05fa645fb56c

[LudovicRousseau]

Good idea.
I did not know clang-format tool.

[Jakuje]

I would vote for 100 maximum to fit two files on the reasonable-sized screen next to each other.

[martinpaljak]

Editors can wrap, but they hardly ever unwrap. Me for example like to glance at the structure of the file on a casual screen - thus as long lines as meaningful. There was also an option to preserve some of the breaks in the source as well (so that if clauses would maintain the breaks on || && operators for example)

In the end it would be good if there was a tool to run and not have any comments on PR-s regarding whitespace and indentation and related friction :)

I do feel that 80 and nearby numbers are IMHO historical lowest common denominators that could be forgotten - people have ultrawide screens (even though I also work on a 15" laptop or even smaller net terminal sometimes I would not define everything by these constraints)

[frankmorgner]

Who would have thought that formatting is an endless spring of discussion?

Let's start with something objective. The recommended formatting should match what's already used throughout most of the code. Currently, the line length is way below 140 lines for most of the code. I personally, think that 80 characters are great to force programmers to write shorter functions (that still fit into one screen).

Now something productive. The recommended formatting needs to be automatically forced onto pull request. There are some people using it in CI already, e.g. https://github.com/r-lib/usethis/pull/22/files.

[martinpaljak]

pre-commit hook?

[Jakuje]

Seems like this got a bit stalled from last year. I would really like to see the formatting a bit more formalized in both documentation and automation. I just ran simple test through the opensc repository to find that there is really many lines even longer than the 140 characters (mostly legacy code, tables/lists or generated cmdline).

$ grep '.\{140\}' `find -name "*.[c|h]"` | wc -l
315
$ grep '.\{120\}' `find -name "*.[c|h]"` | wc -l
703
$ grep '.\{110\}' `find -name "*.[c|h]"` | wc -l
1184
$ grep '.\{100\}' `find -name "*.[c|h]"` | wc -l
2225
$ grep '.\{80\}' `find -name "*.[c|h]"` | wc -l
8219

I think we all agree that we do not want to go as low as 80. I would like to see 100, but I can settle up with 120 as it overflows only few characters when displayed in two columns on my 24" screen. :)

[dengert]

Should // clang-format off and // clang-format on be added to current source to keep defines aligned as is suggested in AlignConsecutiveMacros ? opensc.h has a lot of them.

Are you considering having a separate PR with only formatting changes?

What version of clang-format are you proposing? I see there is a version 9. (Ubuntu-18.4 which I use has both 6 and 7.)

What about: DerivePointerAlignment and PointerAlignment?

[Jakuje]

Most of the code is really Attach, but I got used to Linux here, which I find more readable (opening brace of function starts on new line) and I already used it some of the code already. This is something probably not for a change, but maybe for consideration.

[Jakuje]

I think this would be changing the AfterFunction value before.

[Jakuje]

This is also one thing that I am not sure if we are all on the same page. I use tab width 4 in my editor, but I am fine to adjust on 8 if we will be on the same page here.

[Jakuje]

For the automation, we should not depend only on pre-commit hooks as this would mean that the users would have to enable them (if I remember well). It should run also on CI and hopefully only on the changed lines in the PR. I do not like the idea of a large commit changing all the code to this strict formatting.

With git-clang-format and clang-format I am still getting a lot of differences (for example since last release):

$ git-clang-format --diff --commit 0.20.0

I agree that we should use also AlignConsecutiveMacros: true.

One more question would go to the generated code (mostly src/tools/*cmdline.{c,h}. We should probably be able to exclude them either with comment proposed by @dengert or somewhere in the CI configuration.

I tried the following so far (I think we should really check only the diff of the changes in the PR, not the whole codebase in PRs):

• Using github actions is significantly faster than travis and a bit more closer to the eyes of contributors (not that deep in the log): Test github actions Jakuje/OpenSC#1 (works, but might need some tweaking to figure out upstream/master)
• Something similar can be glued into the travis too, but the log is very deep (it should be able to propose changes though): Introduce clang-format as part of CI using Github Actions #2017 (does not work yet)

Additionally, these formatting recommendation should be covered in contribution guide in human readable form. I think I can take care of that after we will settle down on some,

[frankmorgner]

I've left this open, because I didn't have the time to review all the possible settings. The formatting should be as close as possible to what most of the code uses. Thanks @Jakuje, for looking closely into this. I'm trusting your judgement on this. (I prefer using 8 spaces for a tab 😉)

I have no idea how to resolve limiting the formatting only to the changes. The OpenSSL team solved this by creating one big commit that fixes all the formatting...

Travis CI may offer more control over what to check. I haven't checked it into depth, but https://github.com/Sarcasm/run-clang-format looks like a suitable option, which also allows excluding stuff.

Maybe integration of access tokens and visual output will become easier with Github Actions one day.

[Jakuje]

Please, see the Jakuje#1 -- It already demonstrates how to use github actions to check only changed code (with some clever context matching) in the PR commits. It has advantage that it runs in matter of seconds after the push, rather than minutes as Travis because the actions are in container.

It has also some failed attempts to do same thing in travis but last issue that I hit was the old clang so I would prefer going through the github actions.

I will keep that branch for reference without fixing the issues yet.

[dengert]

I would like OpenSC to consider using a feature of git 2.23 "--ignore-rev " to ignore formatting only commits when using git blame so one can see where the lines actually came from.
"blame.ignoreRevsFile config option" can point to a file with the list of these commits.

See: git 2.23.0-RelNotes and a blog: ignoring-bulk-change-commits-with-git-blame

We can take advantage of this today, so when developers start using git 2.23 OpenSC will already have a list of the formatting only commits.

I would suggest, that we do one big commit, or groups of commits that are formatting only then have developers rebase on top, and start using the new format.

Also see #1987 (comment)

[martinpaljak]

• GH actions is a great tool (Sorry, Travis and Jenkins, times have changed).
• there should be a way to format in pre-commit or equivalent (so that the test in github actions would have 99% chance of not triggering)
• I'm not really picky about coding standards (where the curly braces are after a function) as long as there is a tool to convert code to the canonical form, but there is one thing I dislike and that is breaking my "carefully crafted" long lines. IMHO long lines themselves should be there as a warning that "this probably is not the right/best approach" but whatever the arrangement, it should be kept. I have not looked deep, but I have not found an option to convert wrapped source to full length lines, so that I could see them in their beauty on my not-80-column screen. AFAIK all editors do a decent job with wrapping too long lines into view, if necessary.

[Jakuje]

Thank you for the update on the --ignore-rev. I did not know that. If there is this possibility, if this would work, it would be nicer to convert everything at once. But first figure out minor details. I already ran it through the #1987 which already triggered some questions that I will try to answer and possibly adjust the configuration.

The issue with too long lines allowed is that formatter will try to reflow the code if it was manually formatted to shorter line lengths for better readabilityeven if it is less readable. See for example https://gist.github.com/Jakuje/bd2317af7c4101f7e49c5d2686b414fd#file-cardos-5-3-patch-L550

[Jakuje]

(sorry for the close -- bad touchpad)

[dengert]

It is not only "carefully crafted" long lines, but "carefully crafted" short line converted to hard to read long lines. I ran clang-client-7 on card-cardos.c in master. These are some of the changes it made:

  68 -               if ((atr[4] != 0xff && atr[4] != 0x02) ||
  69 -                   (atr[6] != 0x10 && atr[6] != 0x0a) ||
  70 -                   (atr[9] != 0x55 && atr[9] != 0x58))
  71 +               if ((atr[4] != 0xff && atr[4] != 0x02) || (atr[6] != 0x10 && atr[6] != 0x0a) || (atr[9] != 0x55 && atr[9] != 0x58))

 166 -       } else if (card->type == SC_CARD_TYPE_CARDOS_M4_3 
 167 -               || card->type == SC_CARD_TYPE_CARDOS_M4_2B
 168 -               || card->type == SC_CARD_TYPE_CARDOS_M4_2C
 169 -               || card->type == SC_CARD_TYPE_CARDOS_M4_4
 170 -               || card->type == SC_CARD_TYPE_CARDOS_V5_0) {
 171 +       } else if (card->type == SC_CARD_TYPE_CARDOS_M4_3 || card->type == SC_CARD_TYPE_CARDOS_M4_2B
 172 +                  || card->type == SC_CARD_TYPE_CARDOS_M4_2C || card->type == SC_CARD_TYPE_CARDOS_M4_4
 173 +                  || card->type == SC_CARD_TYPE_CARDOS_V5_0) {
 174                 rsa_2048 = 1;

And when considering a terminal size, consider the "Write" and "Preview" windows used to type this comment and the window you are reading this comment. They appear to be 110 characters per line.

[dengert]

@Jakuje We are talking about the same hand crafted lines in card-cardos.c

[frankmorgner]

I think line break conditions can be tuned quite a bit...

[dengert]

As I test to see what would be needed to do a mass conversion and how git blame 2.26 would work:
See BIG problem below.

• created new branch.
• git config --add blame.ignoreRevsFile .ignoreRevsFile to my .git/config If not done or on older versions, git blame would continue to work as before.
• committed a blank .ignoreRevsFile to top level OpenSC dir
• committed a .clang-format to top level OpenSC dir, but maybe should be in src.
• cd src
• find . -type d -name .svn -prune , -name .git -prune , -type f -name \*.[hc] -exec clang-format-7 --verbose -i {} \; -print | tee /tmp/mass.txt
  took about 10 minutes and changed 310 files
• committed the mass changed commit id xxxxxx
• git blame card-cardos.c | grep xxxxxx | wc showed 254 lines attributed to the mass change commit
• updated .ignoreRevsFile with the commit id xxxxxx full sha1.
• committed .ignoreRevsFile
• git blame card-cardos.c | xxxxxx | wc showed 14 lines attributed to the mass change. All were blank lines added.

PROBLEM:
The conversion of scconf/parse.c caused two #include" lines to be switched which cased sconf/parse.c` to not compile.

Looking at other files, it looks like clang-format is reordering #include statements files in alphabetical order!

diff --git a/src/scconf/parse.c b/src/scconf/parse.c
index cd45f508..da83f7b4 100644
--- a/src/scconf/parse.c
+++ b/src/scconf/parse.c
@@ -30,14 +30,15 @@
 #include <errno.h>
 
 #include "common/compat_strlcpy.h"
-#include "internal.h"
 #include "scconf.h"
+#include "internal.h"
[...]

[dengert]

Setting .clang-format SortIncludes: false fixed the include problem.
BreakBeforeBraces: Custom is need to get any of the BraceWrapping: options to work. which includes AfterFunction: true

[frankmorgner]

I think that all header files should be self containing. If scconf.h has some dependency to internal.h, it should include that file. The order should not matter in this case. (Sure, there are some exceptions to this principle, but here we have none of those)

[frankmorgner]

I've pushed 4e9cec1 for fixing the includes.

[Jakuje]

The issue you describe in #1641 (comment) is caused by BreakBeforeBinaryOperators NonAssignment option. If it would be on me, I would keep the LLVM default None.

To avoid endless discussion about line lengths (and clang reformatting already formatted code), we can use ColumnLimit 0, which will not explicitly limit and reflow the code (at least for the initial reformat?). But it has a side effect of creating some very very long lines.

Furthermore I updated the .clang-format to skip defaults from LLVM so it is clear what is LLVM and what are differences from there.

Another pain point is a lot of struct arrays aligned like this:

               { SC_ALGORITHM_RSA_PAD_PKCS1,      "pkcs1"     },
               { SC_ALGORITHM_RSA_PAD_ANSI,       "ansi"      },

Clang format can not keep this format as described here. So before doing the mass-reformat, we should identify these array structs to skip formatting using // clang-format off/on (the space between // and clang-format is needed!).

Another common case for reformat is ContinuationIndentWidth. We have usually used just a one tab for this, but LLVM default has 4 spaces, therefore most of the structs/enum initializers are reformatted with this. Therefore I believe we should use UseTab: ForContinuationAndIndentation too.

The updated .clang-format is available in my fork pr: Jakuje#1 (I don't have updated master so there is whole bunch of other commits in that branch, but nice for demonstration of what is it going to change). At this moment, I have 189 thousands lines long patch for formatting (clang 9):

$ git diff | wc -l
189461

Some of them are helpful, some of them less, but I am not sure if we would be able to review this large changeset to filter out only the helpful parts. Having all of the formatting changes in one commit would be good, but we could try to do it iteratively per files or per changes, which will hopefully be more digestible, reviewable and in the end, we could end up with more commits in .ignoreRevsFile.

For changes going in, we should probably exclude the generated code in *-cmdline.c, which has completely different formatting already and would go eventually back after rerunning the gengetopts.

In the above PR it already suggest in comments fixes for the related code (would be good if it would go from some bot account).

[dengert]

Is ".ignoreRevsFile" going to be added via a PR. Looks like blank lines and lines starting with "#" are comments.

Do we plan on doing this before or after next release?

I would suggest after, as there are a number of fixes that really need to be in next release ASAP, and we don't need to introduce any new bugs that might be caused by the formating.

This will also then allow all developers to rebase on the next release, and any new commits can then check for the formating. This we would have only have one or a few .ignoreRevsFile and may not need any others.

I added // clang-format on|off to card-piv.c and pkcs15-piv.c in https://github.com/dengert/OpenSC/pull/new/PIV-4-extensions
It's a work in progress, and won't compile, as I see a bug but have a look if you want.

I can create a version for the current master.

[Jakuje]

I am fine doing the whole reformat after next release. We still have many questions to answer about how to do that, how formatting will look like and how the CI checks should look like.

For the formatting, I would like everyone who has at least some opinion about formatting, to have a look on my revisited .clang-format, which

• is based on the one from @martinpaljak here:
• is more readable as it does not contain default values from parent LLVM
• addresses some of the concerns raised discussed in this thread to follow our intentions and existing code base

https://github.com/Jakuje/OpenSC/pull/1/files#diff-69a68f7ce1ea3687a38d8e4705e6fa48

[Jakuje]

I did a bit archeology and found the following page describing the coding style:

https://github.com/OpenSC/OpenSC/wiki/Development-Policy

Trying to follow JOINT STRIKE FIGHTER AIR VEHICLE C++ CODING STANDARDS, but I hardly find any intersection with existing code. That page will need to be updated too to match what is discussed here.

Anyway, I did not get much feedback here nor in #2017 .

The github actions look promissing, simple and fast.

On the other hand, I like to see formatting suggestions as visible in Jakuje#1, but that would need separate "bot" account to do these comments.

So unless there would be someone strictly against, I would merge changes to .clang-format and Github actions and some formatting hints (keeping the travis version out). This will allow us to try and tweak the formatting config before we will reformat the whole codebase to avoid more reformatting commits later.

[frankmorgner]

done with #2977