Automatic code formatting

Automatic code formatting

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2872 bytes --]

In the past, we've discussed the possibility of using an automatic code
formatter for Git.  That discussion has, as we've seen, not led to us
using one.  I'd like to reopen the discussion and provide a couple
reasons why I think it's a good idea.

We've spent a lot of work encouraging newcomers to contribute to Git.
It's demoralizing when your code comes back with many code style nits to
fix, and it leads to a barrier to contribution.  I also find that I am
likely to take longer to reroll a series if there are many nits to be
fixed, and I find that code style comments are a frequent discussion
topic on my series (even though I generally try to be cognizant of
them).

Having a code formatting tool means that the work for a contributor to
format the file properly consists of about two keystrokes.  This
substantially reduces the amount of time that contributors must spend
thinking about code formatting and simplifies it to an automatic process
that, if we choose, can even be verified in CI.

Most projects written in languages like Rust or Go use an automatic
formatter.  In Go's case, the formatter is specifically stated to be a
fixed style that is nobody's favourite, but because there's an automatic
formatter, everybody just uses it.  Personally, I don't love our coding
style now (I'm a 4-space person in C), but I would love it a lot more if
I didn't have to think about it.  I am substantially less picky about
what the style is than that we have an automated tool to tidy our code,
and I'm okay with us producing the occasional slightly suboptimal style
for the improved efficiency we get.

The impetus for me bringing this up is that I'm rebasing the
SHA-1/SHA-256 interop work to continue work on it and I find myself
spending a lot of time cleaning up formatting that I could instead be
spending on debugging why my tests are failing or writing new code.  I
would like to spend less time on boring scut work and more time solving
interesting problems, as I'm sure we all would.

I should point out that most platforms (and all major platforms) have
clang and therefore I think clang-format should be a fine choice.  It's
highly configurable and will let us pick a style that most resembles the
one we have now. However, I'm not picky and if we like something else
better, great.  As long as the option we pick is shipped in Debian, I'm
for it.

I should note that we already have a .clang-format file, so we can
already use clang-format.  However, we cannot blindly apply it because
it produces output that is not always conformant with our style.  My
proposal here is to define our style in terms of the formatter to avoid
this problem.

Hopefully we can move forward with this discussion and come to some
productive resolution.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: Automatic code formatting

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> I should point out that most platforms (and all major platforms) have
> clang and therefore I think clang-format should be a fine choice.  It's
> highly configurable and will let us pick a style that most resembles the
> one we have now. However, I'm not picky and if we like something else
> better, great.  As long as the option we pick is shipped in Debian, I'm
> for it.

As long as the options in clang-format we choose to use would
reformat the current codebase without disrupting too much, I would
be happy to see this done.  It would probably need a bit of work but
hopefully clang-format is flexible enough to accomodate our style to
allow us do this.

RE: Automatic code formatting

[rsbecker]

On July 10, 2022 5:50 PM, brian m. carlson wrote:
>In the past, we've discussed the possibility of using an automatic code formatter
>for Git.  That discussion has, as we've seen, not led to us using one.  I'd like to
>reopen the discussion and provide a couple reasons why I think it's a good idea.
>
>We've spent a lot of work encouraging newcomers to contribute to Git.
>It's demoralizing when your code comes back with many code style nits to fix, and
>it leads to a barrier to contribution.  I also find that I am likely to take longer to
>reroll a series if there are many nits to be fixed, and I find that code style
>comments are a frequent discussion topic on my series (even though I generally
>try to be cognizant of them).
>
>Having a code formatting tool means that the work for a contributor to format the
>file properly consists of about two keystrokes.  This substantially reduces the
>amount of time that contributors must spend thinking about code formatting and
>simplifies it to an automatic process that, if we choose, can even be verified in CI.
>
>Most projects written in languages like Rust or Go use an automatic formatter.  In
>Go's case, the formatter is specifically stated to be a fixed style that is nobody's
>favourite, but because there's an automatic formatter, everybody just uses it.
>Personally, I don't love our coding style now (I'm a 4-space person in C), but I
>would love it a lot more if I didn't have to think about it.  I am substantially less
>picky about what the style is than that we have an automated tool to tidy our
>code, and I'm okay with us producing the occasional slightly suboptimal style for
>the improved efficiency we get.
>
>The impetus for me bringing this up is that I'm rebasing the
>SHA-1/SHA-256 interop work to continue work on it and I find myself spending a
>lot of time cleaning up formatting that I could instead be spending on debugging
>why my tests are failing or writing new code.  I would like to spend less time on
>boring scut work and more time solving interesting problems, as I'm sure we all
>would.
>
>I should point out that most platforms (and all major platforms) have clang and
>therefore I think clang-format should be a fine choice.  It's highly configurable and
>will let us pick a style that most resembles the one we have now. However, I'm
>not picky and if we like something else better, great.  As long as the option we pick
>is shipped in Debian, I'm for it.
>
>I should note that we already have a .clang-format file, so we can already use
>clang-format.  However, we cannot blindly apply it because it produces output
>that is not always conformant with our style.  My proposal here is to define our
>style in terms of the formatter to avoid this problem.
>
>Hopefully we can move forward with this discussion and come to some productive
>resolution.

Being one of the platforms that will be specifically excluded from this proposal, I would like to offer an alternative. Before that, please remember that not everything is Linux. My suggestion is to create infrastructure to automatically format on add/commit. This could be pluggable relatively simply with clean filter that is language specific - perhaps with a helper option that installs the formatter easily (because clean filters are notoriously painful to install for newbies from my observations). It would be nice to have something in perl that is more portable and pervasive than clang - although perl could launch clang where available. I think having infrastructure for code formatting that is built into git is actually highly desirable - assuming that it is not unduly difficult to install those. It would extend beyond git contributions, but the contributors could be told (Contributor's Guide) that then need to follow standard X, which may very well be clang format. There are java formatters, php and perl formatters, even COBOL and TAL formatters. My position is that having a standard way to plug these in is a more general plan that would reach a larger community. Git contributions could then just leverage standard git functionality.

--Randall

Re: Automatic code formatting

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 3586 bytes --]

On 2022-07-10 at 22:13:01, rsbecker@nexbridge.com wrote:
> 
> Being one of the platforms that will be specifically excluded from
> this proposal, I would like to offer an alternative. Before that,
> please remember that not everything is Linux. My suggestion is to
> create infrastructure to automatically format on add/commit. This
> could be pluggable relatively simply with clean filter that is
> language specific - perhaps with a helper option that installs the
> formatter easily (because clean filters are notoriously painful to
> install for newbies from my observations). It would be nice to have
> something in perl that is more portable and pervasive than clang -
> although perl could launch clang where available. I think having
> infrastructure for code formatting that is built into git is actually
> highly desirable - assuming that it is not unduly difficult to install
> those. It would extend beyond git contributions, but the contributors
> could be told (Contributor's Guide) that then need to follow standard
> X, which may very well be clang format. There are java formatters, php
> and perl formatters, even COBOL and TAL formatters. My position is
> that having a standard way to plug these in is a more general plan
> that would reach a larger community. Git contributions could then just
> leverage standard git functionality.

I am willing to acknowledge the fact that not everybody has clang on their
preferred platform.  However, I assume you do have a laptop or desktop
with which you access your platform (since I believe it's a server
platform) and that you can install software there, or that you have the
ability to run some sort of virtualization framework on some system.

I am in general not very willing to say that we can't use or have useful
developer tools because of uncommon platforms.  Linux, Windows, macOS,
and (I believe) FreeBSD, NetBSD, and OpenBSD all support clang and
related tools, and I don't think it's unreasonable for us to expect that
someone can have access to such a system as part of development, even if
that's in a VM.  Those six operating systems plus Chrome OS constitute
the overwhelming majority of desktop and laptop systems, and there are
several options which are free (both as in speech and beer).

Moreover, clang and LLVM are extremely portable[0].  As a practical
matter, any platform wanting to support software written in Rust (a
popular and growing language) will likely need LLVM, and there is also a
lot of software in C and C++ that only supports GCC-compatible
compilers.  I do feel that providing support for modern toolchains is an
important part of providing a viable OS port, and that we should be able
to rely on porters for that OS to do that work.  I realize that LLVM is
not yet ported to your system, but I believe it's going to functionally
need to happen sooner or later.  When it does, you'll be able to send
patches directly without needing to copy to another OS to format the
code.

I should point out that I'm very willing to accept less common platforms
or architectures as build targets provided they can support the 2008
version of POSIX (which was released 14 years ago) reasonably well.  I
feel strongly that we should continue to support such systems for the
indefinite future to the best of our abilities.

[0] LLVM supports targeting 18 Debian architectures under Linux, Haiku,
VXWorks, Fuchsia, UEFI, WebAssembly, NVidia CUDA, the Nintendo 3DS, and
more.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

RE: Automatic code formatting

[rsbecker]

On July 10, 2022 8:59 PM, brian m. carlson wrote:
>On 2022-07-10 at 22:13:01, rsbecker@nexbridge.com wrote:
>>
>> Being one of the platforms that will be specifically excluded from
>> this proposal, I would like to offer an alternative. Before that,
>> please remember that not everything is Linux. My suggestion is to
>> create infrastructure to automatically format on add/commit. This
>> could be pluggable relatively simply with clean filter that is
>> language specific - perhaps with a helper option that installs the
>> formatter easily (because clean filters are notoriously painful to
>> install for newbies from my observations). It would be nice to have
>> something in perl that is more portable and pervasive than clang -
>> although perl could launch clang where available. I think having
>> infrastructure for code formatting that is built into git is actually
>> highly desirable - assuming that it is not unduly difficult to install
>> those. It would extend beyond git contributions, but the contributors
>> could be told (Contributor's Guide) that then need to follow standard
>> X, which may very well be clang format. There are java formatters, php
>> and perl formatters, even COBOL and TAL formatters. My position is
>> that having a standard way to plug these in is a more general plan
>> that would reach a larger community. Git contributions could then just
>> leverage standard git functionality.
>
>I am willing to acknowledge the fact that not everybody has clang on their
>preferred platform.  However, I assume you do have a laptop or desktop with
>which you access your platform (since I believe it's a server
>platform) and that you can install software there, or that you have the ability to
>run some sort of virtualization framework on some system.
>
>I am in general not very willing to say that we can't use or have useful developer
>tools because of uncommon platforms.  Linux, Windows, macOS, and (I believe)
>FreeBSD, NetBSD, and OpenBSD all support clang and related tools, and I don't
>think it's unreasonable for us to expect that someone can have access to such a
>system as part of development, even if that's in a VM.  Those six operating
>systems plus Chrome OS constitute the overwhelming majority of desktop and
>laptop systems, and there are several options which are free (both as in speech
>and beer).
>
>Moreover, clang and LLVM are extremely portable[0].  As a practical matter, any
>platform wanting to support software written in Rust (a popular and growing
>language) will likely need LLVM, and there is also a lot of software in C and C++
>that only supports GCC-compatible compilers.  I do feel that providing support for
>modern toolchains is an important part of providing a viable OS port, and that we
>should be able to rely on porters for that OS to do that work.  I realize that LLVM is
>not yet ported to your system, but I believe it's going to functionally need to
>happen sooner or later.  When it does, you'll be able to send patches directly
>without needing to copy to another OS to format the code.

I should point out that gcc will *never* according to our latest intel, be supported on my platforms. *Never* is, of course, an indeterminate definition, but until various matters I cannot legally discuss change, which are not likely for at least 5 years, anything depending on gcc is out of the picture and unavailable, including clang. I understand the position regarding git contributions, but I am trying to get the point across that formatting code to a standard is a more general desire than just git contributions. It is a broad desire/requirement that should be considered. Rather than making processes git-contribution-specific, providing a more general solution that git contributors can use is more effective.

--Randall

[RFC PATCH 0/4] make headway towards a clean "clang-format"

[Ævar Arnfjörð Bjarmason]

On Sun, Jul 10 2022, brian m. carlson wrote:

> Having a code formatting tool means that the work for a contributor to
> format the file properly consists of about two keystrokes.  This
> substantially reduces the amount of time that contributors must spend
> thinking about code formatting and simplifies it to an automatic process
> that, if we choose, can even be verified in CI.

I'm generally in favor of this, or more specifically I'm generally in
favor of whatever we can do to make
Documentation/{SubmittingPatches,CodingGuidelines}, t/README,
po/README.md and whatever other "checklist" document we have more
automated.

So we could generally waste less time in asking contributors to read and
internalize these documents, or in fixing submission mistakes, and more
time on worthwhile work.

Whether using an automatic formatter would land us on the right side of
that trade-off is something I'm not sure about, but if it would: Great!

But!

> I should note that we already have a .clang-format file, so we can
> already use clang-format.  However, we cannot blindly apply it because
> it produces output that is not always conformant with our style.  My
> proposal here is to define our style in terms of the formatter to avoid
> this problem.

I think this is very premature at this point.

I experimented a while ago (low number of months) with clang-format for
the first time, and found that the delta between what it proposed as
changes and what it *should* propose was needlessly high.

This RFC is a cleaned-up & updated version of those WIP experiments I
made. I think this series is "ready" in that it's a sensible
improvement, and having a "make style-all" family of rules is very
useful to check how .clang-format behaves.

But I think we're far short of declaring that we should simply refer
to clang-format against our shipped .clang-format configuration as our
canonical style.

At the end of this series we're at a 60% shorter suggested diff from
clang-format, but that still leaves ~13k lines of diff on top of
"master".

And, as 0[2-4]/04 note adjusting those rules isn't always clear, and
there's the major question of what to do about "ColumnLimit", and
things like "BitFieldColonSpacing" which require a much newer version
of clang than we're probably comfortable with requiring.

Someone more interested than me in this (i.e. you:) should really be
building on top of this, i.e. running:

	make style-all-diff-apply

And seeing what general categories these differences fall into, and
whether clang-format has any more options that we've set in the wrong
way, or which we're missing.

There's clearly a lot of changes in the resulting diff that we should
have per our style guide, e.g. there's a lot of "}\nelse {" (bad!)
v.s. "} else {" (good!) and the like.

Before we get to a "clean diff" we could at some point "invert" our
style guide. I.e. instead of saying "code should be like XYZ", we
could say:

	run clang-format, but it might report some differences, as
	long as it's one of the below known-cases that's OK.

I.e. we could one-off search-replace all the "}\nelse {" etc. cases to
the "} else {" form, and then we wouldn't have to waste words on that
in Documentation/CodingGuidelines.

But I *really* don't think we can declare that clang-format is some
source of truth before doing much or all of that work. If you think
nits are annoying now consider e.g. this change to advice.c:
	
	diff --git a/advice.c b/advice.c
	index 6fda9edbc24..e0e453d68d1 100644
	--- a/advice.c
	+++ b/advice.c
	@@ -35,6 +35,7 @@ static struct {
	 	const char *key;
	 	int enabled;
	 } advice_setting[] = {
	+	[ADVICE_FOO]					= { "foo", 1 },f
	 	[ADVICE_ADD_EMBEDDED_REPO]			= { "addEmbeddedRepo", 1 },
	 	[ADVICE_ADD_EMPTY_PATHSPEC]			= { "addEmptyPathspec", 1 },
	 	[ADVICE_ADD_IGNORED_FILE]			= { "addIgnoredFile", 1 },

I'll spare you the full diff, but if you do that and run "make style"
it'll spew out a ~100 line diff at you, as it re-indents and
re-formats that entire block.

That *is* a rather extreme example, most of its suggestions are 1-5
line diffs.

But if there's one hill I'm willing to die on it's that we should
never introduce any sort of "landmine checks" in this codebase.

Those are checks where we don't e.g. assert that the style of git.c is
OK, but rather just run a "diff" on your specific change.

Then if it happens to change any code that was *already in violation*
will make it your problem to fix that. I.e. the landmine was already
there, but you just happened to walk over that path, and *boom*.

But I'm not an unreasonable zealot about that:) E.g. as running the
"make style-all-diff-apply" target here shows you around 40% of our
*.[ch] files don't have any suggested changes.

That's around 30% with just 1/4, i.e. a 10% "gain" is made with the
2-4/4 rule changes here.

So, in combination with updating .clang-format we could start making
headway earlier by e.g. whitelisting files (or globs, or directories)
that are already "style clean".

It would be a bit annoying if we needed to move code from a "dirty" to
a "clean" file, but we mostly don't do that, so I'd think that would
be OK.

But I think like other "partial asserts" (such as the "linux-leaks" CI
job) such an approach really would need to proceed in parallel with
fixing the remaining cases, even though that might take a while.

I.e. we'd want to have some assurances that we'd either be willing to
live with a fully clang-format'd codebase, or at least that the
special-casing of the style rules wasn't too widespread. We have a bit
of "/* clang format (off|on) */" already, and e.g. for the likes of
advice.c that would be OK, but having it be a common pattern would be
annoying.

Ævar Arnfjörð Bjarmason (4):
  Makefile: add a style-all targets for .clang-format testing
  .clang-format: Add a BitFieldColonSpacing=None rule
  .clang-format: do not enforce a ColumnLimit
  .clang-format: don't indent "goto" labels

 .clang-format | 17 +++++++++----
 Makefile      | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 79 insertions(+), 5 deletions(-)

-- 
2.37.0.913.g189dca38629

[RFC PATCH 1/4] Makefile: add a style-all targets for .clang-format testing

[Ævar Arnfjörð Bjarmason]

Extend "style" rule added in 2118805b929 (Makefile: add style build
rule, 2017-08-14) with a set of "style-all" rules. These rules all
apply our ".clang-format" added in 6134de6ac18 (clang-format: outline
the git project's coding style, 2017-08-14) to all our tracked files,
rather than using "git-clang-format", which only applies the rules to
files that differ from those that are tracked.

This allows for testing and improving the .clang-format itself, as it
should as closely as possible mirror out stated
Documentation/CodingGuidelines, or in cases where we don't have
explicit guidelines it should match the prevailing preferred style in
the project.

Let's apply one such change, in 6134de6ac18 the "AlignEscapedNewlines"
configuration was set to "Left", but as setting it to "DontAlign"
shows our more common pattern is to not align "\"'s across newlines in
macro definitions.

Before & after the .clang-format change, running:

	git checkout -f '*.[ch]' && make -k style-all-diff-ok -k; make style-all-diff-apply

Will yield, respectively:

	578 files changed, 32191 insertions(+), 29944 deletions(-)
	579 files changed, 32065 insertions(+), 29818 deletions(-)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 .clang-format |  8 +++---
 Makefile      | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+), 4 deletions(-)

diff --git a/.clang-format b/.clang-format
index c592dda681f..3f536c49f24 100644
--- a/.clang-format
+++ b/.clang-format
@@ -32,12 +32,12 @@ AlignConsecutiveAssignments: false
 # double b = 3.14;
 AlignConsecutiveDeclarations: false
 
-# Align escaped newlines as far left as possible
-# #define A   \
+# Do not align escaped newlines in macro definitions
+# #define A \
 #   int aaaa; \
-#   int b;    \
+#   int b; \
 #   int cccccccc;
-AlignEscapedNewlines: Left
+AlignEscapedNewlines: DontAlign
 
 # Align operands of binary and ternary expressions
 # int aaa = bbbbbbbbbbb +
diff --git a/Makefile b/Makefile
index 04d0fd1fe60..67306820002 100644
--- a/Makefile
+++ b/Makefile
@@ -3108,10 +3108,77 @@ $(HCO): %.hco: %.hcc FORCE
 .PHONY: hdr-check $(HCO)
 hdr-check: $(HCO)
 
+##
+## clang-format targets
+##
+
+### style: apply a clang-format "diff" based on the changes in the
+### working tree
 .PHONY: style
 style:
 	git clang-format --style file --diff --extensions c,h
 
+#### style: common variables
+STYLE_SOURCES = $(filter-out $(THIRD_PARTY_SOURCES) $(EXCEPT_HDRS),\
+	$(FOUND_C_SOURCES) $(FOUND_H_SOURCES))
+
+#### style-all: generate clang-format output for everything. Copying
+#### the file to */in/* is so we can apply it with "git apply" (so one
+#### side won't have a .build/style-all/* prefix). If only "git apply"
+#### had a "-p" that applied only to one of "a/" or "b/" ...
+STYLE_SOURCES_IN = $(addprefix .build/style-all/in/,$(STYLE_SOURCES))
+$(STYLE_SOURCES_IN): .build/style-all/in/% : %
+	$(call mkdir_p_parent_template)
+	$(QUIET_LNCP)cp $< $@
+
+STYLE_SOURCES_OUT = $(addprefix .build/style-all/out/,$(STYLE_SOURCES))
+$(STYLE_SOURCES_OUT): .clang-format
+$(STYLE_SOURCES_OUT): .build/style-all/out/%: .build/style-all/in/%
+	$(call mkdir_p_parent_template)
+	$(QUIET_GEN)clang-format $< >$@
+.PHONY: style-all
+style-all: $(STYLE_SOURCES_GEN)
+
+#### style-all-diff: generate diffs of clang-format output
+#### v.s. checked-in files
+STYLE_SOURCES_DIFF = $(addprefix .build/style-all/diff/,$(STYLE_SOURCES))
+$(STYLE_SOURCES_DIFF): .build/style-all/diff/%: .build/style-all/out/%
+	$(call mkdir_p_parent_template)
+	$(QUIET_GEN)git \
+		-P \
+		diff --no-index \
+		$(<:.build/style-all/out/%=.build/style-all/in/%) $< >$@ || \
+	test -s $@
+
+.PHONY: style-all-diff
+style-all-diff: $(STYLE_SOURCES_DIFF)
+
+#### style-all-diff-ok: fail on files that have pending clang-format
+#### changes
+STYLE_SOURCES_DIFF_OK = $(addsuffix .ok,$(STYLE_SOURCES_DIFF))
+$(STYLE_SOURCES_DIFF_OK): %.ok: %
+	$(QUIET_GEN)! test -s $< && \
+	>$@
+.PHONY: style-all-diff-ok
+style-all-diff-ok: $(STYLE_SOURCES_DIFF_OK)
+
+.build/style-all.diff: $(STYLE_SOURCES_DIFF)
+	$(QUIET_GEN)cat $^ >$@
+
+#### style-all-diff-apply: apply the proposed clang-format changes. We
+#### need to have no local changes here, and use FORCE as the
+#### dependency graph of this rule is circular. I.e. we'll modify
+#### git.c, but eventually depend on it as well.
+STYLE_ALL_DIFF_APPLY_EXCLUDE =
+STYLE_ALL_DIFF_APPLY_EXCLUDE += ':!/Makefile'
+STYLE_ALL_DIFF_APPLY_EXCLUDE += ':!/.clang-format'
+style-all-diff-apply: FORCE
+style-all-diff-apply: .build/style-all.diff
+	git diff --quiet HEAD -- $(STYLE_ALL_DIFF_APPLY_EXCLUDE) && \
+	git diff --quiet --cached -- $(STYLE_ALL_DIFF_APPLY_EXCLUDE) && \
+	git apply -p4 $< && \
+	git -P diff --shortstat -- $(STYLE_ALL_DIFF_APPLY_EXCLUDE)
+
 .PHONY: check
 check: $(GENERATED_H)
 	@if sparse; \
-- 
2.37.0.913.g189dca38629

[RFC PATCH 2/4] .clang-format: Add a BitFieldColonSpacing=None rule

[Ævar Arnfjörð Bjarmason]

Formatting bitfield as "unsigned foo:1" is the usual style in this
project, not "unsigned foo : 1", which clang-format will use by
default.

Before & after this change running "make style-all-diff-apply" will
yield:

	582 files changed, 32029 insertions(+), 29794 deletions(-)
	579 files changed, 32065 insertions(+), 29818 deletions(-)

However this highlights a major limitation in this approach, because
clang-format v12 or newer is required for this rule, but that version
was only released in April 2021.

This change therefore cuts off anyone using an older clang-format. We
could decide to dynamically detect the version, and only supply this
as a --style="" rule on the command-line for older versions, but then
users on older versions would get different results.

So what do we do about that? Declare that "make style" is what mortal
users should run, but that we're going to run "make style-all-diff-ok"
on some blessed version of clang-format?

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 .clang-format | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/.clang-format b/.clang-format
index 3f536c49f24..87398d24d4f 100644
--- a/.clang-format
+++ b/.clang-format
@@ -144,6 +144,10 @@ SpacesInParentheses: false
 # int a[5];    not    int a[ 5 ];
 SpacesInSquareBrackets: false
 
+# Use "unsigned bf:2;", not "bf : 2" or whatever. Requires
+# clang-format 12.
+BitFieldColonSpacing: None
+
 # Insert a space after '{' and before '}' in struct initializers
 Cpp11BracedListStyle: false
 
-- 
2.37.0.913.g189dca38629

[RFC PATCH 3/4] .clang-format: do not enforce a ColumnLimit

[Ævar Arnfjörð Bjarmason]

Our Documentation/CodingGuidelines mention that "We try to keep to at
most 80 characters per line", but in reality we have a lot of code
that runs afoul of that rule.

Before & after this change running "make style-all-diff-apply" will
yield:

	579 files changed, 32065 insertions(+), 29818 deletions(-)
	509 files changed, 13042 insertions(+), 12745 deletions(-)

As with the preceding change what this leaves us with an unresolved
question though, should we have some stricter version of "make
style-all" that incorporates "ColumnLimit: 80", or perhaps apply it
only on "make style", but then what if someone modifies code that
happens to e.g. search/replace a line running afoul of the limit?

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 .clang-format | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.clang-format b/.clang-format
index 87398d24d4f..5a106d959be 100644
--- a/.clang-format
+++ b/.clang-format
@@ -12,7 +12,7 @@ UseTab: Always
 TabWidth: 8
 IndentWidth: 8
 ContinuationIndentWidth: 8
-ColumnLimit: 80
+ColumnLimit: 0
 
 # C Language specifics
 Language: Cpp
-- 
2.37.0.913.g189dca38629

[RFC PATCH 4/4] .clang-format: don't indent "goto" labels

[Ævar Arnfjörð Bjarmason]

This change is a slightly mixed bag, we have a lot of "goto" labels
that are indented by exactly one space.

Before & after this change running "make style-all-diff-apply" will
yield:

	509 files changed, 13042 insertions(+), 12745 deletions(-)
	510 files changed, 13039 insertions(+), 12742 deletions(-)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 .clang-format | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/.clang-format b/.clang-format
index 5a106d959be..56d7e8f9def 100644
--- a/.clang-format
+++ b/.clang-format
@@ -148,6 +148,9 @@ SpacesInSquareBrackets: false
 # clang-format 12.
 BitFieldColonSpacing: None
 
+# Do not indent "goto" labels, they should be flushed left.
+IndentGotoLabels: false
+
 # Insert a space after '{' and before '}' in struct initializers
 Cpp11BracedListStyle: false
 
-- 
2.37.0.913.g189dca38629

Re: Automatic code formatting

[Phillip Wood]

Hi brian

On 10/07/2022 22:50, brian m. carlson wrote:
> Most projects written in languages like Rust or Go use an automatic
> formatter.  In Go's case, the formatter is specifically stated to be a
> fixed style that is nobody's favourite, but because there's an automatic
> formatter, everybody just uses it.  Personally, I don't love our coding
> style now (I'm a 4-space person in C), but I would love it a lot more if
> I didn't have to think about it.  I am substantially less picky about
> what the style is than that we have an automated tool to tidy our code,
> and I'm okay with us producing the occasional slightly suboptimal style
> for the improved efficiency we get.
 > [...]
 > I should note that we already have a .clang-format file, so we can
 > already use clang-format.  However, we cannot blindly apply it because
 > it produces output that is not always conformant with our style.  My
 > proposal here is to define our style in terms of the formatter to avoid
 > this problem.

I agree it is lovely not to have to think about the style rules when 
writing code for a project that has an automatic formatter. As Junio 
said I think it would take a bit of work to persuade clang-format to 
match our style and I'm concerned that adopting the style it produces 
now would lead to a lot of churn. Below is an example taken from [1] 
that shows one area for improvement. At the moment its struct formatting 
seems inconsistent (one struct ends up with one field per line and the 
second has more than one field per line with a completely different 
indentation to the first) and I'm not sure we want it reformatting the 
whole definition when a new member is added. When Han-Wen Nienhuys 
submitted that patch I did have a brief play with the clang-format rules 
to try and improve it but didn't get anywhere.

Best Wishes

Phillip

[1] 
https://lore.kernel.org/git/2c2f94ddc0e77c8c70041a2a736e3a56698f058c.1589226388.git.gitgitgadget@gmail.com

@@ -3173,30 +3282,31 @@ static int files_init_db(struct ref_store 
*ref_store, struct strbuf *err)
  	return 0;
  }

-struct ref_storage_be refs_be_files = {
-	NULL,
-	"files",
-	files_ref_store_create,
-	files_init_db,
-	files_transaction_prepare,
-	files_transaction_finish,
-	files_transaction_abort,
-	files_initial_transaction_commit,
-
-	files_pack_refs,
-	files_create_symref,
-	files_delete_refs,
-	files_rename_ref,
-	files_copy_ref,
-
-	files_ref_iterator_begin,
-	files_read_raw_ref,
-
-	files_reflog_iterator_begin,
-	files_for_each_reflog_ent,
-	files_for_each_reflog_ent_reverse,
-	files_reflog_exists,
-	files_create_reflog,
-	files_delete_reflog,
-	files_reflog_expire
-};
+struct ref_storage_be refs_be_files = { NULL,
+					"files",
+					files_ref_store_create,
+					files_init_db,
+					files_transaction_prepare,
+					files_transaction_finish,
+					files_transaction_abort,
+					files_initial_transaction_commit,
+
+					files_pack_refs,
+					files_create_symref,
+					files_delete_refs,
+					files_rename_ref,
+					files_copy_ref,
+
+					files_write_pseudoref,
+					files_delete_pseudoref,
+
+					files_ref_iterator_begin,
+					files_read_raw_ref,
+
+					files_reflog_iterator_begin,
+					files_for_each_reflog_ent,
+					files_for_each_reflog_ent_reverse,
+					files_reflog_exists,
+					files_create_reflog,
+					files_delete_reflog,
+					files_reflog_expire };
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 4458a0f69cc..8f9b85f0b0c 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1641,29 +1641,19 @@ static int packed_reflog_expire(struct ref_store 
*ref_store,
  }

  struct ref_storage_be refs_be_packed = {
-	NULL,
-	"packed",
-	packed_ref_store_create,
-	packed_init_db,
-	packed_transaction_prepare,
-	packed_transaction_finish,
-	packed_transaction_abort,
-	packed_initial_transaction_commit,
-
-	packed_pack_refs,
-	packed_create_symref,
-	packed_delete_refs,
-	packed_rename_ref,
-	packed_copy_ref,
-
-	packed_ref_iterator_begin,
-	packed_read_raw_ref,
-
-	packed_reflog_iterator_begin,
-	packed_for_each_reflog_ent,
-	packed_for_each_reflog_ent_reverse,
-	packed_reflog_exists,
-	packed_create_reflog,
-	packed_delete_reflog,
-	packed_reflog_expire
+	NULL, "packed", packed_ref_store_create, packed_init_db,
+	packed_transaction_prepare, packed_transaction_finish,
+	packed_transaction_abort, packed_initial_transaction_commit,
+
+	packed_pack_refs, packed_create_symref, packed_delete_refs,
+	packed_rename_ref, packed_copy_ref,
+
+	/* XXX */
+	NULL, NULL,
+
+	packed_ref_iterator_begin, packed_read_raw_ref,
+
+	packed_reflog_iterator_begin, packed_for_each_reflog_ent,
+	packed_for_each_reflog_ent_reverse, packed_reflog_exists,
+	packed_create_reflog, packed_delete_reflog, packed_reflog_expire
  };

Re: Automatic code formatting

[Ævar Arnfjörð Bjarmason]

On Mon, Jul 11 2022, Phillip Wood wrote:

> Hi brian
>
> On 10/07/2022 22:50, brian m. carlson wrote:
>> Most projects written in languages like Rust or Go use an automatic
>> formatter.  In Go's case, the formatter is specifically stated to be a
>> fixed style that is nobody's favourite, but because there's an automatic
>> formatter, everybody just uses it.  Personally, I don't love our coding
>> style now (I'm a 4-space person in C), but I would love it a lot more if
>> I didn't have to think about it.  I am substantially less picky about
>> what the style is than that we have an automated tool to tidy our code,
>> and I'm okay with us producing the occasional slightly suboptimal style
>> for the improved efficiency we get.
>> [...]
>> I should note that we already have a .clang-format file, so we can
>> already use clang-format.  However, we cannot blindly apply it because
>> it produces output that is not always conformant with our style.  My
>> proposal here is to define our style in terms of the formatter to avoid
>> this problem.
>
> I agree it is lovely not to have to think about the style rules when
> writing code for a project that has an automatic formatter. As Junio 
> said I think it would take a bit of work to persuade clang-format to
> match our style and I'm concerned that adopting the style it produces 
> now would lead to a lot of churn. Below is an example taken from [1]
> that shows one area for improvement. At the moment its struct
> formatting seems inconsistent (one struct ends up with one field per
> line and the second has more than one field per line with a completely
> different indentation to the first) and I'm not sure we want it
> reformatting the whole definition when a new member is added. When
> Han-Wen Nienhuys submitted that patch I did have a brief play with the
> clang-format rules to try and improve it but didn't get anywhere.
>
> [1]
> https://lore.kernel.org/git/2c2f94ddc0e77c8c70041a2a736e3a56698f058c.1589226388.git.gitgitgadget@gmail.com
>
> @@ -3173,30 +3282,31 @@ static int files_init_db(struct ref_store
> *ref_store, struct strbuf *err)
>  	return 0;
>  }
>
> -struct ref_storage_be refs_be_files = {
> -	NULL,
> -	"files",
> -	files_ref_store_create,
> -	files_init_db,
> -	files_transaction_prepare,
> -	files_transaction_finish,
> -	files_transaction_abort,
> -	files_initial_transaction_commit,
> -
> -	files_pack_refs,
> -	files_create_symref,
> -	files_delete_refs,
> -	files_rename_ref,
> -	files_copy_ref,
> -
> -	files_ref_iterator_begin,
> -	files_read_raw_ref,
> -
> -	files_reflog_iterator_begin,
> -	files_for_each_reflog_ent,
> -	files_for_each_reflog_ent_reverse,
> -	files_reflog_exists,
> -	files_create_reflog,
> -	files_delete_reflog,
> -	files_reflog_expire
> -};
> +struct ref_storage_be refs_be_files = { NULL,
> +					"files",
> +					files_ref_store_create,
> +					files_init_db,
> +					files_transaction_prepare,
> +					files_transaction_finish,
> +					files_transaction_abort,
> +					files_initial_transaction_commit,
> +
> +					files_pack_refs,
> +					files_create_symref,
> +					files_delete_refs,
> +					files_rename_ref,
> +					files_copy_ref,
> +
> +					files_write_pseudoref,
> +					files_delete_pseudoref,
> +
> +					files_ref_iterator_begin,
> +					files_read_raw_ref,
> +
> +					files_reflog_iterator_begin,
> +					files_for_each_reflog_ent,
> +					files_for_each_reflog_ent_reverse,
> +					files_reflog_exists,
> +					files_create_reflog,
> +					files_delete_reflog,
> +					files_reflog_expire };

As my RFC series in the side-thread notes there's a lot of little
outstanding issues with its formatting, but this in particular isn't one
of them.

This one was resolved in my 32bff617c6a (refs: use designated
initializers for "struct ref_storage_be", 2022-03-17), and clang-format
currently has no formatting suggstions for these assignments on
"master".

And in general I think all such large assignments we wanted to convert
to designated initializers anyway, and/or have already done so.

With that series try:

	make style-all-diff-apply &&
	git diff

There's lots of outstanding issues for sure, for structs we lose some
borderline ascii-art alignment in some cases (e.g. the one in tar.h),
but I'd think those would be OK to either just format consistently, or
use "/* clang-format off */".

Re: Automatic code formatting

[Elijah Newren]

On Sun, Jul 10, 2022 at 6:31 PM <rsbecker@nexbridge.com> wrote:
>
> On July 10, 2022 8:59 PM, brian m. carlson wrote:
> >On 2022-07-10 at 22:13:01, rsbecker@nexbridge.com wrote:
> >>
> >> Being one of the platforms that will be specifically excluded from
> >> this proposal, I would like to offer an alternative. Before that,
> >> please remember that not everything is Linux. My suggestion is to
> >> create infrastructure to automatically format on add/commit. This
> >> could be pluggable relatively simply with clean filter that is
> >> language specific - perhaps with a helper option that installs the
> >> formatter easily (because clean filters are notoriously painful to
> >> install for newbies from my observations). It would be nice to have
> >> something in perl that is more portable and pervasive than clang -
> >> although perl could launch clang where available. I think having
> >> infrastructure for code formatting that is built into git is actually
> >> highly desirable - assuming that it is not unduly difficult to install
> >> those. It would extend beyond git contributions, but the contributors
> >> could be told (Contributor's Guide) that then need to follow standard
> >> X, which may very well be clang format. There are java formatters, php
> >> and perl formatters, even COBOL and TAL formatters. My position is
> >> that having a standard way to plug these in is a more general plan
> >> that would reach a larger community. Git contributions could then just
> >> leverage standard git functionality.
> >
> >I am willing to acknowledge the fact that not everybody has clang on their
> >preferred platform.  However, I assume you do have a laptop or desktop with
> >which you access your platform (since I believe it's a server
> >platform) and that you can install software there, or that you have the ability to
> >run some sort of virtualization framework on some system.
> >
> >I am in general not very willing to say that we can't use or have useful developer
> >tools because of uncommon platforms.  Linux, Windows, macOS, and (I believe)
> >FreeBSD, NetBSD, and OpenBSD all support clang and related tools, and I don't
> >think it's unreasonable for us to expect that someone can have access to such a
> >system as part of development, even if that's in a VM.  Those six operating
> >systems plus Chrome OS constitute the overwhelming majority of desktop and
> >laptop systems, and there are several options which are free (both as in speech
> >and beer).
> >
> >Moreover, clang and LLVM are extremely portable[0].  As a practical matter, any
> >platform wanting to support software written in Rust (a popular and growing
> >language) will likely need LLVM, and there is also a lot of software in C and C++
> >that only supports GCC-compatible compilers.  I do feel that providing support for
> >modern toolchains is an important part of providing a viable OS port, and that we
> >should be able to rely on porters for that OS to do that work.  I realize that LLVM is
> >not yet ported to your system, but I believe it's going to functionally need to
> >happen sooner or later.  When it does, you'll be able to send patches directly
> >without needing to copy to another OS to format the code.
>
> I should point out that gcc will *never* according to our latest intel, be supported on my platforms. *Never* is, of course, an indeterminate definition, but until various matters I cannot legally discuss change, which are not likely for at least 5 years, anything depending on gcc is out of the picture and unavailable, including clang. I understand the position regarding git contributions, but I am trying to get the point across that formatting code to a standard is a more general desire than just git contributions. It is a broad desire/requirement that should be considered. Rather than making processes git-contribution-specific, providing a more general solution that git contributors can use is more effective.

I don't understand why this matters to the proposal, though.  My
experience with projects adopting code-formatting is that there are
always some people who just don't use it (e.g. because they use a
different IDE that doesn't integrate with it, or they never bother
installing the extra tool, etc.).  Such users run the risk of having
automated CI jobs flag their code with a problem, and then they have
to go back and fix it, but it's still a net win because reviewers
don't have to spend time catching those problems, and the original
folks would have had to fix their code anyway.  In fact, in some
projects, I've been one of those users and having the project use a
code formatter that I didn't want to bother to set up and run didn't
negatively impact me at all.

Whether or not we have an automatic formatter, you're still
responsible to fix your code to match the project's guidelines.  I
don't think that becomes any harder, because if we have an automatic
formatter, the _most_ that changes is there's a flag day where the
style is adjusted slightly.  After that point, all you have to do is
the same as today: look at the surrounding code to see the format and
follow it.  So, from what I can see, you lose nothing from this
proposal if it is implemented.

Re: Automatic code formatting

[Ævar Arnfjörð Bjarmason]

On Mon, Jul 11 2022, Elijah Newren wrote:

> On Sun, Jul 10, 2022 at 6:31 PM <rsbecker@nexbridge.com> wrote:
>>
>> On July 10, 2022 8:59 PM, brian m. carlson wrote:
>> >On 2022-07-10 at 22:13:01, rsbecker@nexbridge.com wrote:
>> >>
>> >> Being one of the platforms that will be specifically excluded from
>> >> this proposal, I would like to offer an alternative. Before that,
>> >> please remember that not everything is Linux. My suggestion is to
>> >> create infrastructure to automatically format on add/commit. This
>> >> could be pluggable relatively simply with clean filter that is
>> >> language specific - perhaps with a helper option that installs the
>> >> formatter easily (because clean filters are notoriously painful to
>> >> install for newbies from my observations). It would be nice to have
>> >> something in perl that is more portable and pervasive than clang -
>> >> although perl could launch clang where available. I think having
>> >> infrastructure for code formatting that is built into git is actually
>> >> highly desirable - assuming that it is not unduly difficult to install
>> >> those. It would extend beyond git contributions, but the contributors
>> >> could be told (Contributor's Guide) that then need to follow standard
>> >> X, which may very well be clang format. There are java formatters, php
>> >> and perl formatters, even COBOL and TAL formatters. My position is
>> >> that having a standard way to plug these in is a more general plan
>> >> that would reach a larger community. Git contributions could then just
>> >> leverage standard git functionality.
>> >
>> >I am willing to acknowledge the fact that not everybody has clang on their
>> >preferred platform.  However, I assume you do have a laptop or desktop with
>> >which you access your platform (since I believe it's a server
>> >platform) and that you can install software there, or that you have the ability to
>> >run some sort of virtualization framework on some system.
>> >
>> >I am in general not very willing to say that we can't use or have useful developer
>> >tools because of uncommon platforms.  Linux, Windows, macOS, and (I believe)
>> >FreeBSD, NetBSD, and OpenBSD all support clang and related tools, and I don't
>> >think it's unreasonable for us to expect that someone can have access to such a
>> >system as part of development, even if that's in a VM.  Those six operating
>> >systems plus Chrome OS constitute the overwhelming majority of desktop and
>> >laptop systems, and there are several options which are free (both as in speech
>> >and beer).
>> >
>> >Moreover, clang and LLVM are extremely portable[0].  As a practical matter, any
>> >platform wanting to support software written in Rust (a popular and growing
>> >language) will likely need LLVM, and there is also a lot of software in C and C++
>> >that only supports GCC-compatible compilers.  I do feel that providing support for
>> >modern toolchains is an important part of providing a viable OS port, and that we
>> >should be able to rely on porters for that OS to do that work.  I realize that LLVM is
>> >not yet ported to your system, but I believe it's going to functionally need to
>> >happen sooner or later.  When it does, you'll be able to send patches directly
>> >without needing to copy to another OS to format the code.
>>
>> I should point out that gcc will *never* according to our latest
>> intel, be supported on my platforms. *Never* is, of course, an
>> indeterminate definition, but until various matters I cannot legally
>> discuss change, which are not likely for at least 5 years, anything
>> depending on gcc is out of the picture and unavailable, including
>> clang. I understand the position regarding git contributions, but I
>> am trying to get the point across that formatting code to a standard
>> is a more general desire than just git contributions. It is a broad
>> desire/requirement that should be considered. Rather than making
>> processes git-contribution-specific, providing a more general
>> solution that git contributors can use is more effective.
>
> I don't understand why this matters to the proposal, though.  My
> experience with projects adopting code-formatting is that there are
> always some people who just don't use it (e.g. because they use a
> different IDE that doesn't integrate with it, or they never bother
> installing the extra tool, etc.).  Such users run the risk of having
> automated CI jobs flag their code with a problem, and then they have
> to go back and fix it, but it's still a net win because reviewers
> don't have to spend time catching those problems, and the original
> folks would have had to fix their code anyway.  In fact, in some
> projects, I've been one of those users and having the project use a
> code formatter that I didn't want to bother to set up and run didn't
> negatively impact me at all.
>
> Whether or not we have an automatic formatter, you're still
> responsible to fix your code to match the project's guidelines.  I
> don't think that becomes any harder, because if we have an automatic
> formatter, the _most_ that changes is there's a flag day where the
> style is adjusted slightly.  After that point, all you have to do is
> the same as today: look at the surrounding code to see the format and
> follow it.  So, from what I can see, you lose nothing from this
> proposal if it is implemented.

I'm not saying I'm against this for the git project, but I think this is
a rather rosy view of how automated code formatting tends to go. I've
also seen all of:

 * It used to be trivial-ish to submit a patch to the project, and as my
   RFC series a lot of these minor formatting issues slide through the
   cracks, and we're not really worse for wear, but now it becomes a
   matter of automated enforcement.

   Which can be good, but also...

 * I think what's being pointed out upthread here is that it's also
   important that we pick development tools that are portable, because
   yes you'll find out about the issue eventually, but it's much faster
   if the turnaround is on your own computer.

   Some people who submit patches to git.git also don't use GitHub CI at
   all, so they'd find out about it after it hits "seen".

   You might argue that we'll catch it otherwise, but I have a "make"
   command you can run to get a ~30k diff that demonstrates otherwise :)

 * For e.g. the struct alignment exceptions I pointed out in the RFC
   cover letter I have myself (and seen others) written code that looks
   visually pleasant, but the code formatter happens to hate it
   (e.g. when you want to align a table in a certain way).

   Yes, you can usually use whatever the magic "format off" incantation
   is. But if you thought we had bikeshedding about code nits now, wait
   until we have long threads about whether something is or isn't the
   right time & place to override the automated formatting logic, and
   whether clang-format is buggy or whatever :)

In any case, per the RFC I sent I really think we should step more
gently into this area.

I was trying to find out if clang-format could be run in some mode where
it could only adjust one bit of its configured formatting,
e.g. "}\n\telse {" to "} else {", but I didn't find a way to do that.

That would also be a way to phase it in, and see how helpful
v.s. annoying it would be to e.g. run it in CI.

I also wonder if we can just go for another formatter altogether that's
more portable, e.g. GNU indent, but I haven't tested out how it compares
to clang-format.

Re: [RFC PATCH 4/4] .clang-format: don't indent "goto" labels

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> This change is a slightly mixed bag, we have a lot of "goto" labels
> that are indented by exactly one space.
>
> Before & after this change running "make style-all-diff-apply" will
> yield:
>
> 	509 files changed, 13042 insertions(+), 12745 deletions(-)
> 	510 files changed, 13039 insertions(+), 12742 deletions(-)

More files changed with fewer changes, or the above is not
copy-and-paste but typed in?

> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  .clang-format | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/.clang-format b/.clang-format
> index 5a106d959be..56d7e8f9def 100644
> --- a/.clang-format
> +++ b/.clang-format
> @@ -148,6 +148,9 @@ SpacesInSquareBrackets: false
>  # clang-format 12.
>  BitFieldColonSpacing: None
>  
> +# Do not indent "goto" labels, they should be flushed left.
> +IndentGotoLabels: false
> +
>  # Insert a space after '{' and before '}' in struct initializers
>  Cpp11BracedListStyle: false

Re: Automatic code formatting

[Elijah Newren]

On Mon, Jul 11, 2022 at 1:28 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> On Mon, Jul 11 2022, Elijah Newren wrote:
>
> > On Sun, Jul 10, 2022 at 6:31 PM <rsbecker@nexbridge.com> wrote:
> >>
> >> On July 10, 2022 8:59 PM, brian m. carlson wrote:
> >> >On 2022-07-10 at 22:13:01, rsbecker@nexbridge.com wrote:
> >> >>
> >> >> Being one of the platforms that will be specifically excluded from
> >> >> this proposal, I would like to offer an alternative. Before that,
> >> >> please remember that not everything is Linux. My suggestion is to
> >> >> create infrastructure to automatically format on add/commit. This
> >> >> could be pluggable relatively simply with clean filter that is
> >> >> language specific - perhaps with a helper option that installs the
> >> >> formatter easily (because clean filters are notoriously painful to
> >> >> install for newbies from my observations). It would be nice to have
> >> >> something in perl that is more portable and pervasive than clang -
> >> >> although perl could launch clang where available. I think having
> >> >> infrastructure for code formatting that is built into git is actually
> >> >> highly desirable - assuming that it is not unduly difficult to install
> >> >> those. It would extend beyond git contributions, but the contributors
> >> >> could be told (Contributor's Guide) that then need to follow standard
> >> >> X, which may very well be clang format. There are java formatters, php
> >> >> and perl formatters, even COBOL and TAL formatters. My position is
> >> >> that having a standard way to plug these in is a more general plan
> >> >> that would reach a larger community. Git contributions could then just
> >> >> leverage standard git functionality.
> >> >
> >> >I am willing to acknowledge the fact that not everybody has clang on their
> >> >preferred platform.  However, I assume you do have a laptop or desktop with
> >> >which you access your platform (since I believe it's a server
> >> >platform) and that you can install software there, or that you have the ability to
> >> >run some sort of virtualization framework on some system.
> >> >
> >> >I am in general not very willing to say that we can't use or have useful developer
> >> >tools because of uncommon platforms.  Linux, Windows, macOS, and (I believe)
> >> >FreeBSD, NetBSD, and OpenBSD all support clang and related tools, and I don't
> >> >think it's unreasonable for us to expect that someone can have access to such a
> >> >system as part of development, even if that's in a VM.  Those six operating
> >> >systems plus Chrome OS constitute the overwhelming majority of desktop and
> >> >laptop systems, and there are several options which are free (both as in speech
> >> >and beer).
> >> >
> >> >Moreover, clang and LLVM are extremely portable[0].  As a practical matter, any
> >> >platform wanting to support software written in Rust (a popular and growing
> >> >language) will likely need LLVM, and there is also a lot of software in C and C++
> >> >that only supports GCC-compatible compilers.  I do feel that providing support for
> >> >modern toolchains is an important part of providing a viable OS port, and that we
> >> >should be able to rely on porters for that OS to do that work.  I realize that LLVM is
> >> >not yet ported to your system, but I believe it's going to functionally need to
> >> >happen sooner or later.  When it does, you'll be able to send patches directly
> >> >without needing to copy to another OS to format the code.
> >>
> >> I should point out that gcc will *never* according to our latest
> >> intel, be supported on my platforms. *Never* is, of course, an
> >> indeterminate definition, but until various matters I cannot legally
> >> discuss change, which are not likely for at least 5 years, anything
> >> depending on gcc is out of the picture and unavailable, including
> >> clang. I understand the position regarding git contributions, but I
> >> am trying to get the point across that formatting code to a standard
> >> is a more general desire than just git contributions. It is a broad
> >> desire/requirement that should be considered. Rather than making
> >> processes git-contribution-specific, providing a more general
> >> solution that git contributors can use is more effective.
> >
> > I don't understand why this matters to the proposal, though.  My
> > experience with projects adopting code-formatting is that there are
> > always some people who just don't use it (e.g. because they use a
> > different IDE that doesn't integrate with it, or they never bother
> > installing the extra tool, etc.).  Such users run the risk of having
> > automated CI jobs flag their code with a problem, and then they have
> > to go back and fix it, but it's still a net win because reviewers
> > don't have to spend time catching those problems, and the original
> > folks would have had to fix their code anyway.  In fact, in some
> > projects, I've been one of those users and having the project use a
> > code formatter that I didn't want to bother to set up and run didn't
> > negatively impact me at all.
> >
> > Whether or not we have an automatic formatter, you're still
> > responsible to fix your code to match the project's guidelines.  I
> > don't think that becomes any harder, because if we have an automatic
> > formatter, the _most_ that changes is there's a flag day where the
> > style is adjusted slightly.  After that point, all you have to do is
> > the same as today: look at the surrounding code to see the format and
> > follow it.  So, from what I can see, you lose nothing from this
> > proposal if it is implemented.
>
> I'm not saying I'm against this for the git project, but I think this is
> a rather rosy view of how automated code formatting tends to go. I've
> also seen all of:
>
>  * It used to be trivial-ish to submit a patch to the project, and as my
>    RFC series a lot of these minor formatting issues slide through the
>    cracks, and we're not really worse for wear, but now it becomes a
>    matter of automated enforcement.
>
>    Which can be good, but also...

I think here you are just arguing that it's a huge pile of work to
switch to using an automatic code formatter.  I totally agree and
estimate it at multiple man months, and predict that it'd involve
adopting a different style (because no code formatter seems to produce
exactly what the project wants).  And there's always exclusions to
deal with, such as generated code, "vendored code", and
don't-you-dare-touch-this-low-level-thing-last-touched-by-someone-no-longer-involved-in-the-project,
or whatever.  I've seen many scars from other projects.  The cost of
switching can be formidable and may itself be a compelling reason to
not go through the effort.  But I was responding to the objection
raised, which wasn't about the cost of switching, but about the
end-state after switching.

I don't see how, once we've made the switch, that the barrier to
submitting patches is any higher.

>  * I think what's being pointed out upthread here is that it's also
>    important that we pick development tools that are portable, because
>    yes you'll find out about the issue eventually, but it's much faster
>    if the turnaround is on your own computer.

Currently, no one can find out about such issues on their own
computer.  The suggested change to use an automatic formatter would
let 90% (number pulled from thin air) of contributors find out.  I
agree that it is better if 100% of contributors can find out about
these issues on their own computer, so portable tools would be
beneficial, but I don't see why letting 0% of contributors find out on
their own computer is better than letting 90% find out.

>    Some people who submit patches to git.git also don't use GitHub CI at
>    all, so they'd find out about it after it hits "seen".

Much as they also do for a variety of other issues.  What's the
problem with that?

>    You might argue that we'll catch it otherwise, but I have a "make"
>    command you can run to get a ~30k diff that demonstrates otherwise :)

And I think here you might be arguing against using "portable" tools
without realizing it.  I totally believe your 30k diff number without
even having tried it, due to the other battle scars I've seen in other
projects.  If adopting automatic code formatting was trivial, everyone
would have done it years ago.  It's a huge lift, and the odds of
trying to get it to work with the code style that already exists are
slim to none.  I suspect that writing a versatile code formatter is
pretty close to writing a compiler for many languages, so if I were
choosing one, I would bias very heavily towards using one associated
with a compiler for the language I'm using. (e.g. clang-format,
rustfmt, gofmt, etc.).

>  * For e.g. the struct alignment exceptions I pointed out in the RFC
>    cover letter I have myself (and seen others) written code that looks
>    visually pleasant, but the code formatter happens to hate it
>    (e.g. when you want to align a table in a certain way).
>
>    Yes, you can usually use whatever the magic "format off" incantation
>    is. But if you thought we had bikeshedding about code nits now, wait
>    until we have long threads about whether something is or isn't the
>    right time & place to override the automated formatting logic, and
>    whether clang-format is buggy or whatever :)
>
> In any case, per the RFC I sent I really think we should step more
> gently into this area.
>
> I was trying to find out if clang-format could be run in some mode where
> it could only adjust one bit of its configured formatting,
> e.g. "}\n\telse {" to "} else {", but I didn't find a way to do that.
>
> That would also be a way to phase it in, and see how helpful
> v.s. annoying it would be to e.g. run it in CI.

Yeah, I think we're in full agreement that switching is much harder
than it often looks on the surface, and may well have some costs in
terms of changing the existing style in some places.

I'd be (very) surprised if switching was easy, but if someone else
wants to do the work of switching, then I'm all for it.

Especially since from past experience in other projects, even if _I_
don't use the automatic code formatting tools, the end result is still
better than the starting state.

But, yeah, I'm not volunteering to help with the transition.  Just
voicing that I think if others are willing to put in the effort, then
I think they should get a green light to go for it.

Re: [RFC PATCH 3/4] .clang-format: do not enforce a ColumnLimit

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> As with the preceding change what this leaves us with an unresolved
> question though, should we have some stricter version of "make
> style-all" that incorporates "ColumnLimit: 80", or perhaps apply it
> only on "make style", but then what if someone modifies code that
> happens to e.g. search/replace a line running afoul of the limit?

A more important thing to think about is that there is no single
good cut-off point.  When we say "wrap your lines at around 80
columns", we mean that when there is a good place to fold at around
column 65 and the next good place is at column 82, then it is OK to
go slightly over 80 and wrap at 82, which may be better than
wrapping at 65.  If the last good place to wrap is at column 72 and
the long function call at the end of the line makes you go past the
82nd column, wrapping at column 72 might be better.  I wonder if
there is an automated formatter that understands this kind of shades
of gray and lets us express that.

Thanks.

Re: [RFC PATCH 3/4] .clang-format: do not enforce a ColumnLimit

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1545 bytes --]

On 2022-07-11 at 11:37:27, Ævar Arnfjörð Bjarmason wrote:
> Our Documentation/CodingGuidelines mention that "We try to keep to at
> most 80 characters per line", but in reality we have a lot of code
> that runs afoul of that rule.
> 
> Before & after this change running "make style-all-diff-apply" will
> yield:
> 
> 	579 files changed, 32065 insertions(+), 29818 deletions(-)
> 	509 files changed, 13042 insertions(+), 12745 deletions(-)
> 
> As with the preceding change what this leaves us with an unresolved
> question though, should we have some stricter version of "make
> style-all" that incorporates "ColumnLimit: 80", or perhaps apply it
> only on "make style", but then what if someone modifies code that
> happens to e.g. search/replace a line running afoul of the limit?
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

As mentioned upthread, I am fine with an 80-character limit.  It's a
reasonable choice and what's we've traditionally done.

However, I don't think we should drop a limit altogether unless we're
going to not bother people about this in code review.  I would say that
if people are going to want a limit on line length, then we should pick
one.

Now, we could well pick one that's longer than 80 characters.  132 is a
common terminal size and it would avoid needing to rewrap all of those
lines.  But sticking with 80 columns is also fine, and we'll just need
to send some patches accordingly.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [RFC PATCH 2/4] .clang-format: Add a BitFieldColonSpacing=None rule

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 955 bytes --]

On 2022-07-11 at 11:37:26, Ævar Arnfjörð Bjarmason wrote:
> Formatting bitfield as "unsigned foo:1" is the usual style in this
> project, not "unsigned foo : 1", which clang-format will use by
> default.
> 
> Before & after this change running "make style-all-diff-apply" will
> yield:
> 
> 	582 files changed, 32029 insertions(+), 29794 deletions(-)
> 	579 files changed, 32065 insertions(+), 29818 deletions(-)
> 
> However this highlights a major limitation in this approach, because
> clang-format v12 or newer is required for this rule, but that version
> was only released in April 2021.

This isn't supported on Debian stable, which has clang 11.  I think we
should expect that to be a viable development target here, and I know
it's what some Git developers actually use.

I think for now we should drop this patch, and we can reconsider it in
the future.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [RFC PATCH 3/4] .clang-format: do not enforce a ColumnLimit

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> Now, we could well pick one that's longer than 80 characters.  132 is a
> common terminal size and it would avoid needing to rewrap all of those
> lines.  But sticking with 80 columns is also fine, and we'll just need
> to send some patches accordingly.

As long as people do not start sending an overly wide code that
consistently are 130 columns wide, I am fine.

Let's not encourage people to use automation as excuses for sending
unreadable mess and (worse yet) push back reviewer comments when
such issues that cannot be corrected with automation are pointed
out.

Thanks.

Re: [RFC PATCH 2/4] .clang-format: Add a BitFieldColonSpacing=None rule

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> On 2022-07-11 at 11:37:26, Ævar Arnfjörð Bjarmason wrote:
>> Formatting bitfield as "unsigned foo:1" is the usual style in this
>> project, not "unsigned foo : 1", which clang-format will use by
>> default.
>> 
>> Before & after this change running "make style-all-diff-apply" will
>> yield:
>> 
>> 	582 files changed, 32029 insertions(+), 29794 deletions(-)
>> 	579 files changed, 32065 insertions(+), 29818 deletions(-)
>> 
>> However this highlights a major limitation in this approach, because
>> clang-format v12 or newer is required for this rule, but that version
>> was only released in April 2021.
>
> This isn't supported on Debian stable, which has clang 11.  I think we
> should expect that to be a viable development target here, and I know
> it's what some Git developers actually use.
>
> I think for now we should drop this patch, and we can reconsider it in
> the future.

Earlier, somebody said "things that are not explicitly spelled out
in the guidelines, pick the more prevailing style", but I wonder
which one between "unsigned foo:1" and "unsigned foo : 1" is more
common in the current code.

Also, I am a bit curious why nobody has brought up the checkpatch
script we can borrow from the Linux kernel project.  I used to check
incoming patches before applying them and it was fairly effective in
catching malformed code.

Re: [RFC PATCH 3/4] .clang-format: do not enforce a ColumnLimit

[Eric Sunshine]

On Mon, Jul 11, 2022 at 6:50 PM Junio C Hamano <gitster@pobox.com> wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> > Now, we could well pick one that's longer than 80 characters.  132 is a
> > common terminal size and it would avoid needing to rewrap all of those
> > lines.  But sticking with 80 columns is also fine, and we'll just need
> > to send some patches accordingly.
>
> As long as people do not start sending an overly wide code that
> consistently are 130 columns wide, I am fine.
>
> Let's not encourage people to use automation as excuses for sending
> unreadable mess and (worse yet) push back reviewer comments when
> such issues that cannot be corrected with automation are pointed
> out.

In my experience, clang-format will reflow a line of code so that it
fills the configured line-length. So, even if you manually wrap the
code to fit nicely in 80 columns, if clang-format is set to 132
columns, then it will automatically reflow your nicely hand-wrapped
80-column code out to 132 columns, which I think is not what we'd want
(at least those of us who always work in 80-column terminals and
editors). But perhaps there is a configuration knob which disables
clang-format's "reflow-to-occupy-full-width" behavior? brian?

Re: [RFC PATCH 3/4] .clang-format: do not enforce a ColumnLimit

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> So, even if you manually wrap the code to fit nicely in 80
> columns, if clang-format is set to 132 columns, then it will
> automatically reflow your nicely hand-wrapped 80-column code out
> to 132 columns, which I think is not what we'd want (at least
> those of us who always work in 80-column terminals and
> editors). But perhaps there is a configuration knob which disables
> clang-format's "reflow-to-occupy-full-width" behavior? brian?

Agreed.

We should keep in mind that there is no single good fill-column.

When we say "wrap your lines at around 80 columns", we mean that
when there is a good place to fold at around column 65 and the next
good place is at column 82, then it is OK to go slightly over 80 and
wrap at 82, which may be better than wrapping at 65.  If the last
good place to wrap is at column 72 and the long function call at the
end of the line makes you go past the 82nd column, wrapping at
column 72 might be better.

I wonder if there is an automated formatter that understands this
kind of shades of gray and lets us express that.

Thanks.

Re: [RFC PATCH 4/4] .clang-format: don't indent "goto" labels

[Ævar Arnfjörð Bjarmason]

On Mon, Jul 11 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> This change is a slightly mixed bag, we have a lot of "goto" labels
>> that are indented by exactly one space.
>>
>> Before & after this change running "make style-all-diff-apply" will
>> yield:
>>
>> 	509 files changed, 13042 insertions(+), 12745 deletions(-)
>> 	510 files changed, 13039 insertions(+), 12742 deletions(-)
>
> More files changed with fewer changes, or the above is not
> copy-and-paste but typed in?

It's copy/pasted, i.e. it's hit & miss when you massage these settings
if we change more files, or change fewer lines or whatever, and thes may
not be the same changes in the same files...

Re: [RFC PATCH 2/4] .clang-format: Add a BitFieldColonSpacing=None rule

[Ævar Arnfjörð Bjarmason]

On Mon, Jul 11 2022, brian m. carlson wrote:

> [[PGP Signed Part:Undecided]]
> On 2022-07-11 at 11:37:26, Ævar Arnfjörð Bjarmason wrote:
>> Formatting bitfield as "unsigned foo:1" is the usual style in this
>> project, not "unsigned foo : 1", which clang-format will use by
>> default.
>> 
>> Before & after this change running "make style-all-diff-apply" will
>> yield:
>> 
>> 	582 files changed, 32029 insertions(+), 29794 deletions(-)
>> 	579 files changed, 32065 insertions(+), 29818 deletions(-)
>> 
>> However this highlights a major limitation in this approach, because
>> clang-format v12 or newer is required for this rule, but that version
>> was only released in April 2021.
>
> This isn't supported on Debian stable, which has clang 11.  I think we
> should expect that to be a viable development target here, and I know
> it's what some Git developers actually use.
>
> I think for now we should drop this patch, and we can reconsider it in
> the future.

That makes sense, but the unanswered question is still how we should
relate this to your proposal of standardizing on clang-format.

I.e. this & maybe AlignArrayOfStructures (and probably some other
things) are probably things we'd like to enable to closely match the
style we have now (in the case of that setting, because we've converted
the rest to designated initializers).

So we'd be left with one of:
 
 1. Just formatting according to the style an older version supports,
    even if it's not the preferred one.

 2. Require the newer version, after all you don't *need* to use
    clang-format, it's OK that we can run it somewhere...

 3. Fix some things now, maintain some whitelist of exceptions, and work
    towards parity (possibly with running a newer clang-format e.g. in
    CI).

 4. Something else...?

Re: [RFC PATCH 3/4] .clang-format: do not enforce a ColumnLimit

[Ævar Arnfjörð Bjarmason]

On Mon, Jul 11 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> As with the preceding change what this leaves us with an unresolved
>> question though, should we have some stricter version of "make
>> style-all" that incorporates "ColumnLimit: 80", or perhaps apply it
>> only on "make style", but then what if someone modifies code that
>> happens to e.g. search/replace a line running afoul of the limit?
>
> A more important thing to think about is that there is no single
> good cut-off point.  When we say "wrap your lines at around 80
> columns", we mean that when there is a good place to fold at around
> column 65 and the next good place is at column 82, then it is OK to
> go slightly over 80 and wrap at 82, which may be better than
> wrapping at 65.  If the last good place to wrap is at column 72 and
> the long function call at the end of the line makes you go past the
> 82nd column, wrapping at column 72 might be better. 

There's the story of the sufficiently smart compiler, and now the
sufficiently smart formatter :)

The proposed solution here is to punt on it, which I think makes sense
if we're trying to push forward clang-format.

(Which I'm really not, this RFC is something I thought I'd send in
response to brian's proposal, as I'd poked at this locally a while ago,
after wondering if I could make use of it myself, and whether our
.clang-format was misconfigured[1]).

> I wonder if
> there is an automated formatter that understands this kind of shades
> of gray and lets us express that.

I don't think so, and setting the configuration to "0" is only a
stop-gap, after all we'd still like it to wrap e.g. lines of a length of
150 or whatever, if it finds them somewhere.

1. I think after I found that some odd styling from one of Han-Wen's
   patches was the result of running clang-format, cf.:
   https://lore.kernel.org/git/CAFQ2z_PAqW+RS2Znaf2wwOJfdNfkjP1VV84=xaPu_1EAuX+u5w@mail.gmail.com/