Skip to content

WIP: added .editorconfig file, applied uncrustify to make indentation uniform - #12

Draft
seanm wants to merge 4 commits into
InsightSoftwareConsortium:masterfrom
seanm:editorconfig
Draft

seanm wants to merge 4 commits into
InsightSoftwareConsortium:masterfrom
seanm:editorconfig

Conversation

@seanm

@seanm seanm commented Feb 13, 2025

Copy link
Copy Markdown
Collaborator

No description provided.

@hjmjohnson

Copy link
Copy Markdown
Member

@seanm If we are going to diverge from the main nifti_c lib, I would propose making as many changes as necessary in one commit.

I prefer to use clang format. I have had better luck with integration into my editor workflows. Additionally, we could keep in sync with ITK and reduce the overall maintenance headaches.

@seanm

seanm commented Feb 16, 2025

Copy link
Copy Markdown
Collaborator Author

I guess I thought at least making the indentation uniform would make it easier to work on the codebase, because currently it's a pain since touching different parts requires using different indentation, which my editor does not like.

I'd also like to use clang-format eventually.

I could give it another try, but even after using whatstyle clang-format was making many massive changes...

@seanm
seanm marked this pull request as draft July 31, 2026 19:54
@gdevenyi

gdevenyi commented Aug 3, 2026

Copy link
Copy Markdown

I think what @hjmjohnson is saying is we should merge all the "formatting cleanup" into a single bulk PR so we can ignore it for blame purposes (this would also be a place to setup a clang-format hook and github action to block merging any format fails.

@seanm

seanm commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator Author

I think what @hjmjohnson is saying is we should merge all the "formatting cleanup" into a single bulk PR so we can ignore it for blame purposes

Ignoring things in blame doesn't require one big commit though, you can put as many SHAs as you want in a .git-blame-ignore-revs file.

@gdevenyi

gdevenyi commented Aug 15, 2026 •

Copy link
Copy Markdown
Closed PR comment

#26 does what was asked for here, so this can probably be closed.

@hjmjohnson's point above — clang-format rather than uncrustify, to stay in sync with ITK — is what it implements: ITK's .clang-format verbatim with six overrides, each documented in the file with the reason. @gdevenyi's point about one bulk commit for blame purposes is handled too: the reformat is a single commit and .git-blame-ignore-revs lists it.

@seanm, on your comment that "even after using whatstyle clang-format was making many massive changes" — it does, and there is no configuration that avoids it. I measured LLVM, GNU, Mozilla and ITK styles: all of them rewrite roughly 56k lines out of 53k, because the tree carries five different indent widths and any consistent style touches nearly every line. The useful part is not shrinking the diff but making it checkable, so it is verified by comparing object files rather than by review: 14 of 16 are byte-identical before and after.

This branch is 18 months stale and CONFLICTING against master, so rebasing it would be more work than re-running the formatter. What is worth keeping from it is the .editorconfig and the .git-blame-ignore-revs scaffolding, both of which #26 carries forward — the .editorconfig with indent_size = 2 to match ITK's IndentWidth rather than the 3 here.

@hjmjohnson

Copy link
Copy Markdown
Member

The three config files here are worth having. The uncrustify run that comes with them is what makes this hard to land, and I think the two should be separated.

Recommendation: keep .editorconfig, .git-blame-ignore-revs, and uncrustify.cfg (54 lines, no code touched); drop or defer the reformat (2,458 lines).

What the diff actually consists of

Measured against the merge-base, counting only files that already existed:

lines
added 2,472
of those, changing code 14
of those, changing only whitespace 2,458 (99%)

git diff -w reduces this PR to its three new config files plus fourteen lines. nifti1_io.c alone is rewritten across 2,941 lines with no functional change.

Why the reformat half is expensive right now

It rewrites 8 source files in niftilib/. Fifteen of the thirty-two other open PRs touch those same files:

#21 #22 #24 #38 #43 #44 #47 #48 #49 #50 #52 #53 #61 #67 #74

Landing the reformat means every one of them has to be rebased through a whole-file reindentation, where git's rename and content detection is least reliable and a silent mis-resolution is most likely. The backlog is mostly confirmed bug fixes; paying that cost to land a change with no functional effect inverts the priority.

It is also niftilib/ only. nifti2/, nifticdf/, znzlib/, fsliolib/, and cifti/ keep their current formatting, so the tree ends up less internally consistent than it started, not more.

The config files stand on their own

.editorconfig records the convention the code already follows — I checked, and indent_size = 3 with spaces matches the existing sources. Committing it stops new contributions drifting, which is most of the benefit here and costs no churn at all.

.git-blame-ignore-revs is the right mechanism, and it is worth landing before any reformat rather than with it, so the infrastructure is in place whenever the reformat happens.

If the reformat is still wanted afterward, the version that is cheap to review is one that runs uncrustify over the whole tree in a single commit, once the bug backlog has drained, with that commit's SHA added to .git-blame-ignore-revs in the same PR.

@hjmjohnson

Copy link
Copy Markdown
Member

Commit messages cleaned up in place (prefixes added, WIP: dropped from the subject). Deliberately not rebased — and the earlier recommendation to split this PR now holds more strongly than when it was made.

Recommendation, unchanged: land the three config commits, defer the reformat.

Why no rebase

master's last 44 commits were rewritten on 2026-09-22, so every open PR shows CONFLICTING and needs a rebase. This one is the exception. 0cec098 (Apply uncrustify to niftilib) rewrites 2,463 lines across 8 files that master has been changing continuously; rebasing it means hand-resolving a conflict in nearly every hunk, and the result would be invalidated by the next merge to master anyway. The rebase is only worth doing once the split below is decided, and then only for the reformat commit.

Measured on the branch as it stands: 2,486 added lines, of which 2,426 change only whitespace (git diff -w leaves 60). The whole reformat carries 60 lines of content.

The split, and why the case is stronger now

The four commits divide cleanly:

commit content recommendation
.editorconfig 20 lines, no code touched land now
.git-blame-ignore-revs 14 lines, no code touched land now
uncrustify.cfg 20 lines, no code touched land now
apply uncrustify to niftilib/ 2,472 / 2,463 defer

The first three conflict with nothing, are useful the moment they land, and make the fourth reproducible by anyone at any later date.

The count of other open PRs touching the 8 files the reformat rewrites has gone from 15 to 21 since the earlier review: #21, #22, #24, #38, #39, #42, #43, #44, #47, #48, #49, #50, #52, #53, #55, #56, #58, #61, #67, #72, #74. Landing the reformat invalidates every one of them at once. Landing it after they merge costs nothing, because uncrustify.cfg makes the reformat a command rather than a patch.

Two small things to fix whenever this is picked up
  • uncrustify.cfg is in niftilib/, but it configures a repo-wide convention and the reformat is meant to extend to the other directories. It belongs at the repository root next to .editorconfig.
  • .git-blame-ignore-revs currently contains only # TODO: add reformatting commits. The reformat commit's own hash has to be appended once it lands and its final SHA is known — which, usefully, is another argument for landing the file first and the reformat later.

.editorconfig's indent_size = 3 matches the tree's actual convention; no issue there.

Commit messages

Rewritten in place with git filter-branch --msg-filter, no rebase. Trees, author, committer and all dates verified byte-identical afterwards. Each subject now carries the project's STYLE: prefix and fits 78 characters, and WIP: is gone from the reformat commit's subject since the commit itself is complete — it is the merge that is deferred, not the work.

Nothing was stripped: no commit here carried a tool trailer, a session URL, or a bare cross-reference.

No build or test run

Not run, and not meaningful for this branch in its current state: the reformat commit cannot be compiled against current master without first doing the rebase this comment argues against. The three config commits touch no code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants