Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

indentation unification #1210

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kai-burghardt
Copy link

  • Artificially bloat the source code tree by adding 969 KiB worth of spaces thus unifying the indentation style to 4 spaces per indentation level (as mentioned in issue Document source code style #337).
  • Only tabulator characters before non-whitespace characters were changed. There are still tabulator characters in the middle (or end) of lines.
  • Indentation for #define macros spanning multiple lines has not been changed, because there are multiple styles (zero vs. one vs. two levels of indentation, or align with the first definition word in the first line of #define).
  • Lines not containing any non-whitespace characters now have a consistent indentation level of zero (regardless of their location). Other trailing whitespace has not been removed (since it is not indentation).
  • Labels are now uniformly located at an indentation level of zero. Sometimes authors inserted an arbitrary amount of space but rarely a (positive) multiple of 4.
  • The indentation style for switch statements is now consistent. Everything within a switch is indentended by one level. This does not produce two confusing <same indentation>} lines anymore.
  • The indentation unification has caused many lines to exceed the 80 characters limit now. However no lines were wrapped so you can still attribute line authorship when ignoring whitespace changes.
  • Note: Files exclusively using spaces as indentation (or files exclusively using tabulation characters as indentation) are left unmodified. They may still contain inconsistent indentation.
  • To keep this pull request manageable, this change does not address any other of the many other styling issues (it’s really a big mess). Furthermore in this pull request only *.c and *.h files are changed.

- Artificially bloat the source code tree by adding 969 KiB worth of spaces
  thus unifying the indentation style to 4 spaces per indentation level
  (as mentioned in issue heimdal#337).
- Only tabulator characters before non-whitespace characters were changed.
  There are still tabulator characters in the middle (or end) of lines.
- Indentation for `#define` macros spanning multiple lines has not been changed,
  because there are multiple styles (zero vs. one vs. two levels of indentation,
  or align with the first definition word in the first line of `#define`).
- Lines not containing any non-whitespace characters
  now have a _consistent_ indentation level of zero
  (regardless of their location).
  Other trailing whitespace has not been removed (since it is not indentation).
- Labels are now uniformly located at an indentation level of zero.
  Sometimes authors inserted an arbitrary amount of space
  but rarely a (positive) multiple of 4.
- The indentation style for `switch` statements is now consistent.
  Everything within a `switch` is indentended by one level.
  This does not produce two confusing `<same indentation>}` lines anymore.
- The indentation unification has caused
  many lines to exceed the 80 characters limit now.
  However no lines were wrapped
  so you can still attribute line authorship when ignoring whitespace changes.
- Note:
  Files _exclusively_ using spaces as indentation
  (or files _exclusively_ using tabulation characters as indentation)
  are left unmodified.
  They may still contain inconsistent indentation.
@lhoward
Copy link
Member

lhoward commented Jan 6, 2024

Fixing this does make integrating changes from Apple's fork a bit harder but... it's pretty hard anyway.

@nicowilliams
Copy link
Contributor

I've proposed doing this before but my colleagues rejected the idea for reasons like:

  • the one @lhoward points out, that this makes merging Apple patches harder
  • style changes make git blame useless

I think I'd be happy to do this only to lib/gss code, and the rest fix as we touch it.

@kai-burghardt
Copy link
Author

kai-burghardt commented Jan 6, 2024

Fixing this does make integrating changes from Apple's fork a bit harder but... it's pretty hard anyway.

Oh, I wasn’t aware of a certain fork being vital. Where/what is this fork you mention? I could not find anything relevant in this GitHub project’s fork list.

  • style changes make git blame useless

I thought that’s what the ‑w option, ignore whitespace changes, is good for. Am I mistaken? I don’t use git that much. But is authorship attribution really so important? I’m just asking. Sometimes it’s just nice to have but not, like, mission critical.

I think I'd be happy to do this only to lib/gss code, and the rest fix as we touch it.

I’m afraid most code will never be touched again (because, you know, it works) so the occasion never arises. In general I don’t like the idea of commits addressing multiple unrelated issues at once. I would like to eliminate the opportunity of future commits making indentation unification changes, too, so they can focus on one objective.

@lhoward
Copy link
Member

lhoward commented Jan 6, 2024

Apple fork here:

https://github.com/apple-oss-distributions/Heimdal

@abartlet
Copy link
Member

Samba maintains a fairly extensive fork here: https://gitlab.com/samba-team/devel/lorikeet-heimdal/-/branches

https://gitlab.com/samba-team/devel/lorikeet-heimdal/-/commits/lorikeet-heimdal-202311290849/?ref_type=heads has the current outstanding changes on top of Heimdal's master of 2 months ago. A large re-indent could cause us a lot of work, and potentially introduce regressions if the automatic or manual merge is incorrect.

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.

None yet

4 participants