Rebased because #6790 is merged. Also, resolve trivial merge conflict.

ACK

Why did this not pick up on the lack of indentation here?

@dcousens I did only clang-format recently added files to prevent creating merge conflicts and see if any updates are necessary with regard to the clang-format workflow.

You can find the set of files in the OP (hidden in the command line)

Please discuss and schedule this at the next IRC meeting.

Similar to the libconsensus reformats, there is no point creating a pull request for cosmetic, mechanical changes unless they are ready to be merged soon after creation.

If cosmetic changes will not be merged immediately, this creates a constant-rebase hell for the pull request in question.

Given that it takes mere minutes to re-create this from scratch, it is better to

• Schedule cosmetic changes in a time window
• Create pull request when that time window opens
• Harass maintainers to merge ASAP :)

@MarcoFalke is there perhaps a way we can put this under test? Making maintainers conform to a style that would otherwise be a breaking test is a great way to avoid discussion/bike shedding over this.

Perhaps something to bring up at the IRC meeting.

The general plan was to format files, then have an automated check for that set of files, e.g. either (a) auto-reformat those files for future changes or (b) reject a push unless those files comply with the format.

Option (b) is less prone to dangerous errors sneaking if a bug exists in the formatter, so I've typically opted for that personally.

next IRC meeting

In case this means http://www.wolframalpha.com/input/?i=15+utc+thursday #bitcoin-core-dev , I am already scheduled.

@MarcoFalke, @sipa mentioned it was at http://www.wolframalpha.com/input/?i=19+utc+thursday, is it tracked anywhere?

Ok, then. 19 utc works for me.

Tend towards NACK. My big gripe with changes like this is that it makes it much harder to find when actual code changes happened. Every time you try to use e.g. git-blame you end up with tons of "code beautification" to wade through. It makes git less useful as a maintainer. That these are recently added files makes it less of an issue, that's true...

@laanwj I think the sooner we can just put this under test, the bike-shedding and nitting will just disappear because the tests will spell it out, and it just becomes a non-issue.
Yes, it will make a few commits that may appear trivial [initially], but, in the interest of the long term readability of the code base, I think it would be worth it. YMMV.

If there is any discussion of this at the IRC meeting, it should probably be directly in response to #6839 (comment), not necessarily the PR itself.

I'm just not a big fan of this. We can also make bikeshedding and nitting go away by discouraging it in other ways, which don't involve mangling the entire codebase.

To be clear, my position is

• NAK, close pull request
• Schedule time window
• Re-create pull request from scratch when time window opens (concept ACK)

FTR, there is still a lack of consensus over how to structure these changes going forward [in terms of moving away from the current ad hoc nature].

A couple thoughts:

When changing existing code we must not just trust clang-format repeatability to verify that no bugs were introduced, we should test via checks for identical object files. This will take a little effort because some macros insert line numbers, and these will need to be bypassed.

To use clang format normativeily we must end up specifying a particular version that all contributors must use, as clang format changes its behavior. This is irritating, and disruptive to contributing. It could be made much better with good automation like cfield's depbuilder work, to help people get a particular version of clang-format; though due to the flowing API breakage in C++ code, I don't know how long an older version will be maintainable.

Another concern is which style to specify. The most common behavior in Bitcoin Core today has stylistic elements which have been objectively shown to increase software defect rate and confuse review-- e.g. unbraced multi-line IFs. We should not make any disruptive reformating changes unless we are quite confident that the style is what we want to live with.

We could avoid disrupting the workflow of existing patches by only reformatting (and then requiring it) on new files and substantial rewrites. I think that would be a better path.

To the extent that there is hope to address bikeshedding via this, I beg anyone thinking that to give up that thought. Bikeshedding is not a result of any particular coding style or consistency, bikeshedding arises from a lack of perspective which can only be addressed by experience, community norms, and having more important things to worry about.

Stylistic consistency has actual benefits; it aids newcomers in their contributions because it is easier for them to make sure their work is okay on styleistic grounds; though this may be offset by having to install some particular version of clang-format before they can get started. It eases review because the uniformity creates better expectations; but reformating makes looking at the history harder, which harms review. Good style choices (as opposed to merely consistent) have, at times, been shown to lower defect rates in software-- but there is not a universal opinion on what choices are good. So there are benefits to be had, but they must be carefully managed with other risks and costs. And the magnitude of the potential benefits, I think, are enough to say that this subject should have a very low priority in general.

@gmaxwell To use clang format normativeily we must end up specifying a particular version that all contributors must use

https://github.com/bitcoin/bitcoin/blob/master/contrib/devtools/clang-format.py#L15

@gmaxwell We could avoid disrupting the workflow of existing patches by only reformatting (and then requiring it) on new files and substantial rewrites. I think that would be a better path.

I agree, but I am not sure about the requirement. Imagine changes to critical code, refactor-only or just a new feature whose PR aims to produce a simple diff that is easy to read and to verify. Often clean diffs and proper formatting are orthogonal.

Entirely new files are the exception but they should ideally be formatted before submitting the PR (at least prior to merging).

I think, are enough to say that this subject should have a very low priority in general.

Yes, and with that, I'm going to close this.