[PATCH 0/8] clang-format: add more rules and enable on CI

[PATCH 0/8] clang-format: add more rules and enable on CI

[Karthik Nayak]

The series was mostly inspired by a patch sent recently to 'include
kh_foreach* macros in ForEachMacros' [1]. I was wondering why we don't
run the formatting on CI and reduce some of the workload of reviewers.

We have a '.clang-format' file to provide rules for code formatting.
The commits 1-6 aims to add more rules to the file while deprecating old
ones.

Commit 7 enables CI action on GitHub and GitLab to also check for the
code formatting. Currently, it is allowed to fail. This way we can build
confidence and fine tune the values as needed and finally enforce the
check in a future patch. I'm not well versed with GitHub workflows, and
I mostly tried to copy existing work there. Expecting some feedback in
that section!

Commit 8 fixes an existing issue with the 'check-whitespace' job, which
is failing as success in the GitLab CI.

1: https://lore.kernel.org/git/4e7893f5-2dd9-46cf-8a64-cf780f4e1730@web.de/

Karthik Nayak (8):
  clang-format: indent preprocessor directives after hash
  clang-format: avoid spacing around bitfield colon
  clang-format: ensure files end with newlines
  clang-format: replace deprecated option with 'SpacesInParens'
  clang-format: avoid braces on simple single-statement bodies
  clang-format: formalize some of the spacing rules
  ci: run style check on GitHub and GitLab
  check-whitespace: detect if no base_commit is provided

 .clang-format                     | 41 +++++++++++++++++++++++++++----
 .github/workflows/check-style.yml | 29 ++++++++++++++++++++++
 .gitlab-ci.yml                    | 14 ++++++++++-
 ci/check-whitespace.sh            | 13 +++++++---
 ci/install-dependencies.sh        |  2 +-
 ci/run-style-check.sh             |  8 ++++++
 6 files changed, 97 insertions(+), 10 deletions(-)
 create mode 100644 .github/workflows/check-style.yml
 create mode 100755 ci/run-style-check.sh

-- 
2.45.1

[PATCH 1/8] clang-format: indent preprocessor directives after hash

[Karthik Nayak]

We do not have a rule around the indentation of preprocessor directives.
This was also discussed on the list [1], noting how there is often
inconsistency in the styling. While there was discussion, there was no
conclusion around what is the preferred style here. One style being
indenting after the hash:

    #if FOO
    #  if BAR
    #    include <foo>
    #  endif
    #endif

The other being before the hash:

    #if FOO
      #if BAR
        #include <foo>
      #endif
    #endif

Let's pick the former and add 'IndentPPDirectives: AfterHash' value to
our '.clang-format'. There is no clear reason to pick one over the
other, but it would definitely be nicer to be consistent.

[1]: https://lore.kernel.org/r/xmqqwmmm1bw6.fsf@gitster.g

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .clang-format | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/.clang-format b/.clang-format
index 3ed4fac753..5e128519bf 100644
--- a/.clang-format
+++ b/.clang-format
@@ -96,6 +96,12 @@ BreakStringLiterals: false
 # Switch statement body is always indented one level more than case labels.
 IndentCaseLabels: false
 
+# Indents directives before the hash.
+# #if FOO
+# #  include <foo>
+# #endif
+IndentPPDirectives: AfterHash
+
 # Don't indent a function definition or declaration if it is wrapped after the
 # type
 IndentWrappedFunctionNames: false
-- 
2.45.1

[PATCH 2/8] clang-format: avoid spacing around bitfield colon

[Karthik Nayak]

The spacing around colons is currently not standardized and as such we
have the following practices in our code base:
- Spacing around the colon `int bf : 1`: 146 instances
- No spacing around the colon `int bf:1`: 148 instances
- Spacing before the colon `int bf :1`: 6 instances
- Spacing after the colon `int bf: 1`: 12 instances

Let's formalize this by picking the most followed pattern and add the
corresponding style to '.clang-format'.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .clang-format | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/.clang-format b/.clang-format
index 5e128519bf..803b274dd5 100644
--- a/.clang-format
+++ b/.clang-format
@@ -72,6 +72,10 @@ AlwaysBreakAfterReturnType: None
 BinPackArguments: true
 BinPackParameters: true
 
+# Add no space around the bit field
+# unsigned bf:2;
+BitFieldColonSpacing: None
+
 # Attach braces to surrounding context except break before braces on function
 # definitions.
 # void foo()
-- 
2.45.1

[PATCH 3/8] clang-format: ensure files end with newlines

[Karthik Nayak]

All our source files end with a newline, let's formalize in
'.clang-format'.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .clang-format | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/.clang-format b/.clang-format
index 803b274dd5..4c9751a9db 100644
--- a/.clang-format
+++ b/.clang-format
@@ -106,6 +106,9 @@ IndentCaseLabels: false
 # #endif
 IndentPPDirectives: AfterHash
 
+# Insert a newline at end of file if missing
+InsertNewlineAtEOF: true
+
 # Don't indent a function definition or declaration if it is wrapped after the
 # type
 IndentWrappedFunctionNames: false
-- 
2.45.1

[PATCH 4/8] clang-format: replace deprecated option with 'SpacesInParens'

[Karthik Nayak]

Replace the deprecated options 'SpacesInParentheses' and
'SpaceInEmptyParentheses' with the newer 'SpacesInParens' option. The
usage is the same.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .clang-format | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/.clang-format b/.clang-format
index 4c9751a9db..914254a29b 100644
--- a/.clang-format
+++ b/.clang-format
@@ -134,8 +134,6 @@ SpaceBeforeAssignmentOperators: true
 # }
 SpaceBeforeParens: ControlStatements
 
-# Don't insert spaces inside empty '()'
-SpaceInEmptyParentheses: false
 
 # The number of spaces before trailing line comments (// - comments).
 # This does not affect trailing block comments (/* - comments).
@@ -149,9 +147,10 @@ SpacesInCStyleCastParentheses: false
 # var arr = [1, 2, 3];    not    var arr = [ 1, 2, 3 ];
 SpacesInContainerLiterals: false
 
-# Don't insert spaces after '(' or before ')'
-# f(arg);    not    f( arg );
-SpacesInParentheses: false
+SpacesInParens: Custom
+SpacesInParensOptions:
+  # Don't insert spaces inside empty '()'
+  InEmptyParentheses: false
 
 # Don't insert spaces after '[' or before ']'
 # int a[5];    not    int a[ 5 ];
-- 
2.45.1

[PATCH 5/8] clang-format: avoid braces on simple single-statement bodies

[Karthik Nayak]

Set the 'RemoveBracesLLVM' to 'true' which ensures that we avoid curly
braces for single-statement bodies in conditional blocks.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .clang-format | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/.clang-format b/.clang-format
index 914254a29b..1a5f0c9046 100644
--- a/.clang-format
+++ b/.clang-format
@@ -117,6 +117,11 @@ IndentWrappedFunctionNames: false
 # int *a;
 PointerAlignment: Right
 
+# Remove optional braces of control statements (if, else, for, and while)
+# according to the LLVM coding style
+# This avoids braces on simple single-statement bodies of statements.
+RemoveBracesLLVM: true
+
 # Don't insert a space after a cast
 # x = (int32)y;    not    x = (int32) y;
 SpaceAfterCStyleCast: false
-- 
2.45.1

[PATCH 6/8] clang-format: formalize some of the spacing rules

[Karthik Nayak]

There are some spacing rules that we follow in the project and it makes
sen to formalize them:
* Ensure there is no space inserted after the logical not '!' operator.
* Ensure there is no space before the case statement's color.
* Ensure there is no space before the first bracket '[' of an array.
* Ensure there is no space in empty blocks.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .clang-format | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/.clang-format b/.clang-format
index 1a5f0c9046..05036f610b 100644
--- a/.clang-format
+++ b/.clang-format
@@ -126,11 +126,18 @@ RemoveBracesLLVM: true
 # x = (int32)y;    not    x = (int32) y;
 SpaceAfterCStyleCast: false
 
+# No space is inserted after the logical not operator
+SpaceAfterLogicalNot: false
+
 # Insert spaces before and after assignment operators
 # int a = 5;    not    int a=5;
 # a += 42;             a+=42;
 SpaceBeforeAssignmentOperators: true
 
+# Spaces will be removed before case colon.
+# case 1: break;    not     case 1 : break;
+SpaceBeforeCaseColon: false
+
 # Put a space before opening parentheses only after control statement keywords.
 # void f() {
 #   if (true) {
@@ -139,6 +146,13 @@ SpaceBeforeAssignmentOperators: true
 # }
 SpaceBeforeParens: ControlStatements
 
+# No space before first '[' in arrays
+# int a[5][5];     not      int a [5][5];
+SpaceBeforeSquareBrackets: false
+
+# No space will be inserted into {}
+# while (true) {}    not    while (true) { }
+SpaceInEmptyBlock: false
 
 # The number of spaces before trailing line comments (// - comments).
 # This does not affect trailing block comments (/* - comments).
-- 
2.45.1

[PATCH 7/8] ci: run style check on GitHub and GitLab

[Karthik Nayak]

We don't run style checks on our CI, even though we have a
'.clang-format' setup in the repository. Let's add one, the job will
validate only against the new commits added and will only run on merge
requests. Since we're introducing it for the first time, let's allow
this job to fail, so we can validate if this is useful and eventually
enforce it.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .github/workflows/check-style.yml | 29 +++++++++++++++++++++++++++++
 .gitlab-ci.yml                    | 12 ++++++++++++
 ci/install-dependencies.sh        |  2 +-
 ci/run-style-check.sh             |  8 ++++++++
 4 files changed, 50 insertions(+), 1 deletion(-)
 create mode 100644 .github/workflows/check-style.yml
 create mode 100755 ci/run-style-check.sh

diff --git a/.github/workflows/check-style.yml b/.github/workflows/check-style.yml
new file mode 100644
index 0000000000..27276dfe5e
--- /dev/null
+++ b/.github/workflows/check-style.yml
@@ -0,0 +1,29 @@
+name: check-style
+
+# Get the repository with all commits to ensure that we can analyze
+# all of the commits contributed via the Pull Request.
+
+on:
+  pull_request:
+    types: [opened, synchronize]
+
+# Avoid unnecessary builds. Unlike the main CI jobs, these are not
+# ci-configurable (but could be).
+concurrency:
+  group: ${{ github.workflow }}-${{ github.ref }}
+  cancel-in-progress: true
+
+jobs:
+  check-style:
+    runs-on: ubuntu-latest
+    steps:
+    - uses: actions/checkout@v4
+      with:
+        fetch-depth: 0
+
+    - name: git clang-format
+      continue-on-error: true
+      id: check_out
+      run: |
+        ./ci/run-style-check.sh \
+          "${{github.event.pull_request.base.sha}}"
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 37b991e080..65fd261e5e 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -123,6 +123,18 @@ check-whitespace:
   rules:
     - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
 
+check-style:
+  image: ubuntu:latest
+  allow_failure: true
+  variables:
+    CC: clang
+  before_script:
+    - ./ci/install-dependencies.sh
+  script:
+    - ./ci/run-style-check.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
+  rules:
+    - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
+
 documentation:
   image: ubuntu:latest
   variables:
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 6ec0f85972..46fe12a690 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -43,7 +43,7 @@ ubuntu-*)
 		make libssl-dev libcurl4-openssl-dev libexpat-dev wget sudo default-jre \
 		tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl \
 		libemail-valid-perl libio-pty-perl libio-socket-ssl-perl libnet-smtp-ssl-perl libdbd-sqlite3-perl libcgi-pm-perl \
-		${CC_PACKAGE:-${CC:-gcc}} $PYTHON_PACKAGE
+		${CC_PACKAGE:-${CC:-gcc}} $PYTHON_PACKAGE clang-format
 
 	mkdir --parents "$CUSTOM_PATH"
 	wget --quiet --directory-prefix="$CUSTOM_PATH" \
diff --git a/ci/run-style-check.sh b/ci/run-style-check.sh
new file mode 100755
index 0000000000..9d4c4089c1
--- /dev/null
+++ b/ci/run-style-check.sh
@@ -0,0 +1,8 @@
+#!/usr/bin/env sh
+#
+# Perform style check
+#
+
+baseCommit=$1
+
+git clang-format --style file --diff --extensions c,h "$baseCommit"
-- 
2.45.1

[PATCH 8/8] check-whitespace: detect if no base_commit is provided

[Karthik Nayak]

The 'check-whitespace' CI script exists gracefully if no base commit is
provided or if an invalid revision is provided. This is not good because
if a particular CI provides an incorrect base_commit, it would fail
successfully.

This is exactly the case with the GitLab CI. The CI is using the
"$CI_MERGE_REQUEST_TARGET_BRANCH_SHA" variable to get the base commit
SHA, but variable is only defined for _merged_ pipelines. So it is empty
for regular pipelines [1]. This should've failed the check-whitespace
job.

Let's fix the variable used in the GitLab CI. Let's also add a check for
incorrect base_commit in the 'check-whitespace.sh' script. While here,
fix a small typo too.

[1]: https://docs.gitlab.com/ee/ci/variables/predefined_variables.html#predefined-variables-for-merge-request-pipelines

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .gitlab-ci.yml         |  2 +-
 ci/check-whitespace.sh | 13 ++++++++++---
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 65fd261e5e..36199893d8 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -119,7 +119,7 @@ check-whitespace:
   before_script:
     - ./ci/install-dependencies.sh
   script:
-    - ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
+    - ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
   rules:
     - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
 
diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh
index db399097a5..ab023f9519 100755
--- a/ci/check-whitespace.sh
+++ b/ci/check-whitespace.sh
@@ -9,12 +9,19 @@ baseCommit=$1
 outputFile=$2
 url=$3
 
-if test "$#" -ne 1 && test "$#" -ne 3
+if { test "$#" -ne 1 && test "$#" -ne 3; } || test -z "$1"
 then
 	echo "USAGE: $0 <BASE_COMMIT> [<OUTPUT_FILE> <URL>]"
 	exit 1
 fi
 
+gitLogOutput=$(git log --check --pretty=format:"---% h% s" "${baseCommit}"..)
+if test $? -ne 0
+then
+	echo -n $gitLogOutput
+	exit 1
+fi
+
 problems=()
 commit=
 commitText=
@@ -58,7 +65,7 @@ do
 		echo "${dash} ${sha} ${etc}"
 		;;
 	esac
-done <<< "$(git log --check --pretty=format:"---% h% s" "${baseCommit}"..)"
+done <<< "$gitLogOutput"
 
 if test ${#problems[*]} -gt 0
 then
@@ -67,7 +74,7 @@ then
 		goodParent=${baseCommit: 0:7}
 	fi
 
-	echo "A whitespace issue was found in onen of more of the commits."
+	echo "A whitespace issue was found in one of more of the commits."
 	echo "Run the following command to resolve whitespace issues:"
 	echo "git rebase --whitespace=fix ${goodParent}"
 
-- 
2.45.1

Re: [PATCH 0/8] clang-format: add more rules and enable on CI

[Chris Torek]

On Mon, Jul 8, 2024 at 2:23 AM Karthik Nayak <karthik.188@gmail.com> wrote:
> The series was mostly inspired by a patch sent recently to 'include
> kh_foreach* macros in ForEachMacros' [1]. I was wondering why we don't
> run the formatting on CI and reduce some of the workload of reviewers.

As a big fan of automated formatting, I scanned through all of these.
I did not spot anything problematic and had just a few minor comments.
I'm not an expert on clang-format directives, nor on GitHub/GitLab CI,
though.  Anyway if you want it you can have a reviewed-by from me. :-)

(Comments stuck on individual patches)

Chris

Re: [PATCH 1/8] clang-format: indent preprocessor directives after hash

[Chris Torek]

On Mon, Jul 8, 2024 at 2:23 AM Karthik Nayak <karthik.188@gmail.com> wrote:
> Let's pick the former and add 'IndentPPDirectives: AfterHash' value to
> our '.clang-format'. There is no clear reason to pick one over the
> other, but it would definitely be nicer to be consistent.

There is one extremely weak technical reason to pick "# in left column",
which is: some ancient C compilers required it. I don't think any modern
ones do.

Chris

Re: [PATCH 8/8] check-whitespace: detect if no base_commit is provided

[Chris Torek]

On Mon, Jul 8, 2024 at 2:24 AM Karthik Nayak <karthik.188@gmail.com> wrote:
> diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh
> index db399097a5..ab023f9519 100755
> --- a/ci/check-whitespace.sh
> +++ b/ci/check-whitespace.sh
> @@ -9,12 +9,19 @@ baseCommit=$1
>  outputFile=$2
>  url=$3
>
> -if test "$#" -ne 1 && test "$#" -ne 3
> +if { test "$#" -ne 1 && test "$#" -ne 3; } || test -z "$1"
>  then

The braces `{ ... }` here are unnecessary as `&&`will bind first
(sh and bash give both operators the same precedence but
then use left associativity). Dropping them drops the need for
the semicolon. Of course some might prefer this to be explicit.

The `test` command has AND and OR operators of its own,
which give `-a` (AND) higher precedence than `-o` (OR).  In
addition, `$#` can only expand to an integer value, so quotes
are not required, and the whole thing can be written as:

    if test $# -ne 1 -a $# -ne 3 -o -z "$1"

(which is what I would do myself, unless I wanted a separate
error message for an empty "$1").  It's fine as is though.

Chris

Re: [PATCH 8/8] check-whitespace: detect if no base_commit is provided

[Georg Pfuetzenreuter]

On 7/8/24 12:18 PM, Chris Torek wrote:
> The `test` command has AND and OR operators of its own,
> which give `-a` (AND) higher precedence than `-o` (OR).  In
> addition, `$#` can only expand to an integer value, so quotes
> are not required, and the whole thing can be written as:
> 
>      if test $# -ne 1 -a $# -ne 3 -o -z "$1"
> 
> (which is what I would do myself, unless I wanted a separate
> error message for an empty "$1").  It's fine as is though.

Hi,

https://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html states

"The XSI extensions specifying the -a and -o binary primaries and the 
'(' and ')' operators have been marked obsolescent."

suggesting "&&" being preferred over "-a".

> Chris
> 

Re: [PATCH 8/8] check-whitespace: detect if no base_commit is provided

[Chris Torek]

On Mon, Jul 8, 2024 at 3:35 AM Georg Pfuetzenreuter <georg@syscid.com> wrote:
> https://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html states
>
> "The XSI extensions specifying the -a and -o binary primaries and the
> '(' and ')' operators have been marked obsolescent."
>
> suggesting "&&" being preferred over "-a".

That's annoying, I wonder why they did that. It does make
the "test" parser a bit tricky, especially with empty expansions,
but empty expansions are already a problem requiring careful
quoting in the first place...

Chris

Re: [PATCH 1/8] clang-format: indent preprocessor directives after hash

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> We do not have a rule around the indentation of preprocessor directives.
> This was also discussed on the list [1], noting how there is often
> inconsistency in the styling. While there was discussion, there was no
> conclusion around what is the preferred style here. One style being
> indenting after the hash:
>
>     #if FOO
>     #  if BAR
>     #    include <foo>
>     #  endif
>     #endif
>
> The other being before the hash:
>
>     #if FOO
>       #if BAR
>         #include <foo>
>       #endif
>     #endif
>
> Let's pick the former and add 'IndentPPDirectives: AfterHash' value to
> our '.clang-format'. There is no clear reason to pick one over the
> other, but it would definitely be nicer to be consistent.

When I experimented with reindenting the CPP directives in
<git-compat-util.h> [*], I think I saw an existing violation in an
early part of the file.  Outside the borrowed code in compat/, we
have these:

    $ git grep -e '^[ 	]\{1,\}#' master -- ':!compat/' \*.[ch]
    blame.c:	#define FINGERPRINT_FILE_THRESHOLD	10
    block-sha1/sha1.c:  #define setW(x, val) (*(volatile unsigned int *)&W(x) = (val))
    block-sha1/sha1.c:  #define setW(x, val) do { W(x) = (val); __asm__("":::"memory"); } while (0)
    block-sha1/sha1.c:  #define setW(x, val) (W(x) = (val))
    diff.h:	#define COLOR_MOVED_DEFAULT COLOR_MOVED_ZEBRA
    diff.h:	#define COLOR_MOVED_MIN_ALNUM_COUNT 20
    diff.h:	#define COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE (1<<5)
    diff.h:	#define COLOR_MOVED_WS_ERROR (1<<0)
    git-compat-util.h: #define GIT_GNUC_PREREQ(maj, min) 0
    pkt-line.c:	#define hex(a) (hexchar[(a) & 15])
    pkt-line.c:	#undef hex
    sha1dc/sha1.c:	#define sha1_load(m, t, temp)  { temp = m[t]; }
    sha1dc/sha1.c:	#define sha1_load(m, t, temp)  { temp = m[t]; sha1_bswap32(temp); }

Should we clean them up before we start adding these rules to the
file, especially if we plan to run the rules for stylistic check?
Otherwise wouldn't we see noises coming from the existing lines that
may dwarf the new ones, whose addition we want prevent?

If we were to run the check in CI to help contributors, I would
assume that you are arranging it to only complain about the lines
touched by the commits they are contributing, not about the existing
style violations.  This comment is not limited to the CPP directive
indentation but any other style rules .clang-format defines.

If we are not checking only for lines affected by commits being
contributed, then perhaps we should exclude compat/ from these
rules?

Thanks for working on this.

[Reference]

 * https://lore.kernel.org/git/xmqqjzim197j.fsf@gitster.g/

Re* [PATCH 1/8] clang-format: indent preprocessor directives after hash

[Junio C Hamano]

Subject: cpp: consistently write '#' for CPP directive at column 1

Besides it would get complaints from older compilers and look
strange to older eyes that were trained with such compilers, we will
be introducing a rule to be enforced to new code.  Update a few
existing violators before it happens.

The changes in git-compat-util.h and sha1dc/sha1.c are both about
indenting the body of conditional CPP directives, e.g.,

    (correct)           (incorrect)
    #if foo             #if foo
    # define bar         #define bar
    #endif              #endif

The #define/#undef in pkt-line.c were indented not because they were
inside a conditional compilation block inside #if/#endif but because
they were added inside a function.  As the function body is short,
we can move them out of the function and lose their indentation
without sacrificing the readability.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * So, if we decide to apply a preliminary clean-up to the codebase,
   it should look somethine like this.  I couldn't decide what we
   want to do with diff.h where the indentation is done for the same
   reason as how pkt-line.c temporarily defines hex(), but it is
   messier.

	diff --git i/diff.h w/diff.h
	index 9901c8ca8c..ecaa8ec49e 100644
	--- i/diff.h
	+++ w/diff.h
	@@ -386,12 +386,12 @@ struct diff_options {
	 		COLOR_MOVED_ZEBRA = 3,
	 		COLOR_MOVED_ZEBRA_DIM = 4,
	 	} color_moved;
	-	#define COLOR_MOVED_DEFAULT COLOR_MOVED_ZEBRA
	-	#define COLOR_MOVED_MIN_ALNUM_COUNT 20
	+#define COLOR_MOVED_DEFAULT COLOR_MOVED_ZEBRA
	+#define COLOR_MOVED_MIN_ALNUM_COUNT 20
	 
	 	/* XDF_WHITESPACE_FLAGS regarding block detection are set at 2, 3, 4 */
	-	#define COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE (1<<5)
	-	#define COLOR_MOVED_WS_ERROR (1<<0)
	+#define COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE (1<<5)
	+#define COLOR_MOVED_WS_ERROR (1<<0)
	 	unsigned color_moved_ws_handling;
	 
	 	struct repository *repo;

  I _think_ the right solution for the first half of this is
  actually to define the enum outside the structure and define
  _DEFAULT and _COUNT next to the enum, all at the top-level of the
  header file outside "struct diff_options" definition.

  But I am unsure how to clean up the latter and that is where I
  stopped.

 git-compat-util.h | 2 +-
 pkt-line.c        | 4 ++--
 sha1dc/sha1.c     | 4 ++--
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git c/git-compat-util.h w/git-compat-util.h
index ca7678a379..2cbd7c92b8 100644
--- c/git-compat-util.h
+++ w/git-compat-util.h
@@ -41,7 +41,7 @@ struct strbuf;
 # define GIT_GNUC_PREREQ(maj, min) \
 	((__GNUC__ << 16) + __GNUC_MINOR__ >= ((maj) << 16) + (min))
 #else
- #define GIT_GNUC_PREREQ(maj, min) 0
+# define GIT_GNUC_PREREQ(maj, min) 0
 #endif
 
 
diff --git c/pkt-line.c w/pkt-line.c
index 24479eae4d..30044b2e76 100644
--- c/pkt-line.c
+++ w/pkt-line.c
@@ -132,17 +132,17 @@ void packet_buf_delim(struct strbuf *buf)
 	strbuf_add(buf, "0001", 4);
 }
 
+#define hex(a) (hexchar[(a) & 15])
 void set_packet_header(char *buf, int size)
 {
 	static char hexchar[] = "0123456789abcdef";
 
-	#define hex(a) (hexchar[(a) & 15])
 	buf[0] = hex(size >> 12);
 	buf[1] = hex(size >> 8);
 	buf[2] = hex(size >> 4);
 	buf[3] = hex(size);
-	#undef hex
 }
+#undef hex
 
 static void format_packet(struct strbuf *out, const char *prefix,
 			  const char *fmt, va_list args)
diff --git c/sha1dc/sha1.c w/sha1dc/sha1.c
index f993ef9c69..e82e05425e 100644
--- c/sha1dc/sha1.c
+++ w/sha1dc/sha1.c
@@ -139,9 +139,9 @@
 #define sha1_mix(W, t)  (rotate_left(W[t - 3] ^ W[t - 8] ^ W[t - 14] ^ W[t - 16], 1))
 
 #ifdef SHA1DC_BIGENDIAN
-	#define sha1_load(m, t, temp)  { temp = m[t]; }
+#	define sha1_load(m, t, temp)  { temp = m[t]; }
 #else
-	#define sha1_load(m, t, temp)  { temp = m[t]; sha1_bswap32(temp); }
+#	define sha1_load(m, t, temp)  { temp = m[t]; sha1_bswap32(temp); }
 #endif
 
 #define sha1_store(W, t, x)	*(volatile uint32_t *)&W[t] = x

Re: [PATCH 4/8] clang-format: replace deprecated option with 'SpacesInParens'

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> Replace the deprecated options 'SpacesInParentheses' and
> 'SpaceInEmptyParentheses' with the newer 'SpacesInParens' option. The
> usage is the same.

I do not follow clang and clang-format development; updating to
avoid deprecated features is a welcome change, as long as the
replacing feature isn't too new (like, "introduced only 3 years
ago").

Thanks.

> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  .clang-format | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/.clang-format b/.clang-format
> index 4c9751a9db..914254a29b 100644
> --- a/.clang-format
> +++ b/.clang-format
> @@ -134,8 +134,6 @@ SpaceBeforeAssignmentOperators: true
>  # }
>  SpaceBeforeParens: ControlStatements
>  
> -# Don't insert spaces inside empty '()'
> -SpaceInEmptyParentheses: false
>  
>  # The number of spaces before trailing line comments (// - comments).
>  # This does not affect trailing block comments (/* - comments).
> @@ -149,9 +147,10 @@ SpacesInCStyleCastParentheses: false
>  # var arr = [1, 2, 3];    not    var arr = [ 1, 2, 3 ];
>  SpacesInContainerLiterals: false
>  
> -# Don't insert spaces after '(' or before ')'
> -# f(arg);    not    f( arg );
> -SpacesInParentheses: false
> +SpacesInParens: Custom
> +SpacesInParensOptions:
> +  # Don't insert spaces inside empty '()'
> +  InEmptyParentheses: false
>  
>  # Don't insert spaces after '[' or before ']'
>  # int a[5];    not    int a[ 5 ];

Re: [PATCH 3/8] clang-format: ensure files end with newlines

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> All our source files end with a newline, let's formalize in
> '.clang-format'.

Makes sense.

>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  .clang-format | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/.clang-format b/.clang-format
> index 803b274dd5..4c9751a9db 100644
> --- a/.clang-format
> +++ b/.clang-format
> @@ -106,6 +106,9 @@ IndentCaseLabels: false
>  # #endif
>  IndentPPDirectives: AfterHash
>  
> +# Insert a newline at end of file if missing
> +InsertNewlineAtEOF: true
> +
>  # Don't indent a function definition or declaration if it is wrapped after the
>  # type
>  IndentWrappedFunctionNames: false

Re: [PATCH 5/8] clang-format: avoid braces on simple single-statement bodies

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> Set the 'RemoveBracesLLVM' to 'true' which ensures that we avoid curly
> braces for single-statement bodies in conditional blocks.

Hmph, two warnings in its documentation [*] sound ominous, especially
the second one that says:

    Warning

    Setting this option to true could lead to incorrect code
    formatting due to clang-format’s lack of complete semantic
    information. As such, extra care should be taken to review code
    changes made by this option.

which implies it may not necessarily a good idea to add to
automation without telling contributors that they may get hit with a
false positive (or incorrect rewrite).

Reading from the examples in that documentation page, it was unclear
how it would handle if/else if/.../else cascade where not all branches
are multi-statement blocks, e.g.,

	if (A) {
		do_A_thing();
	} else if (B) {
		do_B_thing();
	} else {
		do_C_things();
		do_other_things();
	}

but looking around I am getting a feeling that the tool would do the
right thing, i.e., to match our preference that is to use {braces}
around all branches, if I am not mistaken.


[Reference]

 * https://releases.llvm.org/16.0.0/tools/clang/docs/ClangFormatStyleOptions.html#:~:text=RemoveBracesLLVM

> +# Remove optional braces of control statements (if, else, for, and while)
> +# according to the LLVM coding style
> +# This avoids braces on simple single-statement bodies of statements.

"... but keeps braces if one side of if/else if/.../else cascade has
multi-statement body."

> +RemoveBracesLLVM: true

Re: [PATCH 6/8] clang-format: formalize some of the spacing rules

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> There are some spacing rules that we follow in the project and it makes
> sen to formalize them:
> * Ensure there is no space inserted after the logical not '!' operator.

Shouldn't the rule be more like "no space between any single operand
prefix or postfix operator and its operand"?  "foo++", "--foo", "~0"
are the examples that come to my mind.

> * Ensure there is no space before the case statement's color.

"color" -> "colon".

> * Ensure there is no space before the first bracket '[' of an array.
> * Ensure there is no space in empty blocks.

Hmph, I actually thought we preferred to be more explicit, using

	if (foo)
		; /* nothing */

instead of any of

	if (foo) {}
	if (foo) { }
	if (foo) { ; }
	if (foo) { ; /* nothing */ }

to write an empty statement.

> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  .clang-format | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/.clang-format b/.clang-format
> index 1a5f0c9046..05036f610b 100644
> --- a/.clang-format
> +++ b/.clang-format
> @@ -126,11 +126,18 @@ RemoveBracesLLVM: true
>  # x = (int32)y;    not    x = (int32) y;
>  SpaceAfterCStyleCast: false
>  
> +# No space is inserted after the logical not operator
> +SpaceAfterLogicalNot: false
> +
>  # Insert spaces before and after assignment operators
>  # int a = 5;    not    int a=5;
>  # a += 42;             a+=42;
>  SpaceBeforeAssignmentOperators: true
>  
> +# Spaces will be removed before case colon.
> +# case 1: break;    not     case 1 : break;
> +SpaceBeforeCaseColon: false
> +
>  # Put a space before opening parentheses only after control statement keywords.
>  # void f() {
>  #   if (true) {
> @@ -139,6 +146,13 @@ SpaceBeforeAssignmentOperators: true
>  # }
>  SpaceBeforeParens: ControlStatements
>  
> +# No space before first '[' in arrays
> +# int a[5][5];     not      int a [5][5];
> +SpaceBeforeSquareBrackets: false
> +
> +# No space will be inserted into {}
> +# while (true) {}    not    while (true) { }
> +SpaceInEmptyBlock: false
>  
>  # The number of spaces before trailing line comments (// - comments).
>  # This does not affect trailing block comments (/* - comments).

Re: [PATCH 6/8] clang-format: formalize some of the spacing rules

[Justin Tobler]

On 24/07/08 11:23AM, Karthik Nayak wrote:
> There are some spacing rules that we follow in the project and it makes
> sen to formalize them:
> * Ensure there is no space inserted after the logical not '!' operator.
> * Ensure there is no space before the case statement's color.

s/color/colon

> * Ensure there is no space before the first bracket '[' of an array.
> * Ensure there is no space in empty blocks.

Re: [PATCH 8/8] check-whitespace: detect if no base_commit is provided

[Karthik Nayak]

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

Chris Torek <chris.torek@gmail.com> writes:

> On Mon, Jul 8, 2024 at 3:35 AM Georg Pfuetzenreuter <georg@syscid.com> wrote:
>> https://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html states
>>
>> "The XSI extensions specifying the -a and -o binary primaries and the
>> '(' and ')' operators have been marked obsolescent."
>>
>> suggesting "&&" being preferred over "-a".
>
> That's annoying, I wonder why they did that. It does make
> the "test" parser a bit tricky, especially with empty expansions,
> but empty expansions are already a problem requiring careful
> quoting in the first place...
>
> Chris

Thanks both, I think with this, it makes sense to keep it as is.

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

Re: [PATCH 7/8] ci: run style check on GitHub and GitLab

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> We don't run style checks on our CI, even though we have a
> '.clang-format' setup in the repository. Let's add one, the job will
> validate only against the new commits added and will only run on merge
> requests.

I hope "against new commits" means what I think it does ;-) I am
worried about the case where a new commit touches a file that has
existing style violations but the commit does not make the situation
any worse at all.

OK, "git clang-format" is used to for this thing and it is designed
to address only lines that differ, so hopefully that would be good.

> Since we're introducing it for the first time, let's allow
> this job to fail, so we can validate if this is useful and eventually
> enforce it.

Very good idea.

> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  .github/workflows/check-style.yml | 29 +++++++++++++++++++++++++++++
>  .gitlab-ci.yml                    | 12 ++++++++++++
>  ci/install-dependencies.sh        |  2 +-
>  ci/run-style-check.sh             |  8 ++++++++
>  4 files changed, 50 insertions(+), 1 deletion(-)
>  create mode 100644 .github/workflows/check-style.yml
>  create mode 100755 ci/run-style-check.sh
>
> diff --git a/.github/workflows/check-style.yml b/.github/workflows/check-style.yml
> new file mode 100644
> index 0000000000..27276dfe5e
> --- /dev/null
> +++ b/.github/workflows/check-style.yml
> @@ -0,0 +1,29 @@
> +name: check-style
> +
> +# Get the repository with all commits to ensure that we can analyze
> +# all of the commits contributed via the Pull Request.
> +
> +on:
> +  pull_request:
> +    types: [opened, synchronize]
> +
> +# Avoid unnecessary builds. Unlike the main CI jobs, these are not
> +# ci-configurable (but could be).
> +concurrency:
> +  group: ${{ github.workflow }}-${{ github.ref }}
> +  cancel-in-progress: true
> +
> +jobs:
> +  check-style:
> +    runs-on: ubuntu-latest
> +    steps:
> +    - uses: actions/checkout@v4
> +      with:
> +        fetch-depth: 0
> +
> +    - name: git clang-format
> +      continue-on-error: true
> +      id: check_out
> +      run: |
> +        ./ci/run-style-check.sh \
> +          "${{github.event.pull_request.base.sha}}"
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index 37b991e080..65fd261e5e 100644
> --- a/.gitlab-ci.yml
> +++ b/.gitlab-ci.yml
> @@ -123,6 +123,18 @@ check-whitespace:
>    rules:
>      - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
>  
> +check-style:
> +  image: ubuntu:latest
> +  allow_failure: true
> +  variables:
> +    CC: clang
> +  before_script:
> +    - ./ci/install-dependencies.sh
> +  script:
> +    - ./ci/run-style-check.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
> +  rules:
> +    - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
> +
>  documentation:
>    image: ubuntu:latest
>    variables:
> diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
> index 6ec0f85972..46fe12a690 100755
> --- a/ci/install-dependencies.sh
> +++ b/ci/install-dependencies.sh
> @@ -43,7 +43,7 @@ ubuntu-*)
>  		make libssl-dev libcurl4-openssl-dev libexpat-dev wget sudo default-jre \
>  		tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl \
>  		libemail-valid-perl libio-pty-perl libio-socket-ssl-perl libnet-smtp-ssl-perl libdbd-sqlite3-perl libcgi-pm-perl \
> -		${CC_PACKAGE:-${CC:-gcc}} $PYTHON_PACKAGE
> +		${CC_PACKAGE:-${CC:-gcc}} $PYTHON_PACKAGE clang-format
>  
>  	mkdir --parents "$CUSTOM_PATH"
>  	wget --quiet --directory-prefix="$CUSTOM_PATH" \
> diff --git a/ci/run-style-check.sh b/ci/run-style-check.sh
> new file mode 100755
> index 0000000000..9d4c4089c1
> --- /dev/null
> +++ b/ci/run-style-check.sh
> @@ -0,0 +1,8 @@
> +#!/usr/bin/env sh

Under ci/ hierarchy we are very inconsistent.  Most use the bog
standard "#!/bin/sh" (which is my preference by the way), but
see what we have here right now:

        $ git grep -e '^#![a-z/]*/bin/[a-z]*sh' -e '^#![a-z/]*bin/env ' ci |
          sort -t: -k2
        ci/check-directional-formatting.bash:#!/bin/bash
        ci/install-dependencies.sh:#!/bin/sh
        ci/make-test-artifacts.sh:#!/bin/sh
        ci/mount-fileshare.sh:#!/bin/sh
        ci/print-test-failures.sh:#!/bin/sh
        ci/run-build-and-minimal-fuzzers.sh:#!/bin/sh
        ci/run-build-and-tests.sh:#!/bin/sh
        ci/run-docker-build.sh:#!/bin/sh
        ci/run-docker.sh:#!/bin/sh
        ci/run-static-analysis.sh:#!/bin/sh
        ci/run-test-slice.sh:#!/bin/sh
        ci/util/extract-trash-dirs.sh:#!/bin/sh
        ci/check-whitespace.sh:#!/usr/bin/env bash
        ci/test-documentation.sh:#!/usr/bin/env bash

Unless you have a strong reason to suspect that in your CI
environment /bin/sh is an unusuably broken shell, please do not
spread the inconsistencies.

I think the consensus from the last discussion we had was to allow
scripts that rely on bash-isms to say "#!/usr/bin/env bash" because
we know /bin/sh can legitimately be not bash and we assume bash may
not be installed as /bin/bash.  As all of them would run in the CI
environment that we have some control over what required packages
are installed at what path, it is OK to assume that "bash" would be
found on the $PATH by using /usr/bin/env (but it does assume
everybody installs "env" there, not /bin/env or /usr/local/bin/env,
with a bit of chicken-and-egg issue).

> +#
> +# Perform style check
> +#
> +
> +baseCommit=$1
> +
> +git clang-format --style file --diff --extensions c,h "$baseCommit"

OK, "git clang-format" compares the working tree with the named
commit, so if we have the tip of the topic branch proposed to be
merged checked out and compare it with the base commit of the topic,
that would give us exactly what we want.  Nice.

Thanks.

Re: [PATCH 8/8] check-whitespace: detect if no base_commit is provided

[Junio C Hamano]

Chris Torek <chris.torek@gmail.com> writes:

> On Mon, Jul 8, 2024 at 3:35 AM Georg Pfuetzenreuter <georg@syscid.com> wrote:
>> https://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html states
>>
>> "The XSI extensions specifying the -a and -o binary primaries and the
>> '(' and ')' operators have been marked obsolescent."
>>
>> suggesting "&&" being preferred over "-a".
>
> That's annoying, I wonder why they did that.

Consult Documentation/CodingGuidelines?

 - We do not write our "test" command with "-a" and "-o" and use "&&"
   or "||" to concatenate multiple "test" commands instead, because
   the use of "-a/-o" is often error-prone.  E.g.

     test -n "$x" -a "$a" = "$b"

   is buggy and breaks when $x is "=", but

     test -n "$x" && test "$a" = "$b"

   does not have such a problem.

Re: [PATCH 6/8] clang-format: formalize some of the spacing rules

[Karthik Nayak]

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

Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> There are some spacing rules that we follow in the project and it makes
>> sen to formalize them:
>> * Ensure there is no space inserted after the logical not '!' operator.
>
> Shouldn't the rule be more like "no space between any single operand
> prefix or postfix operator and its operand"?  "foo++", "--foo", "~0"
> are the examples that come to my mind.
>

The rule here is SpaceAfterLogicalNot [1], is specific to logical not
operator. Unfortunately I couldn't find a general rule for unary
operators. That would be very useful indeed.

>> * Ensure there is no space before the case statement's color.
>
> "color" -> "colon".
>

Will fix, thanks!

>> * Ensure there is no space before the first bracket '[' of an array.
>> * Ensure there is no space in empty blocks.
>
> Hmph, I actually thought we preferred to be more explicit, using
>
> 	if (foo)
> 		; /* nothing */
>
> instead of any of
>
> 	if (foo) {}
> 	if (foo) { }
> 	if (foo) { ; }
> 	if (foo) { ; /* nothing */ }
>
> to write an empty statement.
>

Yup, that is correct. This rule doesn't state that we need to use 'if (foo) {}'
over the more explicit format. It only states that if we do create an
empty block without statements, the rule is to have no spaces.

We only have a few instances of this in our code.

[1]: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#spaceafterlogicalnot

[snip]

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

Re: [PATCH 6/8] clang-format: formalize some of the spacing rules

[Karthik Nayak]

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

Justin Tobler <jltobler@gmail.com> writes:

> On 24/07/08 11:23AM, Karthik Nayak wrote:
>> There are some spacing rules that we follow in the project and it makes
>> sen to formalize them:
>> * Ensure there is no space inserted after the logical not '!' operator.
>> * Ensure there is no space before the case statement's color.
>
> s/color/colon
>

Thanks, will fix in the next version!

>> * Ensure there is no space before the first bracket '[' of an array.
>> * Ensure there is no space in empty blocks.

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

Re: [PATCH 8/8] check-whitespace: detect if no base_commit is provided

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> The 'check-whitespace' CI script exists gracefully if no base commit is

"exists" -> "exits"

> provided or if an invalid revision is provided...
> ...
> Let's fix the variable used in the GitLab CI. Let's also add a check for
> incorrect base_commit in the 'check-whitespace.sh' script. While here,
> fix a small typo too.
>
> [1]: https://docs.gitlab.com/ee/ci/variables/predefined_variables.html#predefined-variables-for-merge-request-pipelines
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  .gitlab-ci.yml         |  2 +-
>  ci/check-whitespace.sh | 13 ++++++++++---
>  2 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index 65fd261e5e..36199893d8 100644
> --- a/.gitlab-ci.yml
> +++ b/.gitlab-ci.yml
> @@ -119,7 +119,7 @@ check-whitespace:
>    before_script:
>      - ./ci/install-dependencies.sh
>    script:
> -    - ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
> +    - ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
>    rules:
>      - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
>  
> diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh
> index db399097a5..ab023f9519 100755
> --- a/ci/check-whitespace.sh
> +++ b/ci/check-whitespace.sh
> @@ -9,12 +9,19 @@ baseCommit=$1
>  outputFile=$2
>  url=$3
>  
> -if test "$#" -ne 1 && test "$#" -ne 3
> +if { test "$#" -ne 1 && test "$#" -ne 3; } || test -z "$1"

You can just add || test -z "$1" after the existing one, making the
thing A && B || C which evaulates left to right with the same
precedence for && and ||.

>  then
>  	echo "USAGE: $0 <BASE_COMMIT> [<OUTPUT_FILE> <URL>]"
>  	exit 1
>  fi
>  
> +gitLogOutput=$(git log --check --pretty=format:"---% h% s" "${baseCommit}"..)

That is a large string to hold in a variable for a potentially large
series with lots of breakages.  I didn't quite read the reasoning
behind this change in the proposed log message.  Under what
condition do you expect the command to exit with non-zero status?
$basecommit being a non-empty string but does not name a valid
commit object or something, in which case shouldn't "git log
--oneline $baseCommit.."  or something simpler should suffice?

> +if test $? -ne 0
> +then
> +	echo -n $gitLogOutput

What is "-n" doing here?  Why are you squashing run of spaces in the
$gitLogOutput variable into a space by not "quoting" inside a dq-pair?

> +	exit 1
> +fi

Looking for "--check" in

	$ git log --help

tells me that the command exits with non-zero status if problems are
found, so wouldn't that mean the cases with problems always exit
early, bypassing the while loop with full of bash-ism that comes
after this block?

>  problems=()
>  commit=
>  commitText=
> @@ -58,7 +65,7 @@ do
>  		echo "${dash} ${sha} ${etc}"
>  		;;
>  	esac
> -done <<< "$(git log --check --pretty=format:"---% h% s" "${baseCommit}"..)"
> +done <<< "$gitLogOutput"
>  
>  if test ${#problems[*]} -gt 0
>  then
> @@ -67,7 +74,7 @@ then
>  		goodParent=${baseCommit: 0:7}
>  	fi
>  
> -	echo "A whitespace issue was found in onen of more of the commits."
> +	echo "A whitespace issue was found in one of more of the commits."
>  	echo "Run the following command to resolve whitespace issues:"
>  	echo "git rebase --whitespace=fix ${goodParent}"

Re: [PATCH 8/8] check-whitespace: detect if no base_commit is provided

[Justin Tobler]

On 24/07/08 11:23AM, Karthik Nayak wrote:
> The 'check-whitespace' CI script exists gracefully if no base commit is
> provided or if an invalid revision is provided. This is not good because
> if a particular CI provides an incorrect base_commit, it would fail
> successfully.

s/exists/exits

If no base commit is provided, we already fail. Here is an example
GitLab CI job demonstrating this:
https://gitlab.com/gitlab-org/git/-/jobs/7289543498#L2370

If the base commit does not exist though, it currently prints that error occured
but still exits with 0. Makes sense to update and fail the job accordingly.

> 
> This is exactly the case with the GitLab CI. The CI is using the
> "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA" variable to get the base commit
> SHA, but variable is only defined for _merged_ pipelines. So it is empty
> for regular pipelines [1]. This should've failed the check-whitespace
> job.

The CI for this project is configured to use merged pipelines. So 
$CI_MERGE_REQUEST_TARGET_BRANCH_SHA is defined. The downside with using 
$CI_MERGE_REQUEST_DIFF_BASE_SHA is that it will include other commits in
the check that are not part of the MR, but are included in the merge for
merge pipelines. With this change, the job can now fail due to unrelated
changes.

If we feel inclined to also support regular pipelines, one option would
be to simply fallback to $CI_MERGE_REQUEST_DIFF_BASE_SHA if a merge
pipeline is not in use.

GitLab CI pipeline showing $CI_MERGE_REQUEST_TARGET_BRANCH_SHA defined:
https://gitlab.com/gitlab-org/git/-/jobs/7289331488#L2371

> 
> Let's fix the variable used in the GitLab CI. Let's also add a check for
> incorrect base_commit in the 'check-whitespace.sh' script. While here,
> fix a small typo too.
> 
> [1]: https://docs.gitlab.com/ee/ci/variables/predefined_variables.html#predefined-variables-for-merge-request-pipelines
> 
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  .gitlab-ci.yml         |  2 +-
>  ci/check-whitespace.sh | 13 ++++++++++---
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index 65fd261e5e..36199893d8 100644
> --- a/.gitlab-ci.yml
> +++ b/.gitlab-ci.yml
> @@ -119,7 +119,7 @@ check-whitespace:
>    before_script:
>      - ./ci/install-dependencies.sh
>    script:
> -    - ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
> +    - ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
>    rules:
>      - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
>  
> diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh
> index db399097a5..ab023f9519 100755
> --- a/ci/check-whitespace.sh
> +++ b/ci/check-whitespace.sh
> @@ -9,12 +9,19 @@ baseCommit=$1
>  outputFile=$2
>  url=$3
>  
> -if test "$#" -ne 1 && test "$#" -ne 3
> +if { test "$#" -ne 1 && test "$#" -ne 3; } || test -z "$1"

I might be misunderstanding, but this additional check seems redundant to me.

>  then
>  	echo "USAGE: $0 <BASE_COMMIT> [<OUTPUT_FILE> <URL>]"
>  	exit 1
>  fi
>  
> +gitLogOutput=$(git log --check --pretty=format:"---% h% s" "${baseCommit}"..)
> +if test $? -ne 0
> +then
> +	echo -n $gitLogOutput
> +	exit 1
> +fi
> +
>  problems=()
>  commit=
>  commitText=
> @@ -58,7 +65,7 @@ do
>  		echo "${dash} ${sha} ${etc}"
>  		;;
>  	esac
> -done <<< "$(git log --check --pretty=format:"---% h% s" "${baseCommit}"..)"
> +done <<< "$gitLogOutput"
>  
>  if test ${#problems[*]} -gt 0
>  then
> @@ -67,7 +74,7 @@ then
>  		goodParent=${baseCommit: 0:7}
>  	fi
>  
> -	echo "A whitespace issue was found in onen of more of the commits."
> +	echo "A whitespace issue was found in one of more of the commits."
There is another preexisting typo:

s/one of/one or/

>  	echo "Run the following command to resolve whitespace issues:"
>  	echo "git rebase --whitespace=fix ${goodParent}"
>  
> -- 
> 2.45.1
> 

Re: [PATCH 7/8] ci: run style check on GitHub and GitLab

[Justin Tobler]

On 24/07/08 11:23AM, Karthik Nayak wrote:
> We don't run style checks on our CI, even though we have a
> '.clang-format' setup in the repository. Let's add one, the job will
> validate only against the new commits added and will only run on merge
> requests. Since we're introducing it for the first time, let's allow
> this job to fail, so we can validate if this is useful and eventually
> enforce it.

[snip]

> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index 37b991e080..65fd261e5e 100644
> --- a/.gitlab-ci.yml
> +++ b/.gitlab-ci.yml
> @@ -123,6 +123,18 @@ check-whitespace:
>    rules:
>      - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
>  
> +check-style:
> +  image: ubuntu:latest
> +  allow_failure: true
> +  variables:
> +    CC: clang
> +  before_script:
> +    - ./ci/install-dependencies.sh
> +  script:
> +    - ./ci/run-style-check.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA"

One downside to using $CI_MERGE_REQUEST_DIFF_BASE_SHA is that for GitLab
merge pipeines, commits from the merge that are not part of the MR
changes are also included. This could lead to somewhat confusing
failures.

Example failure occuring on this patch series:
https://gitlab.com/gitlab-org/git/-/jobs/7284442220

It might be best to use $CI_MERGE_REQUEST_TARGET_BRANCH_SHA instead.

> +  rules:
> +    - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
> +

Re: [PATCH 5/8] clang-format: avoid braces on simple single-statement bodies

[Karthik Nayak]

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

Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Set the 'RemoveBracesLLVM' to 'true' which ensures that we avoid curly
>> braces for single-statement bodies in conditional blocks.
>
> Hmph, two warnings in its documentation [*] sound ominous, especially
> the second one that says:
>
>     Warning
>
>     Setting this option to true could lead to incorrect code
>     formatting due to clang-format’s lack of complete semantic
>     information. As such, extra care should be taken to review code
>     changes made by this option.
>
> which implies it may not necessarily a good idea to add to
> automation without telling contributors that they may get hit with a
> false positive (or incorrect rewrite).
>

Agreed on this one. I'm a bit skeptical to be honest too. I think I
should have added information about the warning in the commit too. I
will for next round. Overall, this also contributes to the reason why I
decided these CI jobs need to be allowed to fail.

>
> Reading from the examples in that documentation page, it was unclear
> how it would handle if/else if/.../else cascade where not all branches
> are multi-statement blocks, e.g.,
>
> 	if (A) {
> 		do_A_thing();
> 	} else if (B) {
> 		do_B_thing();
> 	} else {
> 		do_C_things();
> 		do_other_things();
> 	}
>
> but looking around I am getting a feeling that the tool would do the
> right thing, i.e., to match our preference that is to use {braces}
> around all branches, if I am not mistaken.
>

Yup, that was my understanding and what I could see from some quick
trials that I did too.

It would be a great win to have this though, because it is one of the
things that always get me.

>
> [Reference]
>
>  * https://releases.llvm.org/16.0.0/tools/clang/docs/ClangFormatStyleOptions.html#:~:text=RemoveBracesLLVM
>
>> +# Remove optional braces of control statements (if, else, for, and while)
>> +# according to the LLVM coding style
>> +# This avoids braces on simple single-statement bodies of statements.
>
> "... but keeps braces if one side of if/else if/.../else cascade has
> multi-statement body."
>

Makes sense, will add.

>> +RemoveBracesLLVM: true

Overall I must say, I'd be okay if this patch is also dropped from this
series.

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

Re: [PATCH 1/8] clang-format: indent preprocessor directives after hash

[Karthik Nayak]

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

Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> We do not have a rule around the indentation of preprocessor directives.
>> This was also discussed on the list [1], noting how there is often
>> inconsistency in the styling. While there was discussion, there was no
>> conclusion around what is the preferred style here. One style being
>> indenting after the hash:
>>
>>     #if FOO
>>     #  if BAR
>>     #    include <foo>
>>     #  endif
>>     #endif
>>
>> The other being before the hash:
>>
>>     #if FOO
>>       #if BAR
>>         #include <foo>
>>       #endif
>>     #endif
>>
>> Let's pick the former and add 'IndentPPDirectives: AfterHash' value to
>> our '.clang-format'. There is no clear reason to pick one over the
>> other, but it would definitely be nicer to be consistent.
>
> When I experimented with reindenting the CPP directives in
> <git-compat-util.h> [*], I think I saw an existing violation in an
> early part of the file.  Outside the borrowed code in compat/, we
> have these:
>
>     $ git grep -e '^[ 	]\{1,\}#' master -- ':!compat/' \*.[ch]
>     blame.c:	#define FINGERPRINT_FILE_THRESHOLD	10
>     block-sha1/sha1.c:  #define setW(x, val) (*(volatile unsigned int *)&W(x) = (val))
>     block-sha1/sha1.c:  #define setW(x, val) do { W(x) = (val); __asm__("":::"memory"); } while (0)
>     block-sha1/sha1.c:  #define setW(x, val) (W(x) = (val))
>     diff.h:	#define COLOR_MOVED_DEFAULT COLOR_MOVED_ZEBRA
>     diff.h:	#define COLOR_MOVED_MIN_ALNUM_COUNT 20
>     diff.h:	#define COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE (1<<5)
>     diff.h:	#define COLOR_MOVED_WS_ERROR (1<<0)
>     git-compat-util.h: #define GIT_GNUC_PREREQ(maj, min) 0
>     pkt-line.c:	#define hex(a) (hexchar[(a) & 15])
>     pkt-line.c:	#undef hex
>     sha1dc/sha1.c:	#define sha1_load(m, t, temp)  { temp = m[t]; }
>     sha1dc/sha1.c:	#define sha1_load(m, t, temp)  { temp = m[t]; sha1_bswap32(temp); }
>
> Should we clean them up before we start adding these rules to the
> file, especially if we plan to run the rules for stylistic check?
> Otherwise wouldn't we see noises coming from the existing lines that
> may dwarf the new ones, whose addition we want prevent?
>

Making syntax changes just so the rule works was something I did
consider, but I avoided it mostly because the CI only applies to the
changes made and not pre-existing files.

This also allows us to apply the boy scout rule and cleanup as we go.

> If we were to run the check in CI to help contributors, I would
> assume that you are arranging it to only complain about the lines
> touched by the commits they are contributing, not about the existing
> style violations.  This comment is not limited to the CPP directive
> indentation but any other style rules .clang-format defines.
>

Yes exactly, the usage of 'git-clang-format' allows us to only check for
lines changed.

> If we are not checking only for lines affected by commits being
> contributed, then perhaps we should exclude compat/ from these
> rules?
>
> Thanks for working on this.
>
> [Reference]
>
>  * https://lore.kernel.org/git/xmqqjzim197j.fsf@gitster.g/

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

Re: [PATCH 6/8] clang-format: formalize some of the spacing rules

[Eric Sunshine]

On Mon, Jul 8, 2024 at 5:24 AM Karthik Nayak <karthik.188@gmail.com> wrote:
> There are some spacing rules that we follow in the project and it makes
> sen to formalize them:

Since nobody else pointed it out: s/sen/sense/

Re: [PATCH 7/8] ci: run style check on GitHub and GitLab

[Karthik Nayak]

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

Junio C Hamano <gitster@pobox.com> writes:
[snip]
>> diff --git a/ci/run-style-check.sh b/ci/run-style-check.sh
>> new file mode 100755
>> index 0000000000..9d4c4089c1
>> --- /dev/null
>> +++ b/ci/run-style-check.sh
>> @@ -0,0 +1,8 @@
>> +#!/usr/bin/env sh
>
> Under ci/ hierarchy we are very inconsistent.  Most use the bog
> standard "#!/bin/sh" (which is my preference by the way), but
> see what we have here right now:
>
>         $ git grep -e '^#![a-z/]*/bin/[a-z]*sh' -e '^#![a-z/]*bin/env ' ci |
>           sort -t: -k2
>         ci/check-directional-formatting.bash:#!/bin/bash
>         ci/install-dependencies.sh:#!/bin/sh
>         ci/make-test-artifacts.sh:#!/bin/sh
>         ci/mount-fileshare.sh:#!/bin/sh
>         ci/print-test-failures.sh:#!/bin/sh
>         ci/run-build-and-minimal-fuzzers.sh:#!/bin/sh
>         ci/run-build-and-tests.sh:#!/bin/sh
>         ci/run-docker-build.sh:#!/bin/sh
>         ci/run-docker.sh:#!/bin/sh
>         ci/run-static-analysis.sh:#!/bin/sh
>         ci/run-test-slice.sh:#!/bin/sh
>         ci/util/extract-trash-dirs.sh:#!/bin/sh
>         ci/check-whitespace.sh:#!/usr/bin/env bash
>         ci/test-documentation.sh:#!/usr/bin/env bash
>
> Unless you have a strong reason to suspect that in your CI
> environment /bin/sh is an unusuably broken shell, please do not
> spread the inconsistencies.
>
> I think the consensus from the last discussion we had was to allow
> scripts that rely on bash-isms to say "#!/usr/bin/env bash" because
> we know /bin/sh can legitimately be not bash and we assume bash may
> not be installed as /bin/bash.  As all of them would run in the CI
> environment that we have some control over what required packages
> are installed at what path, it is OK to assume that "bash" would be
> found on the $PATH by using /usr/bin/env (but it does assume
> everybody installs "env" there, not /bin/env or /usr/local/bin/env,
> with a bit of chicken-and-egg issue).
>

I must admit, I didn't put any thought into this while writing. The
usage of '/usr/bin/env' is mostly muscle memory and since I didn't need
any bash-isms, I defaulted to /bin/sh.

I'll change it, thanks!

>> +#
>> +# Perform style check
>> +#
>> +
>> +baseCommit=$1
>> +
>> +git clang-format --style file --diff --extensions c,h "$baseCommit"
>
> OK, "git clang-format" compares the working tree with the named
> commit, so if we have the tip of the topic branch proposed to be
> merged checked out and compare it with the base commit of the topic,
> that would give us exactly what we want.  Nice.
>
> Thanks.

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

Re: [PATCH 7/8] ci: run style check on GitHub and GitLab

[Karthik Nayak]

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

Justin Tobler <jltobler@gmail.com> writes:

> On 24/07/08 11:23AM, Karthik Nayak wrote:
>> We don't run style checks on our CI, even though we have a
>> '.clang-format' setup in the repository. Let's add one, the job will
>> validate only against the new commits added and will only run on merge
>> requests. Since we're introducing it for the first time, let's allow
>> this job to fail, so we can validate if this is useful and eventually
>> enforce it.
>
> [snip]
>
>> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
>> index 37b991e080..65fd261e5e 100644
>> --- a/.gitlab-ci.yml
>> +++ b/.gitlab-ci.yml
>> @@ -123,6 +123,18 @@ check-whitespace:
>>    rules:
>>      - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
>>
>> +check-style:
>> +  image: ubuntu:latest
>> +  allow_failure: true
>> +  variables:
>> +    CC: clang
>> +  before_script:
>> +    - ./ci/install-dependencies.sh
>> +  script:
>> +    - ./ci/run-style-check.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
>
> One downside to using $CI_MERGE_REQUEST_DIFF_BASE_SHA is that for GitLab
> merge pipeines, commits from the merge that are not part of the MR
> changes are also included. This could lead to somewhat confusing
> failures.
>

I'm not sure I follow.

> Example failure occuring on this patch series:
> https://gitlab.com/gitlab-org/git/-/jobs/7284442220
>

If you notice this job, it points to the commit: 1c6551488, and the
parent commit of that commit is: 614dff2011.

The parent commit [1] is a test commit I added to check the failures. So
isn't this working as expected?

> It might be best to use $CI_MERGE_REQUEST_TARGET_BRANCH_SHA instead.
>

I actually started with $CI_MERGE_REQUEST_TARGET_BRANCH_SHA, it didn't
work, because the value was undefined.

See: https://gitlab.com/gitlab-org/git/-/jobs/7283724903

This is why I also decided to fix and change the whitespace check.

>> +  rules:
>> +    - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
>> +

[1]: https://gitlab.com/gitlab-org/git/-/commit/614dff20119aa325661424a9fcef552e242a95d9

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

Re* [PATCH 7/8] ci: run style check on GitHub and GitLab

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I think the consensus from the last discussion we had was to allow
> scripts that rely on bash-isms to say "#!/usr/bin/env bash" because
> we know /bin/sh can legitimately be not bash and we assume bash may
> not be installed as /bin/bash.

Let's do this before we forget.

------- >8 ------------- >8 ------------- >8 -------
Subject: [PATCH] ci: unify bash calling convention

Under ci/ hierarchy, we run scripts under either "sh" (any Bourne
compatible POSIX shell would work) or specifically "bash" (as they
require features from bash, e.g., $(parameter/pattern/string}
expansion).  As we have the CI envionment under our control, we can
expect that /bin/sh will always be fine to run the scripts that only
require Bourne, but we may not know where "bash" gets installed
depending on distros.

So let's make sure we start these scripts with either one of these:

	#!/bin/sh
	#!/usr/bin/env bash

Yes, the latter has to assume that everybody installs "env" at that
path and not as /bin/env or /usr/local/bin/env, but this currently
is the best we could do.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 ci/check-directional-formatting.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ci/check-directional-formatting.bash b/ci/check-directional-formatting.bash
index e6211b141a..3cbbb7030e 100755
--- a/ci/check-directional-formatting.bash
+++ b/ci/check-directional-formatting.bash
@@ -1,4 +1,4 @@
-#!/bin/bash
+#!/usr/bin/env bash
 
 # This script verifies that the non-binary files tracked in the Git index do
 # not contain any Unicode directional formatting: such formatting could be used
-- 
2.45.2-924-g22c02b0a17

Re: Re* [PATCH 7/8] ci: run style check on GitHub and GitLab

[Justin Tobler]

On 24/07/08 03:52PM, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > I think the consensus from the last discussion we had was to allow
> > scripts that rely on bash-isms to say "#!/usr/bin/env bash" because
> > we know /bin/sh can legitimately be not bash and we assume bash may
> > not be installed as /bin/bash.
> 
> Let's do this before we forget.
> 
> ------- >8 ------------- >8 ------------- >8 -------
> Subject: [PATCH] ci: unify bash calling convention
> 
> Under ci/ hierarchy, we run scripts under either "sh" (any Bourne
> compatible POSIX shell would work) or specifically "bash" (as they
> require features from bash, e.g., $(parameter/pattern/string}
> expansion).  As we have the CI envionment under our control, we can

s/envionment/environment

> expect that /bin/sh will always be fine to run the scripts that only
> require Bourne, but we may not know where "bash" gets installed
> depending on distros.
> 
> So let's make sure we start these scripts with either one of these:
> 
> 	#!/bin/sh
> 	#!/usr/bin/env bash
> 
> Yes, the latter has to assume that everybody installs "env" at that
> path and not as /bin/env or /usr/local/bin/env, but this currently
> is the best we could do.

Makes sense to me to be consistent and I also think `#!/usr/bin/env bash`
is probably the best route. Other than the small typo this looks good to
me.

-Justin

Re: [PATCH 7/8] ci: run style check on GitHub and GitLab

[Justin Tobler]

On 24/07/08 02:16PM, Karthik Nayak wrote:
> Justin Tobler <jltobler@gmail.com> writes:
> 
> > On 24/07/08 11:23AM, Karthik Nayak wrote:
> >> We don't run style checks on our CI, even though we have a
> >> '.clang-format' setup in the repository. Let's add one, the job will
> >> validate only against the new commits added and will only run on merge
> >> requests. Since we're introducing it for the first time, let's allow
> >> this job to fail, so we can validate if this is useful and eventually
> >> enforce it.
> >
> > [snip]
> >
> >> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> >> index 37b991e080..65fd261e5e 100644
> >> --- a/.gitlab-ci.yml
> >> +++ b/.gitlab-ci.yml
> >> @@ -123,6 +123,18 @@ check-whitespace:
> >>    rules:
> >>      - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
> >>
> >> +check-style:
> >> +  image: ubuntu:latest
> >> +  allow_failure: true
> >> +  variables:
> >> +    CC: clang
> >> +  before_script:
> >> +    - ./ci/install-dependencies.sh
> >> +  script:
> >> +    - ./ci/run-style-check.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
> >
> > One downside to using $CI_MERGE_REQUEST_DIFF_BASE_SHA is that for GitLab
> > merge pipeines, commits from the merge that are not part of the MR
> > changes are also included. This could lead to somewhat confusing
> > failures.
> >
> 
> I'm not sure I follow.
> 
> > Example failure occuring on this patch series:
> > https://gitlab.com/gitlab-org/git/-/jobs/7284442220
> >
> 
> If you notice this job, it points to the commit: 1c6551488, and the
> parent commit of that commit is: 614dff2011.
> 
> The parent commit [1] is a test commit I added to check the failures. So
> isn't this working as expected?

Ah ok, I misunderstood the setup of that CI job, but the problem is
still present. Here is an example CI job I've run demonstrating it:

CI - https://gitlab.com/gitlab-org/git/-/jobs/7291829941
MR - https://gitlab.com/gitlab-org/git/-/merge_requests/174

For the MR that spawned this CI job, This patch series is the source
branch and the target branch is a version of master one commit ahead
containing a clang format error. Because this is a merge pipeline, using 
$CI_MERGE_REQUEST_DIFF_BASE_SHA will include changes from either side of
the base commit. This means it would be possible for the CI job to fail
due to commits ahead in the target branch, but not in the source branch.
For the check-whitespace CI job, I specifically chose 
$CI_MERGE_REQUEST_TARGET_BRANCH_SHA for this reason.j

> 
> > It might be best to use $CI_MERGE_REQUEST_TARGET_BRANCH_SHA instead.
> >
> 
> I actually started with $CI_MERGE_REQUEST_TARGET_BRANCH_SHA, it didn't
> work, because the value was undefined.
> 
> See: https://gitlab.com/gitlab-org/git/-/jobs/7283724903
> 
> This is why I also decided to fix and change the whitespace check.

I'm not seeing $CI_MERGE_REQUEST_TARGET_BRANCH_SHA as undefined in the
job. Here is a modified version on the check-style CI job printing the
environment variables:

https://gitlab.com/gitlab-org/git/-/jobs/7291792329#L2470

Do you have an example of the check-whitespace job failing in GitLab CI?
Maybe I'm missing something, but I don't see a problem.

-Justin

Re: Re* [PATCH 7/8] ci: run style check on GitHub and GitLab

[brian m. carlson]

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

On 2024-07-08 at 22:52:11, Junio C Hamano wrote:
> Subject: [PATCH] ci: unify bash calling convention
> 
> Under ci/ hierarchy, we run scripts under either "sh" (any Bourne
> compatible POSIX shell would work) or specifically "bash" (as they
> require features from bash, e.g., $(parameter/pattern/string}
> expansion).  As we have the CI envionment under our control, we can
> expect that /bin/sh will always be fine to run the scripts that only
> require Bourne, but we may not know where "bash" gets installed
> depending on distros.
> 
> So let's make sure we start these scripts with either one of these:
> 
> 	#!/bin/sh
> 	#!/usr/bin/env bash
> 
> Yes, the latter has to assume that everybody installs "env" at that
> path and not as /bin/env or /usr/local/bin/env, but this currently
> is the best we could do.

This seems sensible.  We know that /bin/sh is not POSIX-compatible on
some proprietary Unices, but we're not targeting those in CI, and Git
for Windows, macOS, and all of the major open-source Linux and BSD
distros have /bin/sh as a POSIX-compatible shell.

/usr/bin/env bash is also safer than /bin/bash, because bash is not in
/usr/bin on most of the BSDs.  Every other project I've seen writes
/usr/bin/env, so I think that's a fairly safe assumption most places,
and I agree that it's the best we can do.

So I think is the right move.
-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA

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

Re: [PATCH 7/8] ci: run style check on GitHub and GitLab

[Karthik Nayak]

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

Justin Tobler <jltobler@gmail.com> writes:

> On 24/07/08 02:16PM, Karthik Nayak wrote:
>> Justin Tobler <jltobler@gmail.com> writes:
>>
>> > On 24/07/08 11:23AM, Karthik Nayak wrote:
>> >> We don't run style checks on our CI, even though we have a
>> >> '.clang-format' setup in the repository. Let's add one, the job will
>> >> validate only against the new commits added and will only run on merge
>> >> requests. Since we're introducing it for the first time, let's allow
>> >> this job to fail, so we can validate if this is useful and eventually
>> >> enforce it.
>> >
>> > [snip]
>> >
>> >> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
>> >> index 37b991e080..65fd261e5e 100644
>> >> --- a/.gitlab-ci.yml
>> >> +++ b/.gitlab-ci.yml
>> >> @@ -123,6 +123,18 @@ check-whitespace:
>> >>    rules:
>> >>      - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
>> >>
>> >> +check-style:
>> >> +  image: ubuntu:latest
>> >> +  allow_failure: true
>> >> +  variables:
>> >> +    CC: clang
>> >> +  before_script:
>> >> +    - ./ci/install-dependencies.sh
>> >> +  script:
>> >> +    - ./ci/run-style-check.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
>> >
>> > One downside to using $CI_MERGE_REQUEST_DIFF_BASE_SHA is that for GitLab
>> > merge pipeines, commits from the merge that are not part of the MR
>> > changes are also included. This could lead to somewhat confusing
>> > failures.
>> >
>>
>> I'm not sure I follow.
>>
>> > Example failure occuring on this patch series:
>> > https://gitlab.com/gitlab-org/git/-/jobs/7284442220
>> >
>>
>> If you notice this job, it points to the commit: 1c6551488, and the
>> parent commit of that commit is: 614dff2011.
>>
>> The parent commit [1] is a test commit I added to check the failures. So
>> isn't this working as expected?
>
> Ah ok, I misunderstood the setup of that CI job, but the problem is
> still present. Here is an example CI job I've run demonstrating it:
>
> CI - https://gitlab.com/gitlab-org/git/-/jobs/7291829941
> MR - https://gitlab.com/gitlab-org/git/-/merge_requests/174
>
> For the MR that spawned this CI job, This patch series is the source
> branch and the target branch is a version of master one commit ahead
> containing a clang format error. Because this is a merge pipeline, using
> $CI_MERGE_REQUEST_DIFF_BASE_SHA will include changes from either side of
> the base commit. This means it would be possible for the CI job to fail
> due to commits ahead in the target branch, but not in the source branch.
> For the check-whitespace CI job, I specifically chose
> $CI_MERGE_REQUEST_TARGET_BRANCH_SHA for this reason.j
>

You're right indeed. I did some more reading about this and I think the
solution lies somewhere in between..

>>
>> > It might be best to use $CI_MERGE_REQUEST_TARGET_BRANCH_SHA instead.
>> >
>>
>> I actually started with $CI_MERGE_REQUEST_TARGET_BRANCH_SHA, it didn't
>> work, because the value was undefined.
>>
>> See: https://gitlab.com/gitlab-org/git/-/jobs/7283724903
>>
>> This is why I also decided to fix and change the whitespace check.
>
> I'm not seeing $CI_MERGE_REQUEST_TARGET_BRANCH_SHA as undefined in the
> job. Here is a modified version on the check-style CI job printing the
> environment variables:
>

You can see the output

    $ ./ci/run-style-check.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
    fatal: ambiguous argument '': unknown revision or path not in the
working tree.
    Use '--' to separate paths from revisions, like this:
    'git <command> [<revision>...] -- [<file>...]'

This only happens if "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA" is empty.

> https://gitlab.com/gitlab-org/git/-/jobs/7291792329#L2470
>
> Do you have an example of the check-whitespace job failing in GitLab CI?
> Maybe I'm missing something, but I don't see a problem.
>
> -Justin

So I think I get the issue, GitLab has two kinds of pipelines it runs:
1. merge pipeline: Here the pipeline runs on the source branch (the
feature branch which has to be merged).
2. merged pipeline: Here the pipeline creates a merge commit using the
source and target branch and then runs the pipeline on the merged
commit.

And "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA" is only defined in the 'merged
pipeline'. If you see the pipelines for my branch on GitLab [1]. You'll
see only one of them is marked as 'merge results' and the others being
marked as 'merged results'. The former includes the job I mentioned
above, where "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA" is not defined.

I'm still not sure why it marked only one of the pipelines as such, but
this means there is chance that it could happen.

So I guess the best outcome is to use
"$CI_MERGE_REQUEST_TARGET_BRANCH_SHA", but fallback to
"$CI_MERGE_REQUEST_DIFF_BASE_SHA", if the former is not defined.

[1]: https://gitlab.com/gitlab-org/git/-/merge_requests/172/pipelines

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

Re: [PATCH 7/8] ci: run style check on GitHub and GitLab

[Justin Tobler]

On 24/07/09 01:44AM, Karthik Nayak wrote:
> Justin Tobler <jltobler@gmail.com> writes:
> 
> > On 24/07/08 02:16PM, Karthik Nayak wrote:
> >> Justin Tobler <jltobler@gmail.com> writes:
> >>
> >> > On 24/07/08 11:23AM, Karthik Nayak wrote:
> >> >> We don't run style checks on our CI, even though we have a
> >> >> '.clang-format' setup in the repository. Let's add one, the job will
> >> >> validate only against the new commits added and will only run on merge
> >> >> requests. Since we're introducing it for the first time, let's allow
> >> >> this job to fail, so we can validate if this is useful and eventually
> >> >> enforce it.
> >> >
> >> > [snip]
> >> >
> >> >> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> >> >> index 37b991e080..65fd261e5e 100644
> >> >> --- a/.gitlab-ci.yml
> >> >> +++ b/.gitlab-ci.yml
> >> >> @@ -123,6 +123,18 @@ check-whitespace:
> >> >>    rules:
> >> >>      - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
> >> >>
> >> >> +check-style:
> >> >> +  image: ubuntu:latest
> >> >> +  allow_failure: true
> >> >> +  variables:
> >> >> +    CC: clang
> >> >> +  before_script:
> >> >> +    - ./ci/install-dependencies.sh
> >> >> +  script:
> >> >> +    - ./ci/run-style-check.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
> >> >
> >> > One downside to using $CI_MERGE_REQUEST_DIFF_BASE_SHA is that for GitLab
> >> > merge pipeines, commits from the merge that are not part of the MR
> >> > changes are also included. This could lead to somewhat confusing
> >> > failures.
> >> >
> >>
> >> I'm not sure I follow.
> >>
> >> > Example failure occuring on this patch series:
> >> > https://gitlab.com/gitlab-org/git/-/jobs/7284442220
> >> >
> >>
> >> If you notice this job, it points to the commit: 1c6551488, and the
> >> parent commit of that commit is: 614dff2011.
> >>
> >> The parent commit [1] is a test commit I added to check the failures. So
> >> isn't this working as expected?
> >
> > Ah ok, I misunderstood the setup of that CI job, but the problem is
> > still present. Here is an example CI job I've run demonstrating it:
> >
> > CI - https://gitlab.com/gitlab-org/git/-/jobs/7291829941
> > MR - https://gitlab.com/gitlab-org/git/-/merge_requests/174
> >
> > For the MR that spawned this CI job, This patch series is the source
> > branch and the target branch is a version of master one commit ahead
> > containing a clang format error. Because this is a merge pipeline, using
> > $CI_MERGE_REQUEST_DIFF_BASE_SHA will include changes from either side of
> > the base commit. This means it would be possible for the CI job to fail
> > due to commits ahead in the target branch, but not in the source branch.
> > For the check-whitespace CI job, I specifically chose
> > $CI_MERGE_REQUEST_TARGET_BRANCH_SHA for this reason.j
> >
> 
> You're right indeed. I did some more reading about this and I think the
> solution lies somewhere in between..
> 
> >>
> >> > It might be best to use $CI_MERGE_REQUEST_TARGET_BRANCH_SHA instead.
> >> >
> >>
> >> I actually started with $CI_MERGE_REQUEST_TARGET_BRANCH_SHA, it didn't
> >> work, because the value was undefined.
> >>
> >> See: https://gitlab.com/gitlab-org/git/-/jobs/7283724903
> >>
> >> This is why I also decided to fix and change the whitespace check.
> >
> > I'm not seeing $CI_MERGE_REQUEST_TARGET_BRANCH_SHA as undefined in the
> > job. Here is a modified version on the check-style CI job printing the
> > environment variables:
> >
> 
> You can see the output
> 
>     $ ./ci/run-style-check.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
>     fatal: ambiguous argument '': unknown revision or path not in the
> working tree.
>     Use '--' to separate paths from revisions, like this:
>     'git <command> [<revision>...] -- [<file>...]'
> 
> This only happens if "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA" is empty.

Ya I noticed this failure, but was wondering if it was maybe due to
something else. I have been unable to reproduce it and in all the jobs I
was running resulted in a merge pipeline with the variable defined. But 
maybe sometimes a regular pipeline gets run for some reason and 
consequently $CI_MERGE_REQUEST_TARGET_BRANCH_SHA is not defined? Was the
pipeline triggered directly from the source branch?

> 
> > https://gitlab.com/gitlab-org/git/-/jobs/7291792329#L2470
> >
> > Do you have an example of the check-whitespace job failing in GitLab CI?
> > Maybe I'm missing something, but I don't see a problem.
> >
> > -Justin
> 
> So I think I get the issue, GitLab has two kinds of pipelines it runs:
> 1. merge pipeline: Here the pipeline runs on the source branch (the
> feature branch which has to be merged).
> 2. merged pipeline: Here the pipeline creates a merge commit using the
> source and target branch and then runs the pipeline on the merged
> commit.
> 

Correct, this is my understanding.

> And "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA" is only defined in the 'merged
> pipeline'. If you see the pipelines for my branch on GitLab [1]. You'll
> see only one of them is marked as 'merge results' and the others being
> marked as 'merged results'. The former includes the job I mentioned
> above, where "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA" is not defined.
> 
> I'm still not sure why it marked only one of the pipelines as such, but
> this means there is chance that it could happen.

Huh, I'm guessing the CI job must have been triggered from the source
branch directly. Did you manually run the CI job? I wonder if that could
have caused it.

> 
> So I guess the best outcome is to use
> "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA", but fallback to
> "$CI_MERGE_REQUEST_DIFF_BASE_SHA", if the former is not defined.

This is exactly what I think we should do too. For merge pipelines we 
will want to use $CI_MERGE_REQUEST_TARGET_BRANCH_SHA so that only the 
commits included in the MR are scanned in CI. If that variable is not 
defined, it makes sense to fallback to $CI_MERGE_REQUEST_DIFF_BASE_SHA.
Since it's not a merge pipeline it will only scan commits included from
the MR and therefore work as expected.

This should handle both cases correctly. :)

-Justin

Re: [PATCH 6/8] clang-format: formalize some of the spacing rules

[Karthik Nayak]

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

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Mon, Jul 8, 2024 at 5:24 AM Karthik Nayak <karthik.188@gmail.com> wrote:
>> There are some spacing rules that we follow in the project and it makes
>> sen to formalize them:
>
> Since nobody else pointed it out: s/sen/sense/

Will fix in v2.

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

Re: [PATCH 7/8] ci: run style check on GitHub and GitLab

[Karthik Nayak]

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

Justin Tobler <jltobler@gmail.com> writes:

[snip]

>>
>> You can see the output
>>
>>     $ ./ci/run-style-check.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
>>     fatal: ambiguous argument '': unknown revision or path not in the
>> working tree.
>>     Use '--' to separate paths from revisions, like this:
>>     'git <command> [<revision>...] -- [<file>...]'
>>
>> This only happens if "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA" is empty.
>
> Ya I noticed this failure, but was wondering if it was maybe due to
> something else. I have been unable to reproduce it and in all the jobs I
> was running resulted in a merge pipeline with the variable defined. But
> maybe sometimes a regular pipeline gets run for some reason and
> consequently $CI_MERGE_REQUEST_TARGET_BRANCH_SHA is not defined? Was the
> pipeline triggered directly from the source branch?
>

Just a regular push. Not sure at all why this happened. I was testing
different types of style issues on the CI and this happened once.

>>
>> > https://gitlab.com/gitlab-org/git/-/jobs/7291792329#L2470
>> >
>> > Do you have an example of the check-whitespace job failing in GitLab CI?
>> > Maybe I'm missing something, but I don't see a problem.
>> >
>> > -Justin
>>
>> So I think I get the issue, GitLab has two kinds of pipelines it runs:
>> 1. merge pipeline: Here the pipeline runs on the source branch (the
>> feature branch which has to be merged).
>> 2. merged pipeline: Here the pipeline creates a merge commit using the
>> source and target branch and then runs the pipeline on the merged
>> commit.
>>
>
> Correct, this is my understanding.
>
>> And "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA" is only defined in the 'merged
>> pipeline'. If you see the pipelines for my branch on GitLab [1]. You'll
>> see only one of them is marked as 'merge results' and the others being
>> marked as 'merged results'. The former includes the job I mentioned
>> above, where "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA" is not defined.
>>
>> I'm still not sure why it marked only one of the pipelines as such, but
>> this means there is chance that it could happen.
>
> Huh, I'm guessing the CI job must have been triggered from the source
> branch directly. Did you manually run the CI job? I wonder if that could
> have caused it.
>

Not that I remember.

>>
>> So I guess the best outcome is to use
>> "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA", but fallback to
>> "$CI_MERGE_REQUEST_DIFF_BASE_SHA", if the former is not defined.
>
> This is exactly what I think we should do too. For merge pipelines we
> will want to use $CI_MERGE_REQUEST_TARGET_BRANCH_SHA so that only the
> commits included in the MR are scanned in CI. If that variable is not
> defined, it makes sense to fallback to $CI_MERGE_REQUEST_DIFF_BASE_SHA.
> Since it's not a merge pipeline it will only scan commits included from
> the MR and therefore work as expected.
>
> This should handle both cases correctly. :)
>
> -Justin

Yeah seems like the best solution at this point, let me implement this.

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

Re: [PATCH 8/8] check-whitespace: detect if no base_commit is provided

[Karthik Nayak]

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

Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> The 'check-whitespace' CI script exists gracefully if no base commit is
>
> "exists" -> "exits"
>

Will fix.

>> provided or if an invalid revision is provided...
>> ...
>> Let's fix the variable used in the GitLab CI. Let's also add a check for
>> incorrect base_commit in the 'check-whitespace.sh' script. While here,
>> fix a small typo too.
>>
>> [1]: https://docs.gitlab.com/ee/ci/variables/predefined_variables.html#predefined-variables-for-merge-request-pipelines
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>>  .gitlab-ci.yml         |  2 +-
>>  ci/check-whitespace.sh | 13 ++++++++++---
>>  2 files changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
>> index 65fd261e5e..36199893d8 100644
>> --- a/.gitlab-ci.yml
>> +++ b/.gitlab-ci.yml
>> @@ -119,7 +119,7 @@ check-whitespace:
>>    before_script:
>>      - ./ci/install-dependencies.sh
>>    script:
>> -    - ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
>> +    - ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
>>    rules:
>>      - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
>>
>> diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh
>> index db399097a5..ab023f9519 100755
>> --- a/ci/check-whitespace.sh
>> +++ b/ci/check-whitespace.sh
>> @@ -9,12 +9,19 @@ baseCommit=$1
>>  outputFile=$2
>>  url=$3
>>
>> -if test "$#" -ne 1 && test "$#" -ne 3
>> +if { test "$#" -ne 1 && test "$#" -ne 3; } || test -z "$1"
>
> You can just add || test -z "$1" after the existing one, making the
> thing A && B || C which evaulates left to right with the same
> precedence for && and ||.
>

Well, I prefer making it explicit so one does not have to remember the
precedence ordering, but it could just be my lack of shell knowledge.
I'm okay with this change, I'll add it in the next version.

>>  then
>>  	echo "USAGE: $0 <BASE_COMMIT> [<OUTPUT_FILE> <URL>]"
>>  	exit 1
>>  fi
>>
>> +gitLogOutput=$(git log --check --pretty=format:"---% h% s" "${baseCommit}"..)
>
> That is a large string to hold in a variable for a potentially large
> series with lots of breakages.  I didn't quite read the reasoning
> behind this change in the proposed log message.  Under what
> condition do you expect the command to exit with non-zero status?
> $basecommit being a non-empty string but does not name a valid
> commit object or something, in which case shouldn't "git log
> --oneline $baseCommit.."  or something simpler should suffice?
>

Yeah, makes sense. I think I'll simply add in

    if ! git rev-parse --quiet --verify "${baseCommit}"
    then
        echo "Invalid <BASE_COMMIT> '${baseCommit}'"
        exit 1
    fi

instead

>> +if test $? -ne 0
>> +then
>> +	echo -n $gitLogOutput
>
> What is "-n" doing here?  Why are you squashing run of spaces in the
> $gitLogOutput variable into a space by not "quoting" inside a dq-pair?
>

I actually didn't know about this. Thanks for informing.

>> +	exit 1
>> +fi
>
> Looking for "--check" in
>
> 	$ git log --help
>
> tells me that the command exits with non-zero status if problems are
> found, so wouldn't that mean the cases with problems always exit
> early, bypassing the while loop with full of bash-ism that comes
> after this block?
>

It should exist with a non-zero code, but since we're capturing it in
the while loop, it doesn't stop the slow. A consequence of which is that
it'll print the stderr from the `git log` failing, but the script itself
will still exit with a zero exit code. This marks a success on the CI.

>>  problems=()
>>  commit=
>>  commitText=
>> @@ -58,7 +65,7 @@ do
>>  		echo "${dash} ${sha} ${etc}"
>>  		;;
>>  	esac
>> -done <<< "$(git log --check --pretty=format:"---% h% s" "${baseCommit}"..)"
>> +done <<< "$gitLogOutput"
>>
>>  if test ${#problems[*]} -gt 0
>>  then
>> @@ -67,7 +74,7 @@ then
>>  		goodParent=${baseCommit: 0:7}
>>  	fi
>>
>> -	echo "A whitespace issue was found in onen of more of the commits."
>> +	echo "A whitespace issue was found in one of more of the commits."
>>  	echo "Run the following command to resolve whitespace issues:"
>>  	echo "git rebase --whitespace=fix ${goodParent}"

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

Re: [PATCH 8/8] check-whitespace: detect if no base_commit is provided

[Karthik Nayak]

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

Justin Tobler <jltobler@gmail.com> writes:

> On 24/07/08 11:23AM, Karthik Nayak wrote:
>> The 'check-whitespace' CI script exists gracefully if no base commit is
>> provided or if an invalid revision is provided. This is not good because
>> if a particular CI provides an incorrect base_commit, it would fail
>> successfully.
>
> s/exists/exits
>
> If no base commit is provided, we already fail. Here is an example
> GitLab CI job demonstrating this:
> https://gitlab.com/gitlab-org/git/-/jobs/7289543498#L2370
>
> If the base commit does not exist though, it currently prints that error occured
> but still exits with 0. Makes sense to update and fail the job accordingly.
>

This example is running on the 'edb990d9', whose parent's '8d9bcf0a'
parent '57fdb00c' contains my changes, specifically the `|| test -z "$1"`
section. You can check this on master locally though.

    $ ./ci/check-whitespace.sh ""
    fatal: ..: '..' is outside repository at '/home/karthik/git'

    $ echo $?
    0

or for invalid value:

    $ ./ci/check-whitespace.sh "random"
    fatal: ambiguous argument 'random..': unknown revision or path not
in the working tree.
    Use '--' to separate paths from revisions, like this:
    'git <command> [<revision>...] -- [<file>...]'

    $ echo $status
    0

[snip]

>
> I might be misunderstanding, but this additional check seems redundant to me.
>

It is required, as commented above

>>  then
>>  	echo "USAGE: $0 <BASE_COMMIT> [<OUTPUT_FILE> <URL>]"
>>  	exit 1
>>  fi
>>
>> +gitLogOutput=$(git log --check --pretty=format:"---% h% s" "${baseCommit}"..)
>> +if test $? -ne 0
>> +then
>> +	echo -n $gitLogOutput
>> +	exit 1
>> +fi
>> +
>>  problems=()
>>  commit=
>>  commitText=
>> @@ -58,7 +65,7 @@ do
>>  		echo "${dash} ${sha} ${etc}"
>>  		;;
>>  	esac
>> -done <<< "$(git log --check --pretty=format:"---% h% s" "${baseCommit}"..)"
>> +done <<< "$gitLogOutput"
>>
>>  if test ${#problems[*]} -gt 0
>>  then
>> @@ -67,7 +74,7 @@ then
>>  		goodParent=${baseCommit: 0:7}
>>  	fi
>>
>> -	echo "A whitespace issue was found in onen of more of the commits."
>> +	echo "A whitespace issue was found in one of more of the commits."
> There is another preexisting typo:
>
> s/one of/one or/
>

Thanks. will add

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

Re: [PATCH 8/8] check-whitespace: detect if no base_commit is provided

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> Yeah, makes sense. I think I'll simply add in
>
>     if ! git rev-parse --quiet --verify "${baseCommit}"
>     then
>         echo "Invalid <BASE_COMMIT> '${baseCommit}'"
>         exit 1
>     fi

It would make the intent a lot clearer.  Good.

>>> +if test $? -ne 0
>>> +then
>>> +	echo -n $gitLogOutput
>>
>> What is "-n" doing here?  Why are you squashing run of spaces in the
>> $gitLogOutput variable into a space by not "quoting" inside a dq-pair?
>>
>
> I actually didn't know about this. Thanks for informing.
>
>>> +	exit 1
>>> +fi
>>
>> Looking for "--check" in
>>
>> 	$ git log --help
>>
>> tells me that the command exits with non-zero status if problems are
>> found, so wouldn't that mean the cases with problems always exit
>> early, bypassing the while loop with full of bash-ism that comes
>> after this block?
>>
>
> It should exist with a non-zero code, but since we're capturing it in
> the while loop, it doesn't stop the slow.

Sorry, but I am confused.  The control would not even reach a "while
loop", I am afraid.

I was commenting on the exit status check done here:

    +gitLogOutput=$(git log --check --pretty=format:"---% h% s" "${baseCommit}"..)
    +if test $? -ne 0
    +then
    +	echo -n $gitLogOutput
    +	exit 1
    +fi

Even though the output is captured in a variable, the exit status of
"git log --check" is still seen by the shell and "if test $? = 0"
next line say "ah, the thing exited with non-zero status so lets
echo the whole thing and exit with 1", before it gets to the while
loop we have below the above piece of code, no?

Re: [PATCH 8/8] check-whitespace: detect if no base_commit is provided

[Karthik Nayak]

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

Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Yeah, makes sense. I think I'll simply add in
>>
>>     if ! git rev-parse --quiet --verify "${baseCommit}"
>>     then
>>         echo "Invalid <BASE_COMMIT> '${baseCommit}'"
>>         exit 1
>>     fi
>
> It would make the intent a lot clearer.  Good.
>
>>>> +if test $? -ne 0
>>>> +then
>>>> +	echo -n $gitLogOutput
>>>
>>> What is "-n" doing here?  Why are you squashing run of spaces in the
>>> $gitLogOutput variable into a space by not "quoting" inside a dq-pair?
>>>
>>
>> I actually didn't know about this. Thanks for informing.
>>
>>>> +	exit 1
>>>> +fi
>>>
>>> Looking for "--check" in
>>>
>>> 	$ git log --help
>>>
>>> tells me that the command exits with non-zero status if problems are
>>> found, so wouldn't that mean the cases with problems always exit
>>> early, bypassing the while loop with full of bash-ism that comes
>>> after this block?
>>>
>>
>> It should exist with a non-zero code, but since we're capturing it in
>> the while loop, it doesn't stop the slow.
>
> Sorry, but I am confused.  The control would not even reach a "while
> loop", I am afraid.
>
> I was commenting on the exit status check done here:
>
>     +gitLogOutput=$(git log --check --pretty=format:"---% h% s" "${baseCommit}"..)
>     +if test $? -ne 0
>     +then
>     +	echo -n $gitLogOutput
>     +	exit 1
>     +fi
>
> Even though the output is captured in a variable, the exit status of
> "git log --check" is still seen by the shell and "if test $? = 0"
> next line say "ah, the thing exited with non-zero status so lets
> echo the whole thing and exit with 1", before it gets to the while
> loop we have below the above piece of code, no?

My bad, I thought you were referring to the code before my changes. Yes,
here you're right, we don't need the check since the shell would capture
the non-zero status.

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

[PATCH v2 0/8] clang-format: add more rules and enable on CI

[Karthik Nayak]

This is v2 of my series to add clang-format to the CI.

The series was mostly inspired by a patch sent recently to 'include
kh_foreach* macros in ForEachMacros' [1]. I was wondering why we don't
run the formatting on CI and reduce some of the workload of reviewers.

We have a '.clang-format' file to provide rules for code formatting.
The commits 1-6 aims to add more rules to the file while deprecating old
ones.

Commit 7 enables CI action on GitHub and GitLab to also check for the
code formatting. Currently, it is allowed to fail. This way we can build
confidence and fine tune the values as needed and finally enforce the
check in a future patch. I'm not well versed with GitHub workflows, and
I mostly tried to copy existing work there. Expecting some feedback in
that section!

Commit 8 fixes an existing issue with the 'check-whitespace' job, which
is failing as success in the GitLab CI.

1: https://lore.kernel.org/git/4e7893f5-2dd9-46cf-8a64-cf780f4e1730@web.de/

Changes against v1:
* Fixed a lot of typos.
* Added more details about the warnings specified by clang-format for
'RemoveBracesLLVM' in commit 5. Added more details too.
* The last two commits were modified to use "CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
and fallback to "CI_MERGE_REQUEST_DIFF_BASE_SHA" if the former is not
available in GitLab's CI.
* Use `!/bin/sh` for the run-style-check script.
* Modified the last commit to remove block around the && check in the
whitespace script.
* Use `git rev-parse --verify --quiet` to verify the base commit.


Karthik Nayak (8):
  clang-format: indent preprocessor directives after hash
  clang-format: avoid spacing around bitfield colon
  clang-format: ensure files end with newlines
  clang-format: replace deprecated option with 'SpacesInParens'
  clang-format: avoid braces on simple single-statement bodies
  clang-format: formalize some of the spacing rules
  ci: run style check on GitHub and GitLab
  check-whitespace: detect if no base_commit is provided

 .clang-format                     | 42 +++++++++++++++++++++++++++----
 .github/workflows/check-style.yml | 29 +++++++++++++++++++++
 .gitlab-ci.yml                    | 24 +++++++++++++++++-
 ci/check-whitespace.sh            | 10 ++++++--
 ci/install-dependencies.sh        |  2 +-
 ci/run-style-check.sh             |  8 ++++++
 6 files changed, 106 insertions(+), 9 deletions(-)
 create mode 100644 .github/workflows/check-style.yml
 create mode 100755 ci/run-style-check.sh

Range-diff against v1:
1:  6cf91ffc86 = 1:  6cf91ffc86 clang-format: indent preprocessor directives after hash
2:  beb002885f = 2:  beb002885f clang-format: avoid spacing around bitfield colon
3:  3031be43e7 = 3:  3031be43e7 clang-format: ensure files end with newlines
4:  bc1550e300 = 4:  bc1550e300 clang-format: replace deprecated option with 'SpacesInParens'
5:  cb6097aa22 ! 5:  840318a4c9 clang-format: avoid braces on simple single-statement bodies
    @@ Commit message
         Set the 'RemoveBracesLLVM' to 'true' which ensures that we avoid curly
         braces for single-statement bodies in conditional blocks.
     
    +    This option does come with two warnings:
    +
    +        This option will be renamed and expanded to support other styles.
    +
    +    and
    +
    +        Setting this option to true could lead to incorrect code formatting
    +        due to clang-format’s lack of complete semantic information. As
    +        such, extra care should be taken to review code changes made by
    +        this option.
    +
    +    The latter seems to be of concern. But since we only use clang-format to
    +    verify the format and not to apply formatting, we should be okay here.
    +
         Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
     
      ## .clang-format ##
    @@ .clang-format: IndentWrappedFunctionNames: false
      PointerAlignment: Right
      
     +# Remove optional braces of control statements (if, else, for, and while)
    -+# according to the LLVM coding style
    -+# This avoids braces on simple single-statement bodies of statements.
    ++# according to the LLVM coding style. This avoids braces on simple
    ++# single-statement bodies of statements but keeps braces if one side of
    ++# if/else if/.../else cascade has multi-statement body.
     +RemoveBracesLLVM: true
     +
      # Don't insert a space after a cast
6:  c4b6e1e82f ! 6:  0ecfd8d24b clang-format: formalize some of the spacing rules
    @@ Commit message
         clang-format: formalize some of the spacing rules
     
         There are some spacing rules that we follow in the project and it makes
    -    sen to formalize them:
    +    sense to formalize them:
         * Ensure there is no space inserted after the logical not '!' operator.
    -    * Ensure there is no space before the case statement's color.
    +    * Ensure there is no space before the case statement's colon.
         * Ensure there is no space before the first bracket '[' of an array.
         * Ensure there is no space in empty blocks.
     
7:  37f1e5c702 ! 7:  11a9ac4935 ci: run style check on GitHub and GitLab
    @@ Commit message
         this job to fail, so we can validate if this is useful and eventually
         enforce it.
     
    +    For GitLab, we use the 'CI_MERGE_REQUEST_TARGET_BRANCH_SHA' variable by
    +    default to obtain the base SHA of the merged pipeline (which is only
    +    available for merged pipelines [1]). Otherwise we use the
    +    'CI_MERGE_REQUEST_DIFF_BASE_SHA' variable.
    +
    +    [1]: https://docs.gitlab.com/ee/ci/variables/predefined_variables.html#predefined-variables-for-merge-request-pipelines
    +
         Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
     
      ## .github/workflows/check-style.yml (new) ##
    @@ .gitlab-ci.yml: check-whitespace:
     +  before_script:
     +    - ./ci/install-dependencies.sh
     +  script:
    -+    - ./ci/run-style-check.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
    ++    - |
    ++      if [ -z ${CI_MERGE_REQUEST_TARGET_BRANCH_SHA} ]; then
    ++        ./ci/run-style-check.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
    ++      else
    ++        ./ci/run-style-check.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
    ++      fi
     +  rules:
     +    - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
     +
    @@ ci/install-dependencies.sh: ubuntu-*)
     
      ## ci/run-style-check.sh (new) ##
     @@
    -+#!/usr/bin/env sh
    ++#!/bin/sh
     +#
     +# Perform style check
     +#
8:  c6d07402af < -:  ---------- check-whitespace: detect if no base_commit is provided
-:  ---------- > 8:  caccbf8264 check-whitespace: detect if no base_commit is provided
-- 
2.45.1

[PATCH v2 1/8] clang-format: indent preprocessor directives after hash

[Karthik Nayak]

We do not have a rule around the indentation of preprocessor directives.
This was also discussed on the list [1], noting how there is often
inconsistency in the styling. While there was discussion, there was no
conclusion around what is the preferred style here. One style being
indenting after the hash:

    #if FOO
    #  if BAR
    #    include <foo>
    #  endif
    #endif

The other being before the hash:

    #if FOO
      #if BAR
        #include <foo>
      #endif
    #endif

Let's pick the former and add 'IndentPPDirectives: AfterHash' value to
our '.clang-format'. There is no clear reason to pick one over the
other, but it would definitely be nicer to be consistent.

[1]: https://lore.kernel.org/r/xmqqwmmm1bw6.fsf@gitster.g

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .clang-format | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/.clang-format b/.clang-format
index 3ed4fac753..5e128519bf 100644
--- a/.clang-format
+++ b/.clang-format
@@ -96,6 +96,12 @@ BreakStringLiterals: false
 # Switch statement body is always indented one level more than case labels.
 IndentCaseLabels: false
 
+# Indents directives before the hash.
+# #if FOO
+# #  include <foo>
+# #endif
+IndentPPDirectives: AfterHash
+
 # Don't indent a function definition or declaration if it is wrapped after the
 # type
 IndentWrappedFunctionNames: false
-- 
2.45.1

[PATCH v2 2/8] clang-format: avoid spacing around bitfield colon

[Karthik Nayak]

The spacing around colons is currently not standardized and as such we
have the following practices in our code base:
- Spacing around the colon `int bf : 1`: 146 instances
- No spacing around the colon `int bf:1`: 148 instances
- Spacing before the colon `int bf :1`: 6 instances
- Spacing after the colon `int bf: 1`: 12 instances

Let's formalize this by picking the most followed pattern and add the
corresponding style to '.clang-format'.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .clang-format | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/.clang-format b/.clang-format
index 5e128519bf..803b274dd5 100644
--- a/.clang-format
+++ b/.clang-format
@@ -72,6 +72,10 @@ AlwaysBreakAfterReturnType: None
 BinPackArguments: true
 BinPackParameters: true
 
+# Add no space around the bit field
+# unsigned bf:2;
+BitFieldColonSpacing: None
+
 # Attach braces to surrounding context except break before braces on function
 # definitions.
 # void foo()
-- 
2.45.1

[PATCH v2 3/8] clang-format: ensure files end with newlines

[Karthik Nayak]

All our source files end with a newline, let's formalize in
'.clang-format'.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .clang-format | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/.clang-format b/.clang-format
index 803b274dd5..4c9751a9db 100644
--- a/.clang-format
+++ b/.clang-format
@@ -106,6 +106,9 @@ IndentCaseLabels: false
 # #endif
 IndentPPDirectives: AfterHash
 
+# Insert a newline at end of file if missing
+InsertNewlineAtEOF: true
+
 # Don't indent a function definition or declaration if it is wrapped after the
 # type
 IndentWrappedFunctionNames: false
-- 
2.45.1

[PATCH v2 4/8] clang-format: replace deprecated option with 'SpacesInParens'

[Karthik Nayak]

Replace the deprecated options 'SpacesInParentheses' and
'SpaceInEmptyParentheses' with the newer 'SpacesInParens' option. The
usage is the same.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .clang-format | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/.clang-format b/.clang-format
index 4c9751a9db..914254a29b 100644
--- a/.clang-format
+++ b/.clang-format
@@ -134,8 +134,6 @@ SpaceBeforeAssignmentOperators: true
 # }
 SpaceBeforeParens: ControlStatements
 
-# Don't insert spaces inside empty '()'
-SpaceInEmptyParentheses: false
 
 # The number of spaces before trailing line comments (// - comments).
 # This does not affect trailing block comments (/* - comments).
@@ -149,9 +147,10 @@ SpacesInCStyleCastParentheses: false
 # var arr = [1, 2, 3];    not    var arr = [ 1, 2, 3 ];
 SpacesInContainerLiterals: false
 
-# Don't insert spaces after '(' or before ')'
-# f(arg);    not    f( arg );
-SpacesInParentheses: false
+SpacesInParens: Custom
+SpacesInParensOptions:
+  # Don't insert spaces inside empty '()'
+  InEmptyParentheses: false
 
 # Don't insert spaces after '[' or before ']'
 # int a[5];    not    int a[ 5 ];
-- 
2.45.1

[PATCH v2 5/8] clang-format: avoid braces on simple single-statement bodies

[Karthik Nayak]

Set the 'RemoveBracesLLVM' to 'true' which ensures that we avoid curly
braces for single-statement bodies in conditional blocks.

This option does come with two warnings:

    This option will be renamed and expanded to support other styles.

and

    Setting this option to true could lead to incorrect code formatting
    due to clang-format’s lack of complete semantic information. As
    such, extra care should be taken to review code changes made by
    this option.

The latter seems to be of concern. But since we only use clang-format to
verify the format and not to apply formatting, we should be okay here.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .clang-format | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/.clang-format b/.clang-format
index 914254a29b..8b75dece80 100644
--- a/.clang-format
+++ b/.clang-format
@@ -117,6 +117,12 @@ IndentWrappedFunctionNames: false
 # int *a;
 PointerAlignment: Right
 
+# Remove optional braces of control statements (if, else, for, and while)
+# according to the LLVM coding style. This avoids braces on simple
+# single-statement bodies of statements but keeps braces if one side of
+# if/else if/.../else cascade has multi-statement body.
+RemoveBracesLLVM: true
+
 # Don't insert a space after a cast
 # x = (int32)y;    not    x = (int32) y;
 SpaceAfterCStyleCast: false
-- 
2.45.1

[PATCH v2 6/8] clang-format: formalize some of the spacing rules

[Karthik Nayak]

There are some spacing rules that we follow in the project and it makes
sense to formalize them:
* Ensure there is no space inserted after the logical not '!' operator.
* Ensure there is no space before the case statement's colon.
* Ensure there is no space before the first bracket '[' of an array.
* Ensure there is no space in empty blocks.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .clang-format | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/.clang-format b/.clang-format
index 8b75dece80..68c10f03da 100644
--- a/.clang-format
+++ b/.clang-format
@@ -127,11 +127,18 @@ RemoveBracesLLVM: true
 # x = (int32)y;    not    x = (int32) y;
 SpaceAfterCStyleCast: false
 
+# No space is inserted after the logical not operator
+SpaceAfterLogicalNot: false
+
 # Insert spaces before and after assignment operators
 # int a = 5;    not    int a=5;
 # a += 42;             a+=42;
 SpaceBeforeAssignmentOperators: true
 
+# Spaces will be removed before case colon.
+# case 1: break;    not     case 1 : break;
+SpaceBeforeCaseColon: false
+
 # Put a space before opening parentheses only after control statement keywords.
 # void f() {
 #   if (true) {
@@ -140,6 +147,13 @@ SpaceBeforeAssignmentOperators: true
 # }
 SpaceBeforeParens: ControlStatements
 
+# No space before first '[' in arrays
+# int a[5][5];     not      int a [5][5];
+SpaceBeforeSquareBrackets: false
+
+# No space will be inserted into {}
+# while (true) {}    not    while (true) { }
+SpaceInEmptyBlock: false
 
 # The number of spaces before trailing line comments (// - comments).
 # This does not affect trailing block comments (/* - comments).
-- 
2.45.1

[PATCH v2 7/8] ci: run style check on GitHub and GitLab

[Karthik Nayak]

We don't run style checks on our CI, even though we have a
'.clang-format' setup in the repository. Let's add one, the job will
validate only against the new commits added and will only run on merge
requests. Since we're introducing it for the first time, let's allow
this job to fail, so we can validate if this is useful and eventually
enforce it.

For GitLab, we use the 'CI_MERGE_REQUEST_TARGET_BRANCH_SHA' variable by
default to obtain the base SHA of the merged pipeline (which is only
available for merged pipelines [1]). Otherwise we use the
'CI_MERGE_REQUEST_DIFF_BASE_SHA' variable.

[1]: https://docs.gitlab.com/ee/ci/variables/predefined_variables.html#predefined-variables-for-merge-request-pipelines

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .github/workflows/check-style.yml | 29 +++++++++++++++++++++++++++++
 .gitlab-ci.yml                    | 17 +++++++++++++++++
 ci/install-dependencies.sh        |  2 +-
 ci/run-style-check.sh             |  8 ++++++++
 4 files changed, 55 insertions(+), 1 deletion(-)
 create mode 100644 .github/workflows/check-style.yml
 create mode 100755 ci/run-style-check.sh

diff --git a/.github/workflows/check-style.yml b/.github/workflows/check-style.yml
new file mode 100644
index 0000000000..27276dfe5e
--- /dev/null
+++ b/.github/workflows/check-style.yml
@@ -0,0 +1,29 @@
+name: check-style
+
+# Get the repository with all commits to ensure that we can analyze
+# all of the commits contributed via the Pull Request.
+
+on:
+  pull_request:
+    types: [opened, synchronize]
+
+# Avoid unnecessary builds. Unlike the main CI jobs, these are not
+# ci-configurable (but could be).
+concurrency:
+  group: ${{ github.workflow }}-${{ github.ref }}
+  cancel-in-progress: true
+
+jobs:
+  check-style:
+    runs-on: ubuntu-latest
+    steps:
+    - uses: actions/checkout@v4
+      with:
+        fetch-depth: 0
+
+    - name: git clang-format
+      continue-on-error: true
+      id: check_out
+      run: |
+        ./ci/run-style-check.sh \
+          "${{github.event.pull_request.base.sha}}"
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 37b991e080..dc43fc8ba8 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -123,6 +123,23 @@ check-whitespace:
   rules:
     - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
 
+check-style:
+  image: ubuntu:latest
+  allow_failure: true
+  variables:
+    CC: clang
+  before_script:
+    - ./ci/install-dependencies.sh
+  script:
+    - |
+      if [ -z ${CI_MERGE_REQUEST_TARGET_BRANCH_SHA} ]; then
+        ./ci/run-style-check.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
+      else
+        ./ci/run-style-check.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
+      fi
+  rules:
+    - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
+
 documentation:
   image: ubuntu:latest
   variables:
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 6ec0f85972..46fe12a690 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -43,7 +43,7 @@ ubuntu-*)
 		make libssl-dev libcurl4-openssl-dev libexpat-dev wget sudo default-jre \
 		tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl \
 		libemail-valid-perl libio-pty-perl libio-socket-ssl-perl libnet-smtp-ssl-perl libdbd-sqlite3-perl libcgi-pm-perl \
-		${CC_PACKAGE:-${CC:-gcc}} $PYTHON_PACKAGE
+		${CC_PACKAGE:-${CC:-gcc}} $PYTHON_PACKAGE clang-format
 
 	mkdir --parents "$CUSTOM_PATH"
 	wget --quiet --directory-prefix="$CUSTOM_PATH" \
diff --git a/ci/run-style-check.sh b/ci/run-style-check.sh
new file mode 100755
index 0000000000..76dd37d22b
--- /dev/null
+++ b/ci/run-style-check.sh
@@ -0,0 +1,8 @@
+#!/bin/sh
+#
+# Perform style check
+#
+
+baseCommit=$1
+
+git clang-format --style file --diff --extensions c,h "$baseCommit"
-- 
2.45.1

[PATCH v2 8/8] check-whitespace: detect if no base_commit is provided

[Karthik Nayak]

The 'check-whitespace' CI script exits gracefully if no base commit is
provided or if an invalid revision is provided. This is not good because
if a particular CI provides an incorrect base_commit, it would fail
successfully.

This is exactly the case with the GitLab CI. The CI is using the
"$CI_MERGE_REQUEST_TARGET_BRANCH_SHA" variable to get the base commit
SHA, but variable is only defined for _merged_ pipelines. So it is empty
for regular pipelines [1]. This should've failed the check-whitespace
job.

Let's fallback to 'CI_MERGE_REQUEST_DIFF_BASE_SHA' if
"CI_MERGE_REQUEST_TARGET_BRANCH_SHA" isn't available in GitLab CI,
similar to the previous commit. Let's also add a check for incorrect
base_commit in the 'check-whitespace.sh' script. While here, fix a small
typo too.

[1]: https://docs.gitlab.com/ee/ci/variables/predefined_variables.html#predefined-variables-for-merge-request-pipelines

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .gitlab-ci.yml         |  7 ++++++-
 ci/check-whitespace.sh | 10 ++++++++--
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index dc43fc8ba8..c5a18f655a 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -119,7 +119,12 @@ check-whitespace:
   before_script:
     - ./ci/install-dependencies.sh
   script:
-    - ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
+    - |
+      if [ -z ${CI_MERGE_REQUEST_TARGET_BRANCH_SHA} ]; then
+        ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
+      else
+        ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
+      fi
   rules:
     - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
 
diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh
index db399097a5..c40804394c 100755
--- a/ci/check-whitespace.sh
+++ b/ci/check-whitespace.sh
@@ -9,7 +9,7 @@ baseCommit=$1
 outputFile=$2
 url=$3
 
-if test "$#" -ne 1 && test "$#" -ne 3
+if test "$#" -ne 1 && test "$#" -ne 3 || test -z "$1"
 then
 	echo "USAGE: $0 <BASE_COMMIT> [<OUTPUT_FILE> <URL>]"
 	exit 1
@@ -21,6 +21,12 @@ commitText=
 commitTextmd=
 goodParent=
 
+if ! git rev-parse --quiet --verify "${baseCommit}"
+then
+    echo "Invalid <BASE_COMMIT> '${baseCommit}'"
+    exit 1
+fi
+
 while read dash sha etc
 do
 	case "${dash}" in
@@ -67,7 +73,7 @@ then
 		goodParent=${baseCommit: 0:7}
 	fi
 
-	echo "A whitespace issue was found in onen of more of the commits."
+	echo "A whitespace issue was found in one or more of the commits."
 	echo "Run the following command to resolve whitespace issues:"
 	echo "git rebase --whitespace=fix ${goodParent}"
 
-- 
2.45.1

Re: [PATCH 8/8] check-whitespace: detect if no base_commit is provided

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

>> I was commenting on the exit status check done here:
>>
>>     +gitLogOutput=$(git log --check --pretty=format:"---% h% s" "${baseCommit}"..)
>>     +if test $? -ne 0
>>     +then
>>     +	echo -n $gitLogOutput
>>     +	exit 1
>>     +fi
>>
>> Even though the output is captured in a variable, the exit status of
>> "git log --check" is still seen by the shell and "if test $? = 0"
>> next line say "ah, the thing exited with non-zero status so lets
>> echo the whole thing and exit with 1", before it gets to the while
>> loop we have below the above piece of code, no?
>
> My bad, I thought you were referring to the code before my changes. Yes,
> here you're right, we don't need the check since the shell would capture
> the non-zero status.

OK.  

Because in the next round, you'd be checking the error condition
of "git rev-parse $baseCommit" or something that is specifically
designed to check the validity of user input, and not the error
result from the actual "log --check", the above becomes moot ;-)

Thanks.

Re: [PATCH v2 8/8] check-whitespace: detect if no base_commit is provided

[Justin Tobler]

On 24/07/11 10:30AM, Karthik Nayak wrote:
> The 'check-whitespace' CI script exits gracefully if no base commit is
> provided or if an invalid revision is provided. This is not good because
> if a particular CI provides an incorrect base_commit, it would fail
> successfully.
> 
> This is exactly the case with the GitLab CI. The CI is using the
> "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA" variable to get the base commit
> SHA, but variable is only defined for _merged_ pipelines. So it is empty
> for regular pipelines [1]. This should've failed the check-whitespace
> job.
> 
> Let's fallback to 'CI_MERGE_REQUEST_DIFF_BASE_SHA' if
> "CI_MERGE_REQUEST_TARGET_BRANCH_SHA" isn't available in GitLab CI,
> similar to the previous commit. Let's also add a check for incorrect
> base_commit in the 'check-whitespace.sh' script. While here, fix a small
> typo too.
> 
> [1]: https://docs.gitlab.com/ee/ci/variables/predefined_variables.html#predefined-variables-for-merge-request-pipelines
> 
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  .gitlab-ci.yml         |  7 ++++++-
>  ci/check-whitespace.sh | 10 ++++++++--
>  2 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index dc43fc8ba8..c5a18f655a 100644
> --- a/.gitlab-ci.yml
> +++ b/.gitlab-ci.yml
> @@ -119,7 +119,12 @@ check-whitespace:
>    before_script:
>      - ./ci/install-dependencies.sh
>    script:
> -    - ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
> +    - |
> +      if [ -z ${CI_MERGE_REQUEST_TARGET_BRANCH_SHA} ]; then
> +        ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
> +      else
> +        ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
> +      fi

Not worth a reroll, but it would be nice to have a comment here
explaining why we have this fallback as, to me at least, it is not very
obvious.

>    rules:
>      - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
>  
[snip]

Overall the GitLab CI changes look good to me. Thanks :)

-Justin

Re: [PATCH v2 8/8] check-whitespace: detect if no base_commit is provided

[Junio C Hamano]

Justin Tobler <jltobler@gmail.com> writes:

>> +      if [ -z ${CI_MERGE_REQUEST_TARGET_BRANCH_SHA} ]; then
>> +        ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
>> +      else
>> +        ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
>> +      fi
>
> Not worth a reroll, but it would be nice to have a comment here
> explaining why we have this fallback as, to me at least, it is not very
> obvious.

FWIW, it is not obvious to me, either ;-)

Another thing that I find somewhat disturbing is that the
conditional seems the other way around.  It shouldn't be saying "If
B is not available, use A, otherwise use B", as if A is known to be
usable always.  It should be more like

	if test -n "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
	then
		ci/check-whitespace.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
	elif test -n "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
	then
		ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
	else
		... noop?  barf? ...
	fi

shouldn't it?

>>    rules:
>>      - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
>>  
> [snip]
>
> Overall the GitLab CI changes look good to me. Thanks :)

Thanks for a review.  Very much appreciated.

Re: [PATCH v2 5/8] clang-format: avoid braces on simple single-statement bodies

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

>     Setting this option to true could lead to incorrect code formatting
>     due to clang-format’s lack of complete semantic information. As
>     such, extra care should be taken to review code changes made by
>     this option.
>
> The latter seems to be of concern. But since we only use clang-format to
> verify the format and not to apply formatting, we should be okay here.

Hmph.  Could you tell me where I can read more about "we tell
clang-format only to verify but not to apply"?  If that is truely
the case, perhaps I shouldn't be worried to much, but it is not
clear to me how we enforce that this is to be used only for
verification with non-zero false positive, and never for
reformatting before submission.

The senario I was worried about was this.  We aadd to .clang-format
that is in-tree, and not just CI jobs but our human contributors may
use it to check what they newly wrote before committing and they may
even take the differences as suggested fixes (which may end up
breaking their working code).

Thanks.

Re: [PATCH v2 0/8] clang-format: add more rules and enable on CI

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> This is v2 of my series to add clang-format to the CI.

Aside from a few comments, the series looked good.

Will queue.  Thanks.

Re: [PATCH v2 5/8] clang-format: avoid braces on simple single-statement bodies

[Karthik Nayak]

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

Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>>     Setting this option to true could lead to incorrect code formatting
>>     due to clang-format’s lack of complete semantic information. As
>>     such, extra care should be taken to review code changes made by
>>     this option.
>>
>> The latter seems to be of concern. But since we only use clang-format to
>> verify the format and not to apply formatting, we should be okay here.
>
> Hmph.  Could you tell me where I can read more about "we tell
> clang-format only to verify but not to apply"?  If that is truely
> the case, perhaps I shouldn't be worried to much, but it is not
> clear to me how we enforce that this is to be used only for
> verification with non-zero false positive, and never for
> reformatting before submission.
>

I was referring to the fact that, we expose '.clang-format' via 'make
style' which only prints the diff to the STDOUT. The user has to still
manually make these changes.

However users could be using tools to auto-format on save and this could
be an issue.

> The senario I was worried about was this.  We aadd to .clang-format
> that is in-tree, and not just CI jobs but our human contributors may
> use it to check what they newly wrote before committing and they may
> even take the differences as suggested fixes (which may end up
> breaking their working code).
>
> Thanks.

I totally see your point here.

If the contributors do end up with bad formatting because clang messed
it up, that is an okay situation, since that shouldn't happen often, and
when it does, it would be the same situation as without this check,
wherein we rely on reviewers. The issue is whether it would break their
code, I couldn't find anything on this.

But overall, while I personally find this check useful, I'm happy to
drop it, My goal is to ensure we run this on CI as a first step :)

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

Re: [PATCH v2 8/8] check-whitespace: detect if no base_commit is provided

[Karthik Nayak]

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

Junio C Hamano <gitster@pobox.com> writes:

> Justin Tobler <jltobler@gmail.com> writes:
>
>>> +      if [ -z ${CI_MERGE_REQUEST_TARGET_BRANCH_SHA} ]; then
>>> +        ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
>>> +      else
>>> +        ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
>>> +      fi
>>
>> Not worth a reroll, but it would be nice to have a comment here
>> explaining why we have this fallback as, to me at least, it is not very
>> obvious.
>
> FWIW, it is not obvious to me, either ;-)
>
> Another thing that I find somewhat disturbing is that the
> conditional seems the other way around.  It shouldn't be saying "If
> B is not available, use A, otherwise use B", as if A is known to be
> usable always.  It should be more like
>
> 	if test -n "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
> 	then
> 		ci/check-whitespace.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
> 	elif test -n "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
> 	then
> 		ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
> 	else
> 		... noop?  barf? ...
> 	fi
>
> shouldn't it?
>

Agreed, that a comment would be nice here. Will add if I reroll!

In this case A ("$CI_MERGE_REQUEST_DIFF_BASE_SHA") is known to be usable
always [1].

[1]: https://docs.gitlab.com/ee/ci/variables/predefined_variables.html

>>>    rules:
>>>      - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
>>>
>> [snip]
>>
>> Overall the GitLab CI changes look good to me. Thanks :)
>
> Thanks for a review.  Very much appreciated.

Thanks both of you!

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

Re: [PATCH v2 5/8] clang-format: avoid braces on simple single-statement bodies

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>>     Setting this option to true could lead to incorrect code formatting
>>>     due to clang-format’s lack of complete semantic information. As
>>>     such, extra care should be taken to review code changes made by
>>>     this option.
>>>
>>> The latter seems to be of concern. But since we only use clang-format to
>>> verify the format and not to apply formatting, we should be okay here.
>>
>> Hmph.  Could you tell me where I can read more about "we tell
>> clang-format only to verify but not to apply"?  If that is truely
>> the case, perhaps I shouldn't be worried to much, but it is not
>> clear to me how we enforce that this is to be used only for
>> verification with non-zero false positive, and never for
>> reformatting before submission.
>>
>
> I was referring to the fact that, we expose '.clang-format' via 'make
> style' which only prints the diff to the STDOUT. The user has to still
> manually make these changes.
>
> However users could be using tools to auto-format on save and this could
> be an issue.

Yes.

Of course if we really wanted to avoid end-user confusion and still
want to have this in CI (if only to see how well the rule fares, and
what the actual false-positive rate is), we _could_ run CI's job
with custom .clang-format file that is not visible to end-users in
their normal checkout, or something silly like that.  If we are
going to use it, then we should use it everywhere, making sure
everybody is careful.  If the cost of forcing everybody to be
careful is too high, we may want to retract it, but we won't know
until we try.

Thanks.

Re: [PATCH v2 8/8] check-whitespace: detect if no base_commit is provided

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

>> Another thing that I find somewhat disturbing is that the
>> conditional seems the other way around.  It shouldn't be saying "If
>> B is not available, use A, otherwise use B", as if A is known to be
>> usable always.  It should be more like ...
>> shouldn't it?
>>
>
> Agreed, that a comment would be nice here. Will add if I reroll!
>
> In this case A ("$CI_MERGE_REQUEST_DIFF_BASE_SHA") is known to be usable
> always [1].
>
> [1]: https://docs.gitlab.com/ee/ci/variables/predefined_variables.html

Ok, if so check the one you want to use, if exists, first, and then
fall back to what is supposed to exist always but is your second
choice, e.g.,

 	if test -n "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
 	then
 		ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
 	elif 
 	then test -n "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
 		ci/check-whitespace.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
 	else
 		BUG CI_MERGE_REQUEST_DIFF_BASE_SHA should always exist!
 	fi

Thanks.

Re: [PATCH 5/8] clang-format: avoid braces on simple single-statement bodies

[Phillip Wood]

Hi Karthik

On 08/07/2024 21:25, Karthik Nayak wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> Set the 'RemoveBracesLLVM' to 'true' which ensures that we avoid curly
>>> braces for single-statement bodies in conditional blocks.
>>
>> Hmph, two warnings in its documentation [*] sound ominous, especially
>> the second one that says:
>>
>>      Warning
>>
>>      Setting this option to true could lead to incorrect code
>>      formatting due to clang-format’s lack of complete semantic
>>      information. As such, extra care should be taken to review code
>>      changes made by this option.
>>
>> which implies it may not necessarily a good idea to add to
>> automation without telling contributors that they may get hit with a
>> false positive (or incorrect rewrite).
> 
> Agreed on this one. I'm a bit skeptical to be honest too. I think I
> should have added information about the warning in the commit too. I
> will for next round. Overall, this also contributes to the reason why I
> decided these CI jobs need to be allowed to fail.

I'm a bit confused - what does "allowed to fail" mean? We don't have any 
automated requirements on the CI passing. We don't want to train 
contributors to routinely ignore CI failures but if they don't get a 
notification that this job failed how do they know to correct the 
formatting?

>> Reading from the examples in that documentation page, it was unclear
>> how it would handle if/else if/.../else cascade where not all branches
>> are multi-statement blocks, e.g.,
>>
>> 	if (A) {
>> 		do_A_thing();
>> 	} else if (B) {
>> 		do_B_thing();
>> 	} else {
>> 		do_C_things();
>> 		do_other_things();
>> 	}
>>
>> but looking around I am getting a feeling that the tool would do the
>> right thing, i.e., to match our preference that is to use {braces}
>> around all branches, if I am not mistaken.
>>
> 
> Yup, that was my understanding and what I could see from some quick
> trials that I did too.
> 
> It would be a great win to have this though, because it is one of the
> things that always get me.

Yes, having the formatting automated would be really helpful. It is a 
shame the documentation doesn't really explain what the issue is (i.e. 
when does it generate invalid code) so we don't know how likely we are 
to be affected by it.

Best Wishes

Phillip

>>
>> [Reference]
>>
>>   * https://releases.llvm.org/16.0.0/tools/clang/docs/ClangFormatStyleOptions.html#:~:text=RemoveBracesLLVM
>>
>>> +# Remove optional braces of control statements (if, else, for, and while)
>>> +# according to the LLVM coding style
>>> +# This avoids braces on simple single-statement bodies of statements.
>>
>> "... but keeps braces if one side of if/else if/.../else cascade has
>> multi-statement body."
>>
> 
> Makes sense, will add.
> 
>>> +RemoveBracesLLVM: true
> 
> Overall I must say, I'd be okay if this patch is also dropped from this
> series.

Re: [PATCH v2 5/8] clang-format: avoid braces on simple single-statement bodies

[Phillip Wood]

On 12/07/2024 16:09, Junio C Hamano wrote:

> Of course if we really wanted to avoid end-user confusion and still
> want to have this in CI (if only to see how well the rule fares, and
> what the actual false-positive rate is), we _could_ run CI's job
> with custom .clang-format file that is not visible to end-users in
> their normal checkout, or something silly like that.

Getting some idea of how useful the auto-formatter is would be valuable 
I think.

> If we are
> going to use it, then we should use it everywhere, making sure
> everybody is careful.  If the cost of forcing everybody to be
> careful is too high, we may want to retract it, but we won't know
> until we try.

It would be great if we could find a way to auto-format the code without 
inconveniencing contributors.

Best Wishes

Phillip

Re: [PATCH 5/8] clang-format: avoid braces on simple single-statement bodies

[Karthik Nayak]

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

Phillip Wood <phillip.wood123@gmail.com> writes:

> Hi Karthik
>
> On 08/07/2024 21:25, Karthik Nayak wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Karthik Nayak <karthik.188@gmail.com> writes:
>>>
>>>> Set the 'RemoveBracesLLVM' to 'true' which ensures that we avoid curly
>>>> braces for single-statement bodies in conditional blocks.
>>>
>>> Hmph, two warnings in its documentation [*] sound ominous, especially
>>> the second one that says:
>>>
>>>      Warning
>>>
>>>      Setting this option to true could lead to incorrect code
>>>      formatting due to clang-format’s lack of complete semantic
>>>      information. As such, extra care should be taken to review code
>>>      changes made by this option.
>>>
>>> which implies it may not necessarily a good idea to add to
>>> automation without telling contributors that they may get hit with a
>>> false positive (or incorrect rewrite).
>>
>> Agreed on this one. I'm a bit skeptical to be honest too. I think I
>> should have added information about the warning in the commit too. I
>> will for next round. Overall, this also contributes to the reason why I
>> decided these CI jobs need to be allowed to fail.
>
> I'm a bit confused - what does "allowed to fail" mean? We don't have any
> automated requirements on the CI passing. We don't want to train
> contributors to routinely ignore CI failures but if they don't get a
> notification that this job failed how do they know to correct the
> formatting?
>

Well, it mostly means that the CI would show the style-check job as
failed, but the overall pipeline would be successful. We want to ideally
fail the pipeline too, but I'm being careful to not disrupt things
suddenly and I think once we see what false positive rate is and once we
fine tune this settings maybe over the next release or so, we can
enforce this.

[snip]

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

[PATCH v3 0/8] clang-format: add more rules and enable on CI

[Karthik Nayak]

This is v3 of my series to add clang-format to the CI.

The series was mostly inspired by a patch sent recently to 'include
kh_foreach* macros in ForEachMacros' [1]. I was wondering why we don't
run the formatting on CI and reduce some of the workload of reviewers.

We have a '.clang-format' file to provide rules for code formatting.
The commits 1-5 aims to add more rules to the file while deprecating old
ones.

Commit 6 enables CI action on GitHub and GitLab to also check for the
code formatting. Currently, it is allowed to fail. This way we can build
confidence and fine tune the values as needed and finally enforce the
check in a future patch. I'm not well versed with GitHub workflows, and
I mostly tried to copy existing work there. Expecting some feedback in
that section!

Commit 7 fixes an existing issue with the 'check-whitespace' job, which
is failing as success in the GitLab CI.

Commit 8 adds the `RemoveBracesLLVM` only in the context of the CI. If we
decide against it, we could drop this commit from the series.

1: https://lore.kernel.org/git/4e7893f5-2dd9-46cf-8a64-cf780f4e1730@web.de/

Changes against v2:
* Dropped the commit to add 'RemoveBracesLLVM' to the in-tree '.clang-format'.
Now, we only add it in the CI context.
* Added comments for the reasoning and picking of the ENV variables in GitLab's
CI. Also changed the syntax to be more readable.

Karthik Nayak (8):
  clang-format: indent preprocessor directives after hash
  clang-format: avoid spacing around bitfield colon
  clang-format: ensure files end with newlines
  clang-format: replace deprecated option with 'SpacesInParens'
  clang-format: formalize some of the spacing rules
  ci: run style check on GitHub and GitLab
  check-whitespace: detect if no base_commit is provided
  ci/style-check: add `RemoveBracesLLVM` to '.clang-format'

 .clang-format                     | 36 +++++++++++++++++++++++++----
 .github/workflows/check-style.yml | 29 +++++++++++++++++++++++
 .gitlab-ci.yml                    | 38 ++++++++++++++++++++++++++++++-
 ci/check-whitespace.sh            | 10 ++++++--
 ci/install-dependencies.sh        |  2 +-
 ci/run-style-check.sh             | 21 +++++++++++++++++
 6 files changed, 127 insertions(+), 9 deletions(-)
 create mode 100644 .github/workflows/check-style.yml
 create mode 100755 ci/run-style-check.sh

Range-diff against v2:
1:  6cf91ffc86 = 1:  6cf91ffc86 clang-format: indent preprocessor directives after hash
2:  beb002885f = 2:  beb002885f clang-format: avoid spacing around bitfield colon
3:  3031be43e7 = 3:  3031be43e7 clang-format: ensure files end with newlines
4:  bc1550e300 = 4:  bc1550e300 clang-format: replace deprecated option with 'SpacesInParens'
5:  840318a4c9 < -:  ---------- clang-format: avoid braces on simple single-statement bodies
6:  0ecfd8d24b ! 5:  4586c0094b clang-format: formalize some of the spacing rules
    @@ Commit message
         Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
     
      ## .clang-format ##
    -@@ .clang-format: RemoveBracesLLVM: true
    +@@ .clang-format: PointerAlignment: Right
      # x = (int32)y;    not    x = (int32) y;
      SpaceAfterCStyleCast: false
      
7:  11a9ac4935 ! 6:  c18cb23369 ci: run style check on GitHub and GitLab
    @@ .gitlab-ci.yml: check-whitespace:
     +    CC: clang
     +  before_script:
     +    - ./ci/install-dependencies.sh
    ++  # Since $CI_MERGE_REQUEST_TARGET_BRANCH_SHA is only defined for merged
    ++  # pipelines, we fallback to $CI_MERGE_REQUEST_DIFF_BASE_SHA, which should
    ++  # be defined in all pipelines.
     +  script:
     +    - |
    -+      if [ -z ${CI_MERGE_REQUEST_TARGET_BRANCH_SHA} ]; then
    ++      if test -n "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
    ++      then
    ++        ./ci/run-style-check.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
    ++      elif test -n "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
    ++      then
     +        ./ci/run-style-check.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
     +      else
    -+        ./ci/run-style-check.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
    ++        echo "CI_MERGE_REQUEST_DIFF_BASE_SHA should always exist!"; exit 1
     +      fi
     +  rules:
     +    - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
8:  caccbf8264 ! 7:  4d08c570bb check-whitespace: detect if no base_commit is provided
    @@ Commit message
     
      ## .gitlab-ci.yml ##
     @@ .gitlab-ci.yml: check-whitespace:
    +   image: ubuntu:latest
        before_script:
          - ./ci/install-dependencies.sh
    ++  # Since $CI_MERGE_REQUEST_TARGET_BRANCH_SHA is only defined for merged
    ++  # pipelines, we fallback to $CI_MERGE_REQUEST_DIFF_BASE_SHA, which should
    ++  # be defined in all pipelines.
        script:
     -    - ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
     +    - |
    -+      if [ -z ${CI_MERGE_REQUEST_TARGET_BRANCH_SHA} ]; then
    ++      if test -n "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
    ++      then
    ++        ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
    ++      elif test -n "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
    ++      then
     +        ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
     +      else
    -+        ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
    ++        echo "CI_MERGE_REQUEST_DIFF_BASE_SHA should always exist!"; exit 1
     +      fi
        rules:
          - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
-:  ---------- > 8:  2b39431f93 ci/style-check: add `RemoveBracesLLVM` to '.clang-format'
-- 
2.45.2

[PATCH v3 1/8] clang-format: indent preprocessor directives after hash

[Karthik Nayak]

We do not have a rule around the indentation of preprocessor directives.
This was also discussed on the list [1], noting how there is often
inconsistency in the styling. While there was discussion, there was no
conclusion around what is the preferred style here. One style being
indenting after the hash:

    #if FOO
    #  if BAR
    #    include <foo>
    #  endif
    #endif

The other being before the hash:

    #if FOO
      #if BAR
        #include <foo>
      #endif
    #endif

Let's pick the former and add 'IndentPPDirectives: AfterHash' value to
our '.clang-format'. There is no clear reason to pick one over the
other, but it would definitely be nicer to be consistent.

[1]: https://lore.kernel.org/r/xmqqwmmm1bw6.fsf@gitster.g

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .clang-format | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/.clang-format b/.clang-format
index 3ed4fac753..5e128519bf 100644
--- a/.clang-format
+++ b/.clang-format
@@ -96,6 +96,12 @@ BreakStringLiterals: false
 # Switch statement body is always indented one level more than case labels.
 IndentCaseLabels: false
 
+# Indents directives before the hash.
+# #if FOO
+# #  include <foo>
+# #endif
+IndentPPDirectives: AfterHash
+
 # Don't indent a function definition or declaration if it is wrapped after the
 # type
 IndentWrappedFunctionNames: false
-- 
2.45.2

[PATCH v3 2/8] clang-format: avoid spacing around bitfield colon

[Karthik Nayak]

The spacing around colons is currently not standardized and as such we
have the following practices in our code base:
- Spacing around the colon `int bf : 1`: 146 instances
- No spacing around the colon `int bf:1`: 148 instances
- Spacing before the colon `int bf :1`: 6 instances
- Spacing after the colon `int bf: 1`: 12 instances

Let's formalize this by picking the most followed pattern and add the
corresponding style to '.clang-format'.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .clang-format | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/.clang-format b/.clang-format
index 5e128519bf..803b274dd5 100644
--- a/.clang-format
+++ b/.clang-format
@@ -72,6 +72,10 @@ AlwaysBreakAfterReturnType: None
 BinPackArguments: true
 BinPackParameters: true
 
+# Add no space around the bit field
+# unsigned bf:2;
+BitFieldColonSpacing: None
+
 # Attach braces to surrounding context except break before braces on function
 # definitions.
 # void foo()
-- 
2.45.2

[PATCH v3 3/8] clang-format: ensure files end with newlines

[Karthik Nayak]

All our source files end with a newline, let's formalize in
'.clang-format'.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .clang-format | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/.clang-format b/.clang-format
index 803b274dd5..4c9751a9db 100644
--- a/.clang-format
+++ b/.clang-format
@@ -106,6 +106,9 @@ IndentCaseLabels: false
 # #endif
 IndentPPDirectives: AfterHash
 
+# Insert a newline at end of file if missing
+InsertNewlineAtEOF: true
+
 # Don't indent a function definition or declaration if it is wrapped after the
 # type
 IndentWrappedFunctionNames: false
-- 
2.45.2

[PATCH v3 4/8] clang-format: replace deprecated option with 'SpacesInParens'

[Karthik Nayak]

Replace the deprecated options 'SpacesInParentheses' and
'SpaceInEmptyParentheses' with the newer 'SpacesInParens' option. The
usage is the same.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .clang-format | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/.clang-format b/.clang-format
index 4c9751a9db..914254a29b 100644
--- a/.clang-format
+++ b/.clang-format
@@ -134,8 +134,6 @@ SpaceBeforeAssignmentOperators: true
 # }
 SpaceBeforeParens: ControlStatements
 
-# Don't insert spaces inside empty '()'
-SpaceInEmptyParentheses: false
 
 # The number of spaces before trailing line comments (// - comments).
 # This does not affect trailing block comments (/* - comments).
@@ -149,9 +147,10 @@ SpacesInCStyleCastParentheses: false
 # var arr = [1, 2, 3];    not    var arr = [ 1, 2, 3 ];
 SpacesInContainerLiterals: false
 
-# Don't insert spaces after '(' or before ')'
-# f(arg);    not    f( arg );
-SpacesInParentheses: false
+SpacesInParens: Custom
+SpacesInParensOptions:
+  # Don't insert spaces inside empty '()'
+  InEmptyParentheses: false
 
 # Don't insert spaces after '[' or before ']'
 # int a[5];    not    int a[ 5 ];
-- 
2.45.2

[PATCH v3 5/8] clang-format: formalize some of the spacing rules

[Karthik Nayak]

There are some spacing rules that we follow in the project and it makes
sense to formalize them:
* Ensure there is no space inserted after the logical not '!' operator.
* Ensure there is no space before the case statement's colon.
* Ensure there is no space before the first bracket '[' of an array.
* Ensure there is no space in empty blocks.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .clang-format | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/.clang-format b/.clang-format
index 914254a29b..a0dd1f2eb9 100644
--- a/.clang-format
+++ b/.clang-format
@@ -121,11 +121,18 @@ PointerAlignment: Right
 # x = (int32)y;    not    x = (int32) y;
 SpaceAfterCStyleCast: false
 
+# No space is inserted after the logical not operator
+SpaceAfterLogicalNot: false
+
 # Insert spaces before and after assignment operators
 # int a = 5;    not    int a=5;
 # a += 42;             a+=42;
 SpaceBeforeAssignmentOperators: true
 
+# Spaces will be removed before case colon.
+# case 1: break;    not     case 1 : break;
+SpaceBeforeCaseColon: false
+
 # Put a space before opening parentheses only after control statement keywords.
 # void f() {
 #   if (true) {
@@ -134,6 +141,13 @@ SpaceBeforeAssignmentOperators: true
 # }
 SpaceBeforeParens: ControlStatements
 
+# No space before first '[' in arrays
+# int a[5][5];     not      int a [5][5];
+SpaceBeforeSquareBrackets: false
+
+# No space will be inserted into {}
+# while (true) {}    not    while (true) { }
+SpaceInEmptyBlock: false
 
 # The number of spaces before trailing line comments (// - comments).
 # This does not affect trailing block comments (/* - comments).
-- 
2.45.2

[PATCH v3 6/8] ci: run style check on GitHub and GitLab

[Karthik Nayak]

We don't run style checks on our CI, even though we have a
'.clang-format' setup in the repository. Let's add one, the job will
validate only against the new commits added and will only run on merge
requests. Since we're introducing it for the first time, let's allow
this job to fail, so we can validate if this is useful and eventually
enforce it.

For GitLab, we use the 'CI_MERGE_REQUEST_TARGET_BRANCH_SHA' variable by
default to obtain the base SHA of the merged pipeline (which is only
available for merged pipelines [1]). Otherwise we use the
'CI_MERGE_REQUEST_DIFF_BASE_SHA' variable.

[1]: https://docs.gitlab.com/ee/ci/variables/predefined_variables.html#predefined-variables-for-merge-request-pipelines

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .github/workflows/check-style.yml | 29 +++++++++++++++++++++++++++++
 .gitlab-ci.yml                    | 24 ++++++++++++++++++++++++
 ci/install-dependencies.sh        |  2 +-
 ci/run-style-check.sh             |  8 ++++++++
 4 files changed, 62 insertions(+), 1 deletion(-)
 create mode 100644 .github/workflows/check-style.yml
 create mode 100755 ci/run-style-check.sh

diff --git a/.github/workflows/check-style.yml b/.github/workflows/check-style.yml
new file mode 100644
index 0000000000..27276dfe5e
--- /dev/null
+++ b/.github/workflows/check-style.yml
@@ -0,0 +1,29 @@
+name: check-style
+
+# Get the repository with all commits to ensure that we can analyze
+# all of the commits contributed via the Pull Request.
+
+on:
+  pull_request:
+    types: [opened, synchronize]
+
+# Avoid unnecessary builds. Unlike the main CI jobs, these are not
+# ci-configurable (but could be).
+concurrency:
+  group: ${{ github.workflow }}-${{ github.ref }}
+  cancel-in-progress: true
+
+jobs:
+  check-style:
+    runs-on: ubuntu-latest
+    steps:
+    - uses: actions/checkout@v4
+      with:
+        fetch-depth: 0
+
+    - name: git clang-format
+      continue-on-error: true
+      id: check_out
+      run: |
+        ./ci/run-style-check.sh \
+          "${{github.event.pull_request.base.sha}}"
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 37b991e080..14f9d3dc8e 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -123,6 +123,30 @@ check-whitespace:
   rules:
     - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
 
+check-style:
+  image: ubuntu:latest
+  allow_failure: true
+  variables:
+    CC: clang
+  before_script:
+    - ./ci/install-dependencies.sh
+  # Since $CI_MERGE_REQUEST_TARGET_BRANCH_SHA is only defined for merged
+  # pipelines, we fallback to $CI_MERGE_REQUEST_DIFF_BASE_SHA, which should
+  # be defined in all pipelines.
+  script:
+    - |
+      if test -n "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
+      then
+        ./ci/run-style-check.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
+      elif test -n "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
+      then
+        ./ci/run-style-check.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
+      else
+        echo "CI_MERGE_REQUEST_DIFF_BASE_SHA should always exist!"; exit 1
+      fi
+  rules:
+    - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
+
 documentation:
   image: ubuntu:latest
   variables:
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 6ec0f85972..46fe12a690 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -43,7 +43,7 @@ ubuntu-*)
 		make libssl-dev libcurl4-openssl-dev libexpat-dev wget sudo default-jre \
 		tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl \
 		libemail-valid-perl libio-pty-perl libio-socket-ssl-perl libnet-smtp-ssl-perl libdbd-sqlite3-perl libcgi-pm-perl \
-		${CC_PACKAGE:-${CC:-gcc}} $PYTHON_PACKAGE
+		${CC_PACKAGE:-${CC:-gcc}} $PYTHON_PACKAGE clang-format
 
 	mkdir --parents "$CUSTOM_PATH"
 	wget --quiet --directory-prefix="$CUSTOM_PATH" \
diff --git a/ci/run-style-check.sh b/ci/run-style-check.sh
new file mode 100755
index 0000000000..76dd37d22b
--- /dev/null
+++ b/ci/run-style-check.sh
@@ -0,0 +1,8 @@
+#!/bin/sh
+#
+# Perform style check
+#
+
+baseCommit=$1
+
+git clang-format --style file --diff --extensions c,h "$baseCommit"
-- 
2.45.2

[PATCH v3 7/8] check-whitespace: detect if no base_commit is provided

[Karthik Nayak]

The 'check-whitespace' CI script exits gracefully if no base commit is
provided or if an invalid revision is provided. This is not good because
if a particular CI provides an incorrect base_commit, it would fail
successfully.

This is exactly the case with the GitLab CI. The CI is using the
"$CI_MERGE_REQUEST_TARGET_BRANCH_SHA" variable to get the base commit
SHA, but variable is only defined for _merged_ pipelines. So it is empty
for regular pipelines [1]. This should've failed the check-whitespace
job.

Let's fallback to 'CI_MERGE_REQUEST_DIFF_BASE_SHA' if
"CI_MERGE_REQUEST_TARGET_BRANCH_SHA" isn't available in GitLab CI,
similar to the previous commit. Let's also add a check for incorrect
base_commit in the 'check-whitespace.sh' script. While here, fix a small
typo too.

[1]: https://docs.gitlab.com/ee/ci/variables/predefined_variables.html#predefined-variables-for-merge-request-pipelines

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .gitlab-ci.yml         | 14 +++++++++++++-
 ci/check-whitespace.sh | 10 ++++++++--
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 14f9d3dc8e..0596a01b68 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -118,8 +118,20 @@ check-whitespace:
   image: ubuntu:latest
   before_script:
     - ./ci/install-dependencies.sh
+  # Since $CI_MERGE_REQUEST_TARGET_BRANCH_SHA is only defined for merged
+  # pipelines, we fallback to $CI_MERGE_REQUEST_DIFF_BASE_SHA, which should
+  # be defined in all pipelines.
   script:
-    - ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
+    - |
+      if test -n "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
+      then
+        ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
+      elif test -n "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
+      then
+        ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
+      else
+        echo "CI_MERGE_REQUEST_DIFF_BASE_SHA should always exist!"; exit 1
+      fi
   rules:
     - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
 
diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh
index db399097a5..c40804394c 100755
--- a/ci/check-whitespace.sh
+++ b/ci/check-whitespace.sh
@@ -9,7 +9,7 @@ baseCommit=$1
 outputFile=$2
 url=$3
 
-if test "$#" -ne 1 && test "$#" -ne 3
+if test "$#" -ne 1 && test "$#" -ne 3 || test -z "$1"
 then
 	echo "USAGE: $0 <BASE_COMMIT> [<OUTPUT_FILE> <URL>]"
 	exit 1
@@ -21,6 +21,12 @@ commitText=
 commitTextmd=
 goodParent=
 
+if ! git rev-parse --quiet --verify "${baseCommit}"
+then
+    echo "Invalid <BASE_COMMIT> '${baseCommit}'"
+    exit 1
+fi
+
 while read dash sha etc
 do
 	case "${dash}" in
@@ -67,7 +73,7 @@ then
 		goodParent=${baseCommit: 0:7}
 	fi
 
-	echo "A whitespace issue was found in onen of more of the commits."
+	echo "A whitespace issue was found in one or more of the commits."
 	echo "Run the following command to resolve whitespace issues:"
 	echo "git rebase --whitespace=fix ${goodParent}"
 
-- 
2.45.2

[PATCH v3 8/8] ci/style-check: add `RemoveBracesLLVM` to '.clang-format'

[Karthik Nayak]

For 'clang-format' setting  'RemoveBracesLLVM' to 'true', adds a check
to ensure we avoid curly braces for single-statement bodies in
conditional blocks.

However, the option does come with two warnings [1]:

    This option will be renamed and expanded to support other styles.

and

    Setting this option to true could lead to incorrect code formatting
    due to clang-format’s lack of complete semantic information. As
    such, extra care should be taken to review code changes made by
    this option.

The latter seems to be of concern. While we want to experiment with the
rule, adding it to the in-tree '.clang-format' could affect end-users.
Let's only add it to the CI jobs for now. With time, we can evaluate
its efficacy and decide if we want to add it to '.clang-format' or
retract it entirely.

[1]: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#removebracesllvm

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 ci/run-style-check.sh | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/ci/run-style-check.sh b/ci/run-style-check.sh
index 76dd37d22b..1dce50a0ac 100755
--- a/ci/run-style-check.sh
+++ b/ci/run-style-check.sh
@@ -5,4 +5,17 @@
 
 baseCommit=$1
 
+# Remove optional braces of control statements (if, else, for, and while)
+# according to the LLVM coding style. This avoids braces on simple
+# single-statement bodies of statements but keeps braces if one side of
+# if/else if/.../else cascade has multi-statement body.
+#
+# As this rule comes with a warning [1], we want to experiment with it
+# before adding it in-tree. since the CI job for the style check is allowed
+# to fail, appending the rule here allows us to validate its efficacy.
+# While also ensuring that end-users are not affected directly.
+#
+# [1]: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#removebracesllvm
+echo "RemoveBracesLLVM: true" >> .clang-format
+
 git clang-format --style file --diff --extensions c,h "$baseCommit"
-- 
2.45.2

Re: [PATCH 5/8] clang-format: avoid braces on simple single-statement bodies

[phillip.wood123]

On 13/07/2024 13:30, Karthik Nayak wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
 >
>> I'm a bit confused - what does "allowed to fail" mean? We don't have any
>> automated requirements on the CI passing. We don't want to train
>> contributors to routinely ignore CI failures but if they don't get a
>> notification that this job failed how do they know to correct the
>> formatting?
>>
> 
> Well, it mostly means that the CI would show the style-check job as
> failed, but the overall pipeline would be successful.

Ah, I didn't know that was a possibility - do users still get emails 
about the failed job?

> We want to ideally
> fail the pipeline too, but I'm being careful to not disrupt things
> suddenly and I think once we see what false positive rate is and once we
> fine tune this settings maybe over the next release or so, we can
> enforce this.

Starting gently to get some experience with the auto formatter sounds 
sensible. Thanks for working on this, I really hope that once we've got 
more experience with clang format we can figure out how to enable it 
unconditionally.

Best Wishes

Phillip

Re: [PATCH 5/8] clang-format: avoid braces on simple single-statement bodies

[Karthik Nayak]

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

phillip.wood123@gmail.com writes:

> On 13/07/2024 13:30, Karthik Nayak wrote:
>> Phillip Wood <phillip.wood123@gmail.com> writes:
>  >
>>> I'm a bit confused - what does "allowed to fail" mean? We don't have any
>>> automated requirements on the CI passing. We don't want to train
>>> contributors to routinely ignore CI failures but if they don't get a
>>> notification that this job failed how do they know to correct the
>>> formatting?
>>>
>>
>> Well, it mostly means that the CI would show the style-check job as
>> failed, but the overall pipeline would be successful.
>
> Ah, I didn't know that was a possibility - do users still get emails
> about the failed job?
>

I know in GitLab, users _wouldn't_ get an email. This would be a nice
intermediate step, but I think it is still okay.

>> We want to ideally
>> fail the pipeline too, but I'm being careful to not disrupt things
>> suddenly and I think once we see what false positive rate is and once we
>> fine tune this settings maybe over the next release or so, we can
>> enforce this.
>
> Starting gently to get some experience with the auto formatter sounds
> sensible. Thanks for working on this, I really hope that once we've got
> more experience with clang format we can figure out how to enable it
> unconditionally.
>
> Best Wishes
>
> Phillip

Just to clarify, enabling an auto-formatter would always be left to the
user. From the project's perspective, the goal would be to have checks
to notify us (us as in the Git project) if a particular patch series
fails to comply to our standards.

This is done by enforcing the CI rule _eventually_. Users can also
benefit from this if they use the CI for development purposes. One good
example being users of GitGitGadget, where they would be notified of
failures in the CI. Another example is contributions coming from GitLab,
where our team uses the CI before sending patches upstream.

Since the Git project already comes with a '.clang-format', users can
already enable auto-formatting on their end. I know the clangd LSP
already supports this. ;)

Thanks
Karthik

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

Re: [PATCH v3 8/8] ci/style-check: add `RemoveBracesLLVM` to '.clang-format'

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> +# [1]: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#removebracesllvm
> +echo "RemoveBracesLLVM: true" >> .clang-format
> +
>  git clang-format --style file --diff --extensions c,h "$baseCommit"

Style: lose the SP between the redirection operator and its operand.

I know this is well intentioned, but will there be any downsides for
running the check always against a dirty working tree?  

I know during a CI run, the working tree will not be completely
clean, as we create and leave build artifacts, but this is as far as
I know the first instance of us munging a tracked path, changing the
working tree in a way that "git describe --dirty" would notice.

This is done as the last (and only) step of check-style job and the
ci/run-style-check.sh script may not do anything like "git describe
--dirty" right now, but things can change.  Perhaps I am worried
about this a bit too much.

I unfortunately couldn't find an option to "git clang-format" to
tell it to read from an extra file in addition to the usual
".clang-format" file---if such an option existed, we obviously could
use an untracked/ignored file to prepare the custom format file and
use it without making the working tree dirty.

So perhaps the posted change, while making us feel dirty, is the
best we could do for this step.

Thanks.

Re: [PATCH v3 6/8] ci: run style check on GitHub and GitLab

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> +  # Since $CI_MERGE_REQUEST_TARGET_BRANCH_SHA is only defined for merged
> +  # pipelines, we fallback to $CI_MERGE_REQUEST_DIFF_BASE_SHA, which should
> +  # be defined in all pipelines.
> +  script:
> +    - |
> +      if test -n "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
> +      then
> +        ./ci/run-style-check.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
> +      elif test -n "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
> +      then
> +        ./ci/run-style-check.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
> +      else
> +        echo "CI_MERGE_REQUEST_DIFF_BASE_SHA should always exist!"; exit 1
> +      fi

This is fine but we may want to reduce the repetition of the long path
to the script, e.g., by doing something like

	if test -n "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
	then
		R=$CI_MERGE_REQUEST_TARGET_BRANCH_SHA
	elif test -n "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
	then
		R="$CI_MERGE_REQUEST_DIFF_BASE_SHA"
	fi

	if test -z "$R"
	then
		echo "CI_MERGE_REQUEST_DIFF_BASE_SHA should always exist!"
		exit 1
	fi
	./ci/run-style-check.sh "$R"

in a separate "after the dust settles" clean-up #leftoverbits topic.
We could replace the first 7 lines with a single-liner

	R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA?}}

if we wanted to, but all of that will be mere clean-up changes.

Re: [PATCH v3 8/8] ci/style-check: add `RemoveBracesLLVM` to '.clang-format'

[Karthik Nayak]

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

Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +# [1]: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#removebracesllvm
>> +echo "RemoveBracesLLVM: true" >> .clang-format
>> +
>>  git clang-format --style file --diff --extensions c,h "$baseCommit"
>
> Style: lose the SP between the redirection operator and its operand.
>

Will change.

> I know this is well intentioned, but will there be any downsides for
> running the check always against a dirty working tree?
>
> I know during a CI run, the working tree will not be completely
> clean, as we create and leave build artifacts, but this is as far as
> I know the first instance of us munging a tracked path, changing the
> working tree in a way that "git describe --dirty" would notice.
>
> This is done as the last (and only) step of check-style job and the
> ci/run-style-check.sh script may not do anything like "git describe
> --dirty" right now, but things can change.  Perhaps I am worried
> about this a bit too much.
>

Exactly my thoughts too. I did test on GitLab's CI and all other jobs
were unaffected. So I think we're good here.

---

After reading your mail, I figured I should also check GitHub's CI for
this particular change and realized there is a slight issue with my
current series.

GitLab's CI highlights style check jobs which failed with a yellow
warning symbol [1], even with the 'ignore failing check-style'
directive. But GitHub's actions, simply marks the job as successful even
if the job failed [1]. This was an oversight on my side, since I
expected similar behavior. Seems like the required dependency wasn't
even installed on GitHub causing 'git clang-format' to fail.

Unfortunately this means all GitHub workflows for style-check will be
green even if there were style issues found. I couldn't find a way to
fix this from reading the documentation.

This will not be an issue once we enforce, but till then users cannot
rely on the job's outcome for the job's status in GitHub. They will have
to see the logs to know if style check failed.

I will re-roll with a fix, but will wait a day or so, to avoid spamming.

> I unfortunately couldn't find an option to "git clang-format" to
> tell it to read from an extra file in addition to the usual
> ".clang-format" file---if such an option existed, we obviously could
> use an untracked/ignored file to prepare the custom format file and
> use it without making the working tree dirty.
>

This was also something I looked for, but couldn't find. I should have
added that to the commit message. Will do so in the reroll.

> So perhaps the posted change, while making us feel dirty, is the
> best we could do for this step.
>
> Thanks.

Yes, I think it is okay. I'm hoping we can move it in-tree someday.

[1]: https://gitlab.com/gitlab-org/git/-/pipelines/1372326813
[2]: https://github.com/KarthikNayak/git/actions/runs/9921272597/job/27409047062

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

Re: [PATCH v3 8/8] ci/style-check: add `RemoveBracesLLVM` to '.clang-format'

[Ramsay Jones]

On 13/07/2024 17:46, Karthik Nayak wrote:
> Junio C Hamano <gitster@pobox.com> writes:
[snip]

>> I unfortunately couldn't find an option to "git clang-format" to
>> tell it to read from an extra file in addition to the usual
>> ".clang-format" file---if such an option existed, we obviously could
>> use an untracked/ignored file to prepare the custom format file and
>> use it without making the working tree dirty.
>>
> 
> This was also something I looked for, but couldn't find. I should have
> added that to the commit message. Will do so in the reroll.
> 

I had a need recently to try applying the git '.clang-format' file to a
different project:
 
  $ pwd
  /home/ramsay/sparse
  $ clang-format --style=file:/home/ramsay/git/.clang-format sparse.c >xxx.c
  $ meld sparse.c xxx.c # oh my lord :)

Note that I had to specify '/home/ramsay/' rather than just '~', since it
does not get recognized/expanded in that position:

  $ clang-format --style=file:~/git/.clang-format sparse.c >xxx.c
  Error reading ~/git/.clang-format: No such file or directory
  $ rm xxx.c

Also, as you can see, this was 'clang-format' not 'git-clang-format' (which
is what would actually be used in this situation), but the '--help' text
claims that:

  $ git-clang-format --help | grep style
    clangFormat.style
    --style STYLE         passed to clang-format
  $ 

.. so it should work (but I have not actually tried it, so YMMV ;) ).

[So, munging the .clang-format file with sed (say) to a temp file and
using the --style=file:tmp-file syntax should (hopefully) work]

ATB,
Ramsay Jones

    

Re: [PATCH v3 8/8] ci/style-check: add `RemoveBracesLLVM` to '.clang-format'

[Karthik Nayak]

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

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

> On 13/07/2024 17:46, Karthik Nayak wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
> [snip]
>
>>> I unfortunately couldn't find an option to "git clang-format" to
>>> tell it to read from an extra file in addition to the usual
>>> ".clang-format" file---if such an option existed, we obviously could
>>> use an untracked/ignored file to prepare the custom format file and
>>> use it without making the working tree dirty.
>>>
>>
>> This was also something I looked for, but couldn't find. I should have
>> added that to the commit message. Will do so in the reroll.
>>
>
> I had a need recently to try applying the git '.clang-format' file to a
> different project:
>
>   $ pwd
>   /home/ramsay/sparse
>   $ clang-format --style=file:/home/ramsay/git/.clang-format sparse.c >xxx.c
>   $ meld sparse.c xxx.c # oh my lord :)
>
> Note that I had to specify '/home/ramsay/' rather than just '~', since it
> does not get recognized/expanded in that position:
>
>   $ clang-format --style=file:~/git/.clang-format sparse.c >xxx.c
>   Error reading ~/git/.clang-format: No such file or directory
>   $ rm xxx.c
>
> Also, as you can see, this was 'clang-format' not 'git-clang-format' (which
> is what would actually be used in this situation), but the '--help' text
> claims that:
>
>   $ git-clang-format --help | grep style
>     clangFormat.style
>     --style STYLE         passed to clang-format
>   $
>
> .. so it should work (but I have not actually tried it, so YMMV ;) ).
>
> [So, munging the .clang-format file with sed (say) to a temp file and
> using the --style=file:tmp-file syntax should (hopefully) work]
>
> ATB,
> Ramsay Jones
>
>

Hello,

Providing a path does work indeed. But we were discussing the option to
provide an additional path apart from the default '.clang-format'. The
option you mentioned `--style=<file path>` will set the config to the
contents of <file path>, but we want to add on top of that. So that we
could hypothetically do something like

  $ git clang-format --style file --style-append '.ci-clang-format'
--diff --extensions c,h

But seems like this is currently not possible.

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

[PATCH v4 0/8] clang-format: add more rules and enable on CI

[Karthik Nayak]

This is v3 of my series to add clang-format to the CI.

The series was mostly inspired by a patch sent recently to 'include
kh_foreach* macros in ForEachMacros' [1]. I was wondering why we don't
run the formatting on CI and reduce some of the workload of reviewers.

We have a '.clang-format' file to provide rules for code formatting.
The commits 1-5 aims to add more rules to the file while deprecating old
ones.

Commit 6 enables CI action on GitHub and GitLab to also check for the
code formatting. Currently, it is allowed to fail. This way we can build
confidence and fine tune the values as needed and finally enforce the
check in a future patch. I'm not well versed with GitHub workflows, and
I mostly tried to copy existing work there. Expecting some feedback in
that section!

Commit 7 fixes an existing issue with the 'check-whitespace' job, which
is failing as success in the GitLab CI.

Commit 8 adds the `RemoveBracesLLVM` only in the context of the CI. If we
decide against it, we could drop this commit from the series.

1: https://lore.kernel.org/git/4e7893f5-2dd9-46cf-8a64-cf780f4e1730@web.de/

Changes against v3:
- The job was actually failing on GitHub but showing a success since we added
the allow to fail clause. This was because the dependency wasn't installed.
Have fixed that in this version.
- Simplified the bash script in the GitLab job, thanks to Junio.
- Fixed a style issue with redirection. 
- Added some more details to the commit messages. 

Job running on:
- GitHub: https://github.com/KarthikNayak/git/actions/runs/9936188558/job/27443871938?pr=1
- GitLab: https://gitlab.com/gitlab-org/git/-/jobs/7340770469

Karthik Nayak (8):
  clang-format: indent preprocessor directives after hash
  clang-format: avoid spacing around bitfield colon
  clang-format: ensure files end with newlines
  clang-format: replace deprecated option with 'SpacesInParens'
  clang-format: formalize some of the spacing rules
  ci: run style check on GitHub and GitLab
  check-whitespace: detect if no base_commit is provided
  ci/style-check: add `RemoveBracesLLVM` to '.clang-format'

 .clang-format                     | 36 +++++++++++++++++++++++++-----
 .github/workflows/check-style.yml | 34 ++++++++++++++++++++++++++++
 .gitlab-ci.yml                    | 37 ++++++++++++++++++++++++++++++-
 ci/check-whitespace.sh            | 10 +++++++--
 ci/install-dependencies.sh        |  6 ++++-
 ci/run-style-check.sh             | 21 ++++++++++++++++++
 6 files changed, 135 insertions(+), 9 deletions(-)
 create mode 100644 .github/workflows/check-style.yml
 create mode 100755 ci/run-style-check.sh

Range-diff against v3:
1:  6cf91ffc86 = 1:  6cf91ffc86 clang-format: indent preprocessor directives after hash
2:  beb002885f = 2:  beb002885f clang-format: avoid spacing around bitfield colon
3:  3031be43e7 = 3:  3031be43e7 clang-format: ensure files end with newlines
4:  bc1550e300 = 4:  bc1550e300 clang-format: replace deprecated option with 'SpacesInParens'
5:  4586c0094b = 5:  4586c0094b clang-format: formalize some of the spacing rules
6:  c18cb23369 ! 6:  f8eba92c0c ci: run style check on GitHub and GitLab
    @@ Commit message
         this job to fail, so we can validate if this is useful and eventually
         enforce it.
     
    -    For GitLab, we use the 'CI_MERGE_REQUEST_TARGET_BRANCH_SHA' variable by
    -    default to obtain the base SHA of the merged pipeline (which is only
    -    available for merged pipelines [1]). Otherwise we use the
    +    For GitHub, we allow the job to pass by adding 'continue-on-error: true'
    +    to the workflow. This means the job would show as passed, even if the
    +    style check failed. To know the status of the job, users have to
    +    manually check the logs.
    +
    +    For GitLab, we allow the job to pass by adding 'allow_failure: true', to
    +    the job. Unlike GitHub, here the job will show as failed with a yellow
    +    warning symbol, but the pipeline would still show as passed.
    +
    +    Also for GitLab, we use the 'CI_MERGE_REQUEST_TARGET_BRANCH_SHA'
    +    variable by default to obtain the base SHA of the merged pipeline (which
    +    is only available for merged pipelines [1]). Otherwise we use the
         'CI_MERGE_REQUEST_DIFF_BASE_SHA' variable.
     
         [1]: https://docs.gitlab.com/ee/ci/variables/predefined_variables.html#predefined-variables-for-merge-request-pipelines
     
    +    Helped-by: Junio C Hamano <gitster@pobox.com>
         Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
     
      ## .github/workflows/check-style.yml (new) ##
    @@ .github/workflows/check-style.yml (new)
     +
     +jobs:
     +  check-style:
    ++    env:
    ++      CC: clang
    ++      jobname: ClangFormat
     +    runs-on: ubuntu-latest
     +    steps:
     +    - uses: actions/checkout@v4
     +      with:
     +        fetch-depth: 0
     +
    ++    - run: ci/install-dependencies.sh
    ++
     +    - name: git clang-format
     +      continue-on-error: true
     +      id: check_out
    @@ .gitlab-ci.yml: check-whitespace:
     +  allow_failure: true
     +  variables:
     +    CC: clang
    ++    jobname: ClangFormat
     +  before_script:
     +    - ./ci/install-dependencies.sh
     +  # Since $CI_MERGE_REQUEST_TARGET_BRANCH_SHA is only defined for merged
    @@ .gitlab-ci.yml: check-whitespace:
     +  # be defined in all pipelines.
     +  script:
     +    - |
    -+      if test -n "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
    -+      then
    -+        ./ci/run-style-check.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
    -+      elif test -n "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
    ++      R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA?}}
    ++
    ++      if test -z "$R"
     +      then
    -+        ./ci/run-style-check.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
    -+      else
    -+        echo "CI_MERGE_REQUEST_DIFF_BASE_SHA should always exist!"; exit 1
    ++        echo "CI_MERGE_REQUEST_DIFF_BASE_SHA should always exist!"
    ++        exit 1
     +      fi
    ++      ./ci/run-style-check.sh "$R"
     +  rules:
     +    - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
     +
    @@ ci/install-dependencies.sh: ubuntu-*)
      
      	mkdir --parents "$CUSTOM_PATH"
      	wget --quiet --directory-prefix="$CUSTOM_PATH" \
    +@@ ci/install-dependencies.sh: macos-*)
    + esac
    + 
    + case "$jobname" in
    ++ClangFormat)
    ++	sudo apt-get -q update
    ++	sudo apt-get -q -y install clang-format
    ++	;;
    + StaticAnalysis)
    + 	sudo apt-get -q update
    + 	sudo apt-get -q -y install coccinelle libcurl4-openssl-dev libssl-dev \
     
      ## ci/run-style-check.sh (new) ##
     @@
7:  4d08c570bb ! 7:  477419fc6b check-whitespace: detect if no base_commit is provided
    @@ Commit message
     
         [1]: https://docs.gitlab.com/ee/ci/variables/predefined_variables.html#predefined-variables-for-merge-request-pipelines
     
    +    Helped-by: Junio C Hamano <gitster@pobox.com>
         Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
     
      ## .gitlab-ci.yml ##
    @@ .gitlab-ci.yml: check-whitespace:
        script:
     -    - ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
     +    - |
    -+      if test -n "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
    -+      then
    -+        ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
    -+      elif test -n "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
    ++      R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA?}}
    ++
    ++      if test -z "$R"
     +      then
    -+        ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
    -+      else
    -+        echo "CI_MERGE_REQUEST_DIFF_BASE_SHA should always exist!"; exit 1
    ++        echo "CI_MERGE_REQUEST_DIFF_BASE_SHA should always exist!"
    ++        exit 1
     +      fi
    ++      ./ci/check-whitespace.sh "$R"
        rules:
          - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
      
8:  2b39431f93 ! 8:  50007c17da ci/style-check: add `RemoveBracesLLVM` to '.clang-format'
    @@ Commit message
         its efficacy and decide if we want to add it to '.clang-format' or
         retract it entirely.
     
    +    A more ideal solution would be if 'clang-format' allowed us to append
    +    rules to the existing '.clang-format' when invoked. But such an option
    +    does not exist. Since modifying the in-tree '.clang-format' is only done
    +    on the CI job for style-check and doesn't affect any other jobs and is
    +    not persisted in any ways, this hack should be okay.
    +
         [1]: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#removebracesllvm
     
         Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
    @@ ci/run-style-check.sh
     +# While also ensuring that end-users are not affected directly.
     +#
     +# [1]: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#removebracesllvm
    -+echo "RemoveBracesLLVM: true" >> .clang-format
    ++echo "RemoveBracesLLVM: true" >>.clang-format
     +
      git clang-format --style file --diff --extensions c,h "$baseCommit"
-- 
2.45.2

[PATCH v4 1/8] clang-format: indent preprocessor directives after hash

[Karthik Nayak]

We do not have a rule around the indentation of preprocessor directives.
This was also discussed on the list [1], noting how there is often
inconsistency in the styling. While there was discussion, there was no
conclusion around what is the preferred style here. One style being
indenting after the hash:

    #if FOO
    #  if BAR
    #    include <foo>
    #  endif
    #endif

The other being before the hash:

    #if FOO
      #if BAR
        #include <foo>
      #endif
    #endif

Let's pick the former and add 'IndentPPDirectives: AfterHash' value to
our '.clang-format'. There is no clear reason to pick one over the
other, but it would definitely be nicer to be consistent.

[1]: https://lore.kernel.org/r/xmqqwmmm1bw6.fsf@gitster.g

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .clang-format | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/.clang-format b/.clang-format
index 3ed4fac753..5e128519bf 100644
--- a/.clang-format
+++ b/.clang-format
@@ -96,6 +96,12 @@ BreakStringLiterals: false
 # Switch statement body is always indented one level more than case labels.
 IndentCaseLabels: false
 
+# Indents directives before the hash.
+# #if FOO
+# #  include <foo>
+# #endif
+IndentPPDirectives: AfterHash
+
 # Don't indent a function definition or declaration if it is wrapped after the
 # type
 IndentWrappedFunctionNames: false
-- 
2.45.2

[PATCH v4 2/8] clang-format: avoid spacing around bitfield colon

[Karthik Nayak]

The spacing around colons is currently not standardized and as such we
have the following practices in our code base:
- Spacing around the colon `int bf : 1`: 146 instances
- No spacing around the colon `int bf:1`: 148 instances
- Spacing before the colon `int bf :1`: 6 instances
- Spacing after the colon `int bf: 1`: 12 instances

Let's formalize this by picking the most followed pattern and add the
corresponding style to '.clang-format'.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .clang-format | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/.clang-format b/.clang-format
index 5e128519bf..803b274dd5 100644
--- a/.clang-format
+++ b/.clang-format
@@ -72,6 +72,10 @@ AlwaysBreakAfterReturnType: None
 BinPackArguments: true
 BinPackParameters: true
 
+# Add no space around the bit field
+# unsigned bf:2;
+BitFieldColonSpacing: None
+
 # Attach braces to surrounding context except break before braces on function
 # definitions.
 # void foo()
-- 
2.45.2

[PATCH v4 3/8] clang-format: ensure files end with newlines

[Karthik Nayak]

All our source files end with a newline, let's formalize in
'.clang-format'.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .clang-format | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/.clang-format b/.clang-format
index 803b274dd5..4c9751a9db 100644
--- a/.clang-format
+++ b/.clang-format
@@ -106,6 +106,9 @@ IndentCaseLabels: false
 # #endif
 IndentPPDirectives: AfterHash
 
+# Insert a newline at end of file if missing
+InsertNewlineAtEOF: true
+
 # Don't indent a function definition or declaration if it is wrapped after the
 # type
 IndentWrappedFunctionNames: false
-- 
2.45.2

[PATCH v4 4/8] clang-format: replace deprecated option with 'SpacesInParens'

[Karthik Nayak]

Replace the deprecated options 'SpacesInParentheses' and
'SpaceInEmptyParentheses' with the newer 'SpacesInParens' option. The
usage is the same.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .clang-format | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/.clang-format b/.clang-format
index 4c9751a9db..914254a29b 100644
--- a/.clang-format
+++ b/.clang-format
@@ -134,8 +134,6 @@ SpaceBeforeAssignmentOperators: true
 # }
 SpaceBeforeParens: ControlStatements
 
-# Don't insert spaces inside empty '()'
-SpaceInEmptyParentheses: false
 
 # The number of spaces before trailing line comments (// - comments).
 # This does not affect trailing block comments (/* - comments).
@@ -149,9 +147,10 @@ SpacesInCStyleCastParentheses: false
 # var arr = [1, 2, 3];    not    var arr = [ 1, 2, 3 ];
 SpacesInContainerLiterals: false
 
-# Don't insert spaces after '(' or before ')'
-# f(arg);    not    f( arg );
-SpacesInParentheses: false
+SpacesInParens: Custom
+SpacesInParensOptions:
+  # Don't insert spaces inside empty '()'
+  InEmptyParentheses: false
 
 # Don't insert spaces after '[' or before ']'
 # int a[5];    not    int a[ 5 ];
-- 
2.45.2

[PATCH v4 5/8] clang-format: formalize some of the spacing rules

[Karthik Nayak]

There are some spacing rules that we follow in the project and it makes
sense to formalize them:
* Ensure there is no space inserted after the logical not '!' operator.
* Ensure there is no space before the case statement's colon.
* Ensure there is no space before the first bracket '[' of an array.
* Ensure there is no space in empty blocks.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .clang-format | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/.clang-format b/.clang-format
index 914254a29b..a0dd1f2eb9 100644
--- a/.clang-format
+++ b/.clang-format
@@ -121,11 +121,18 @@ PointerAlignment: Right
 # x = (int32)y;    not    x = (int32) y;
 SpaceAfterCStyleCast: false
 
+# No space is inserted after the logical not operator
+SpaceAfterLogicalNot: false
+
 # Insert spaces before and after assignment operators
 # int a = 5;    not    int a=5;
 # a += 42;             a+=42;
 SpaceBeforeAssignmentOperators: true
 
+# Spaces will be removed before case colon.
+# case 1: break;    not     case 1 : break;
+SpaceBeforeCaseColon: false
+
 # Put a space before opening parentheses only after control statement keywords.
 # void f() {
 #   if (true) {
@@ -134,6 +141,13 @@ SpaceBeforeAssignmentOperators: true
 # }
 SpaceBeforeParens: ControlStatements
 
+# No space before first '[' in arrays
+# int a[5][5];     not      int a [5][5];
+SpaceBeforeSquareBrackets: false
+
+# No space will be inserted into {}
+# while (true) {}    not    while (true) { }
+SpaceInEmptyBlock: false
 
 # The number of spaces before trailing line comments (// - comments).
 # This does not affect trailing block comments (/* - comments).
-- 
2.45.2

[PATCH v4 6/8] ci: run style check on GitHub and GitLab

[Karthik Nayak]

We don't run style checks on our CI, even though we have a
'.clang-format' setup in the repository. Let's add one, the job will
validate only against the new commits added and will only run on merge
requests. Since we're introducing it for the first time, let's allow
this job to fail, so we can validate if this is useful and eventually
enforce it.

For GitHub, we allow the job to pass by adding 'continue-on-error: true'
to the workflow. This means the job would show as passed, even if the
style check failed. To know the status of the job, users have to
manually check the logs.

For GitLab, we allow the job to pass by adding 'allow_failure: true', to
the job. Unlike GitHub, here the job will show as failed with a yellow
warning symbol, but the pipeline would still show as passed.

Also for GitLab, we use the 'CI_MERGE_REQUEST_TARGET_BRANCH_SHA'
variable by default to obtain the base SHA of the merged pipeline (which
is only available for merged pipelines [1]). Otherwise we use the
'CI_MERGE_REQUEST_DIFF_BASE_SHA' variable.

[1]: https://docs.gitlab.com/ee/ci/variables/predefined_variables.html#predefined-variables-for-merge-request-pipelines

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .github/workflows/check-style.yml | 34 +++++++++++++++++++++++++++++++
 .gitlab-ci.yml                    | 24 ++++++++++++++++++++++
 ci/install-dependencies.sh        |  6 +++++-
 ci/run-style-check.sh             |  8 ++++++++
 4 files changed, 71 insertions(+), 1 deletion(-)
 create mode 100644 .github/workflows/check-style.yml
 create mode 100755 ci/run-style-check.sh

diff --git a/.github/workflows/check-style.yml b/.github/workflows/check-style.yml
new file mode 100644
index 0000000000..c052a5df23
--- /dev/null
+++ b/.github/workflows/check-style.yml
@@ -0,0 +1,34 @@
+name: check-style
+
+# Get the repository with all commits to ensure that we can analyze
+# all of the commits contributed via the Pull Request.
+
+on:
+  pull_request:
+    types: [opened, synchronize]
+
+# Avoid unnecessary builds. Unlike the main CI jobs, these are not
+# ci-configurable (but could be).
+concurrency:
+  group: ${{ github.workflow }}-${{ github.ref }}
+  cancel-in-progress: true
+
+jobs:
+  check-style:
+    env:
+      CC: clang
+      jobname: ClangFormat
+    runs-on: ubuntu-latest
+    steps:
+    - uses: actions/checkout@v4
+      with:
+        fetch-depth: 0
+
+    - run: ci/install-dependencies.sh
+
+    - name: git clang-format
+      continue-on-error: true
+      id: check_out
+      run: |
+        ./ci/run-style-check.sh \
+          "${{github.event.pull_request.base.sha}}"
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 37b991e080..817266226e 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -123,6 +123,30 @@ check-whitespace:
   rules:
     - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
 
+check-style:
+  image: ubuntu:latest
+  allow_failure: true
+  variables:
+    CC: clang
+    jobname: ClangFormat
+  before_script:
+    - ./ci/install-dependencies.sh
+  # Since $CI_MERGE_REQUEST_TARGET_BRANCH_SHA is only defined for merged
+  # pipelines, we fallback to $CI_MERGE_REQUEST_DIFF_BASE_SHA, which should
+  # be defined in all pipelines.
+  script:
+    - |
+      R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA?}}
+
+      if test -z "$R"
+      then
+        echo "CI_MERGE_REQUEST_DIFF_BASE_SHA should always exist!"
+        exit 1
+      fi
+      ./ci/run-style-check.sh "$R"
+  rules:
+    - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
+
 documentation:
   image: ubuntu:latest
   variables:
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 6ec0f85972..8b01bfb89f 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -43,7 +43,7 @@ ubuntu-*)
 		make libssl-dev libcurl4-openssl-dev libexpat-dev wget sudo default-jre \
 		tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl \
 		libemail-valid-perl libio-pty-perl libio-socket-ssl-perl libnet-smtp-ssl-perl libdbd-sqlite3-perl libcgi-pm-perl \
-		${CC_PACKAGE:-${CC:-gcc}} $PYTHON_PACKAGE
+		${CC_PACKAGE:-${CC:-gcc}} $PYTHON_PACKAGE clang-format
 
 	mkdir --parents "$CUSTOM_PATH"
 	wget --quiet --directory-prefix="$CUSTOM_PATH" \
@@ -87,6 +87,10 @@ macos-*)
 esac
 
 case "$jobname" in
+ClangFormat)
+	sudo apt-get -q update
+	sudo apt-get -q -y install clang-format
+	;;
 StaticAnalysis)
 	sudo apt-get -q update
 	sudo apt-get -q -y install coccinelle libcurl4-openssl-dev libssl-dev \
diff --git a/ci/run-style-check.sh b/ci/run-style-check.sh
new file mode 100755
index 0000000000..76dd37d22b
--- /dev/null
+++ b/ci/run-style-check.sh
@@ -0,0 +1,8 @@
+#!/bin/sh
+#
+# Perform style check
+#
+
+baseCommit=$1
+
+git clang-format --style file --diff --extensions c,h "$baseCommit"
-- 
2.45.2

[PATCH v4 7/8] check-whitespace: detect if no base_commit is provided

[Karthik Nayak]

The 'check-whitespace' CI script exits gracefully if no base commit is
provided or if an invalid revision is provided. This is not good because
if a particular CI provides an incorrect base_commit, it would fail
successfully.

This is exactly the case with the GitLab CI. The CI is using the
"$CI_MERGE_REQUEST_TARGET_BRANCH_SHA" variable to get the base commit
SHA, but variable is only defined for _merged_ pipelines. So it is empty
for regular pipelines [1]. This should've failed the check-whitespace
job.

Let's fallback to 'CI_MERGE_REQUEST_DIFF_BASE_SHA' if
"CI_MERGE_REQUEST_TARGET_BRANCH_SHA" isn't available in GitLab CI,
similar to the previous commit. Let's also add a check for incorrect
base_commit in the 'check-whitespace.sh' script. While here, fix a small
typo too.

[1]: https://docs.gitlab.com/ee/ci/variables/predefined_variables.html#predefined-variables-for-merge-request-pipelines

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .gitlab-ci.yml         | 13 ++++++++++++-
 ci/check-whitespace.sh | 10 ++++++++--
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 817266226e..320b78b9ae 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -118,8 +118,19 @@ check-whitespace:
   image: ubuntu:latest
   before_script:
     - ./ci/install-dependencies.sh
+  # Since $CI_MERGE_REQUEST_TARGET_BRANCH_SHA is only defined for merged
+  # pipelines, we fallback to $CI_MERGE_REQUEST_DIFF_BASE_SHA, which should
+  # be defined in all pipelines.
   script:
-    - ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
+    - |
+      R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA?}}
+
+      if test -z "$R"
+      then
+        echo "CI_MERGE_REQUEST_DIFF_BASE_SHA should always exist!"
+        exit 1
+      fi
+      ./ci/check-whitespace.sh "$R"
   rules:
     - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
 
diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh
index db399097a5..c40804394c 100755
--- a/ci/check-whitespace.sh
+++ b/ci/check-whitespace.sh
@@ -9,7 +9,7 @@ baseCommit=$1
 outputFile=$2
 url=$3
 
-if test "$#" -ne 1 && test "$#" -ne 3
+if test "$#" -ne 1 && test "$#" -ne 3 || test -z "$1"
 then
 	echo "USAGE: $0 <BASE_COMMIT> [<OUTPUT_FILE> <URL>]"
 	exit 1
@@ -21,6 +21,12 @@ commitText=
 commitTextmd=
 goodParent=
 
+if ! git rev-parse --quiet --verify "${baseCommit}"
+then
+    echo "Invalid <BASE_COMMIT> '${baseCommit}'"
+    exit 1
+fi
+
 while read dash sha etc
 do
 	case "${dash}" in
@@ -67,7 +73,7 @@ then
 		goodParent=${baseCommit: 0:7}
 	fi
 
-	echo "A whitespace issue was found in onen of more of the commits."
+	echo "A whitespace issue was found in one or more of the commits."
 	echo "Run the following command to resolve whitespace issues:"
 	echo "git rebase --whitespace=fix ${goodParent}"
 
-- 
2.45.2

[PATCH v4 8/8] ci/style-check: add `RemoveBracesLLVM` to '.clang-format'

[Karthik Nayak]

For 'clang-format' setting  'RemoveBracesLLVM' to 'true', adds a check
to ensure we avoid curly braces for single-statement bodies in
conditional blocks.

However, the option does come with two warnings [1]:

    This option will be renamed and expanded to support other styles.

and

    Setting this option to true could lead to incorrect code formatting
    due to clang-format’s lack of complete semantic information. As
    such, extra care should be taken to review code changes made by
    this option.

The latter seems to be of concern. While we want to experiment with the
rule, adding it to the in-tree '.clang-format' could affect end-users.
Let's only add it to the CI jobs for now. With time, we can evaluate
its efficacy and decide if we want to add it to '.clang-format' or
retract it entirely.

A more ideal solution would be if 'clang-format' allowed us to append
rules to the existing '.clang-format' when invoked. But such an option
does not exist. Since modifying the in-tree '.clang-format' is only done
on the CI job for style-check and doesn't affect any other jobs and is
not persisted in any ways, this hack should be okay.

[1]: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#removebracesllvm

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 ci/run-style-check.sh | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/ci/run-style-check.sh b/ci/run-style-check.sh
index 76dd37d22b..16b593fa1b 100755
--- a/ci/run-style-check.sh
+++ b/ci/run-style-check.sh
@@ -5,4 +5,17 @@
 
 baseCommit=$1
 
+# Remove optional braces of control statements (if, else, for, and while)
+# according to the LLVM coding style. This avoids braces on simple
+# single-statement bodies of statements but keeps braces if one side of
+# if/else if/.../else cascade has multi-statement body.
+#
+# As this rule comes with a warning [1], we want to experiment with it
+# before adding it in-tree. since the CI job for the style check is allowed
+# to fail, appending the rule here allows us to validate its efficacy.
+# While also ensuring that end-users are not affected directly.
+#
+# [1]: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#removebracesllvm
+echo "RemoveBracesLLVM: true" >>.clang-format
+
 git clang-format --style file --diff --extensions c,h "$baseCommit"
-- 
2.45.2

Re: [PATCH v3 8/8] ci/style-check: add `RemoveBracesLLVM` to '.clang-format'

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> Providing a path does work indeed. But we were discussing the option to
> provide an additional path apart from the default '.clang-format'.

But that is not a requirement.  The requirement is to allow us to
use what we have in .clang-format plus one other rule.

And if that requirement is met by copying the entire contents in
.clang-format to a temporary file, adding the other one to the same
temporary file, and then using the temporary file instead of
.clang-format, that is fine, isn't it?

Re: [PATCH v3 8/8] ci/style-check: add `RemoveBracesLLVM` to '.clang-format'

[Karthik Nayak]

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

Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Providing a path does work indeed. But we were discussing the option to
>> provide an additional path apart from the default '.clang-format'.
>
> But that is not a requirement.  The requirement is to allow us to
> use what we have in .clang-format plus one other rule.
>
> And if that requirement is met by copying the entire contents in
> .clang-format to a temporary file, adding the other one to the same
> temporary file, and then using the temporary file instead of
> .clang-format, that is fine, isn't it?

Ah right. Let me summarise:
- Method 1: Inject the extra config to '.clang-format' in the CI's job.
This is the current method.
- Method 2: Create '.clang-format-ci' to use in the CI
  - Method 2.a: The new file contains '.clang-format' + CI specific
  rules.
  - Method 2.b: The new file simply contains the new rules and we inject
  the rest in the CI's job.

I'd say methods '1' and '2.b' are similar, since they modify the tree on
the CI. So no real benefit of one over the other, no?

The issue with method '2.a' is that we have two sources of truth and we
need to ensure both files are updates always.

Since the former methods don't have any cons of maintenance apart from
seeming a bit hacky, I'd prefer that. But happy to go the other way too!

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

Re: [PATCH v3 8/8] ci/style-check: add `RemoveBracesLLVM` to '.clang-format'

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> Ah right. Let me summarise:
> - Method 1: Inject the extra config to '.clang-format' in the CI's job.
> This is the current method.
> - Method 2: Create '.clang-format-ci' to use in the CI
>   - Method 2.a: The new file contains '.clang-format' + CI specific
>   rules.
>   - Method 2.b: The new file simply contains the new rules and we inject
>   the rest in the CI's job.
>
> I'd say methods '1' and '2.b' are similar, since they modify the tree on
> the CI. So no real benefit of one over the other, no?

Sorry, but I am not sure what you are trying to say, especially with
2.a and 2.b, your assumption on "the new file".  Is it tracked?

Try running "git describe --dirty" and see if the command can tell
the difference.  If you smudge .clang-format, which is a tracked
file, it will be noticed.

But you can use a temporary file and use --style=file:/... to point
at it.  The temporary file can be an untracked and ignored file,
just like any *.o files we would create during a build.  Then "git
describe --dirty" would not complain that you are making the working
tree dirty.

The temporary file does not even have to be inside our working tree.
If we know we can write into /tmp/clang-format-rules file, then the
CI script can do something like

	{
		cat .clang-format
		echo echo "RemoveBracesLLVM: true"
	} >/tmp/clang-format-rules
	git clang-format --style=file:/tmp/clang-format-rules \
		 --diff --extensions c,h "$baseCommit"

right?  Then "git status" would even say "there is no untracked
cruft" (although I do not know we *need* to keep the working tree
that clean, without untracked cruft).

Re: [PATCH v4 8/8] ci/style-check: add `RemoveBracesLLVM` to '.clang-format'

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> A more ideal solution would be if 'clang-format' allowed us to append
> rules to the existing '.clang-format' when invoked. But such an option
> does not exist. Since modifying the in-tree '.clang-format' is only done
> on the CI job for style-check and doesn't affect any other jobs and is
> not persisted in any ways, this hack should be okay.

I think our mails crossed, but I do not know why this hack is OK.
Are there other CI jobs that muck with tracked files in the working
tree?

Thanks.

Re: [PATCH v3 8/8] ci/style-check: add `RemoveBracesLLVM` to '.clang-format'

[Karthik Nayak]

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

Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Ah right. Let me summarise:
>> - Method 1: Inject the extra config to '.clang-format' in the CI's job.
>> This is the current method.
>> - Method 2: Create '.clang-format-ci' to use in the CI
>>   - Method 2.a: The new file contains '.clang-format' + CI specific
>>   rules.
>>   - Method 2.b: The new file simply contains the new rules and we inject
>>   the rest in the CI's job.
>>
>> I'd say methods '1' and '2.b' are similar, since they modify the tree on
>> the CI. So no real benefit of one over the other, no?
>
> Sorry, but I am not sure what you are trying to say, especially with
> 2.a and 2.b, your assumption on "the new file".  Is it tracked?
>

Yes. I was referring to a tracked file.

> Try running "git describe --dirty" and see if the command can tell
> the difference.  If you smudge .clang-format, which is a tracked
> file, it will be noticed.
>
> But you can use a temporary file and use --style=file:/... to point
> at it.  The temporary file can be an untracked and ignored file,
> just like any *.o files we would create during a build.  Then "git
> describe --dirty" would not complain that you are making the working
> tree dirty.
>
> The temporary file does not even have to be inside our working tree.

I feel stupid now. I completely didn't think of this and this solves
everything. Thanks for being explicit here. The temporary file
absolutely doesn't have to be in our working tree.

> If we know we can write into /tmp/clang-format-rules file, then the
> CI script can do something like
>
> 	{
> 		cat .clang-format
> 		echo echo "RemoveBracesLLVM: true"
> 	} >/tmp/clang-format-rules
> 	git clang-format --style=file:/tmp/clang-format-rules \
> 		 --diff --extensions c,h "$baseCommit"
>
> right?  Then "git status" would even say "there is no untracked
> cruft" (although I do not know we *need* to keep the working tree
> that clean, without untracked cruft).

Yes this is the best solution.

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

Re: [PATCH v4 8/8] ci/style-check: add `RemoveBracesLLVM` to '.clang-format'

[Karthik Nayak]

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

Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> A more ideal solution would be if 'clang-format' allowed us to append
>> rules to the existing '.clang-format' when invoked. But such an option
>> does not exist. Since modifying the in-tree '.clang-format' is only done
>> on the CI job for style-check and doesn't affect any other jobs and is
>> not persisted in any ways, this hack should be okay.
>
> I think our mails crossed, but I do not know why this hack is OK.
> Are there other CI jobs that muck with tracked files in the working
> tree?
>
> Thanks.

I mean from an operation of a CI job, the repository is discarded after
the job. So there isn't a problem with murking with the working tree.

But I agree to your latest response [1], it would be best to do this
with a temporary file outside the working tree.

Will send a new version soon! Thanks

[1]: https://lore.kernel.org/r/xmqqle224npf.fsf@gitster.g

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

Re: [PATCH v3 8/8] ci/style-check: add `RemoveBracesLLVM` to '.clang-format'

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

>> If we know we can write into /tmp/clang-format-rules file, then the
>> CI script can do something like
>>
>> 	{
>> 		cat .clang-format
>> 		echo echo "RemoveBracesLLVM: true"
>> 	} >/tmp/clang-format-rules
>> 	git clang-format --style=file:/tmp/clang-format-rules \
>> 		 --diff --extensions c,h "$baseCommit"
>>
>> right?  Then "git status" would even say "there is no untracked
>> cruft" (although I do not know we *need* to keep the working tree
>> that clean, without untracked cruft).
>
> Yes this is the best solution.

FWIW, I think an in-tree throw-away file is a better option, simply
because we _know_ that the working tree can be written (by the fact
that we can do "make" and have compilers write *.o and other cruft),
but we do not know if the CI environment allows us to write to /tmp
or /var/tmp or /usr/local/tmp and you do not want to run around and
see if there is a suitable temporary directory you can use.



	

[PATCH v5 0/6] : add more rules and enable on CI

[Karthik Nayak]

This is v5 of my series to add clang-format to the CI.

The series was mostly inspired by a patch sent recently to 'include
kh_foreach* macros in ForEachMacros' [1]. I was wondering why we don't
run the formatting on CI and reduce some of the workload of reviewers.

We have a '.clang-format' file to provide rules for code formatting.
The commits 1-3 aims to add more rules to the file.

Commit 4 enables CI action on GitHub and GitLab to also check for the
code formatting. Currently, it is allowed to fail. This way we can build
confidence and fine tune the values as needed and finally enforce the
check in a future patch. I'm not well versed with GitHub workflows, and
I mostly tried to copy existing work there. Expecting some feedback in
that section!

Commit 5 fixes an existing issue with the 'check-whitespace' job, which
is failing as success in the GitLab CI.

Commit 6 adds the `RemoveBracesLLVM` only in the context of the CI. If we
decide against it, we could drop this commit from the series.

1: https://lore.kernel.org/git/4e7893f5-2dd9-46cf-8a64-cf780f4e1730@web.de/

Changes against v4:
- While testing out temp file as source for the CI, I noticed that version
of clang-format on GitHub is much older than that of GitLab, in accordance
with that, I removed some of the rules which are newer and not supported.
- Cleaned up extra dependency addition in 'install-dependencies'. 
- Instead of murking the in-tree '.clang-format', now we create a temporary
file and use that. 

Test jobs:
- With error:
  - GitLab: https://gitlab.com/gitlab-org/git/-/jobs/7357893486
  - GitHub: https://github.com/git/git/actions/runs/9964605815/job/27533243002
- Without error: 
  - GitLab: https://gitlab.com/gitlab-org/git/-/jobs/7357903589
  - GitHub: https://github.com/git/git/actions/runs/9964631696/job/27533328240

Karthik Nayak (6):
  clang-format: indent preprocessor directives after hash
  clang-format: avoid spacing around bitfield colon
  clang-format: formalize some of the spacing rules
  ci: run style check on GitHub and GitLab
  check-whitespace: detect if no base_commit is provided
  ci/style-check: add `RemoveBracesLLVM` in CI job

 .clang-format                     | 25 +++++++++++++++++++++
 .github/workflows/check-style.yml | 34 ++++++++++++++++++++++++++++
 .gitlab-ci.yml                    | 37 ++++++++++++++++++++++++++++++-
 ci/check-whitespace.sh            | 10 +++++++--
 ci/install-dependencies.sh        |  4 ++++
 ci/run-style-check.sh             | 25 +++++++++++++++++++++
 6 files changed, 132 insertions(+), 3 deletions(-)
 create mode 100644 .github/workflows/check-style.yml
 create mode 100755 ci/run-style-check.sh

Range-diff against v4:
1:  6cf91ffc86 = 1:  6cf91ffc86 clang-format: indent preprocessor directives after hash
2:  beb002885f = 2:  beb002885f clang-format: avoid spacing around bitfield colon
3:  3031be43e7 < -:  ---------- clang-format: ensure files end with newlines
4:  bc1550e300 < -:  ---------- clang-format: replace deprecated option with 'SpacesInParens'
5:  4586c0094b ! 3:  3922529001 clang-format: formalize some of the spacing rules
    @@ .clang-format: PointerAlignment: Right
      # Put a space before opening parentheses only after control statement keywords.
      # void f() {
      #   if (true) {
    -@@ .clang-format: SpaceBeforeAssignmentOperators: true
    - # }
    - SpaceBeforeParens: ControlStatements
    +@@ .clang-format: SpaceBeforeParens: ControlStatements
    + # Don't insert spaces inside empty '()'
    + SpaceInEmptyParentheses: false
      
     +# No space before first '[' in arrays
     +# int a[5][5];     not      int a [5][5];
    @@ .clang-format: SpaceBeforeAssignmentOperators: true
     +# No space will be inserted into {}
     +# while (true) {}    not    while (true) { }
     +SpaceInEmptyBlock: false
    - 
    ++
      # The number of spaces before trailing line comments (// - comments).
      # This does not affect trailing block comments (/* - comments).
    + SpacesBeforeTrailingComments: 1
6:  c18cb23369 ! 4:  ae23eb5af8 ci: run style check on GitHub and GitLab
    @@ Commit message
         this job to fail, so we can validate if this is useful and eventually
         enforce it.
     
    -    For GitLab, we use the 'CI_MERGE_REQUEST_TARGET_BRANCH_SHA' variable by
    -    default to obtain the base SHA of the merged pipeline (which is only
    -    available for merged pipelines [1]). Otherwise we use the
    +    For GitHub, we allow the job to pass by adding 'continue-on-error: true'
    +    to the workflow. This means the job would show as passed, even if the
    +    style check failed. To know the status of the job, users have to
    +    manually check the logs.
    +
    +    For GitLab, we allow the job to pass by adding 'allow_failure: true', to
    +    the job. Unlike GitHub, here the job will show as failed with a yellow
    +    warning symbol, but the pipeline would still show as passed.
    +
    +    Also for GitLab, we use the 'CI_MERGE_REQUEST_TARGET_BRANCH_SHA'
    +    variable by default to obtain the base SHA of the merged pipeline (which
    +    is only available for merged pipelines [1]). Otherwise we use the
         'CI_MERGE_REQUEST_DIFF_BASE_SHA' variable.
     
         [1]: https://docs.gitlab.com/ee/ci/variables/predefined_variables.html#predefined-variables-for-merge-request-pipelines
     
    +    Helped-by: Junio C Hamano <gitster@pobox.com>
         Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
     
      ## .github/workflows/check-style.yml (new) ##
    @@ .github/workflows/check-style.yml (new)
     +
     +jobs:
     +  check-style:
    ++    env:
    ++      CC: clang
    ++      jobname: ClangFormat
     +    runs-on: ubuntu-latest
     +    steps:
     +    - uses: actions/checkout@v4
     +      with:
     +        fetch-depth: 0
     +
    ++    - run: ci/install-dependencies.sh
    ++
     +    - name: git clang-format
     +      continue-on-error: true
     +      id: check_out
    @@ .gitlab-ci.yml: check-whitespace:
     +  allow_failure: true
     +  variables:
     +    CC: clang
    ++    jobname: ClangFormat
     +  before_script:
     +    - ./ci/install-dependencies.sh
     +  # Since $CI_MERGE_REQUEST_TARGET_BRANCH_SHA is only defined for merged
    @@ .gitlab-ci.yml: check-whitespace:
     +  # be defined in all pipelines.
     +  script:
     +    - |
    -+      if test -n "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
    -+      then
    -+        ./ci/run-style-check.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
    -+      elif test -n "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
    ++      R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA?}}
    ++
    ++      if test -z "$R"
     +      then
    -+        ./ci/run-style-check.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
    -+      else
    -+        echo "CI_MERGE_REQUEST_DIFF_BASE_SHA should always exist!"; exit 1
    ++        echo "CI_MERGE_REQUEST_DIFF_BASE_SHA should always exist!"
    ++        exit 1
     +      fi
    ++      ./ci/run-style-check.sh "$R"
     +  rules:
     +    - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
     +
    @@ .gitlab-ci.yml: check-whitespace:
        variables:
     
      ## ci/install-dependencies.sh ##
    -@@ ci/install-dependencies.sh: ubuntu-*)
    - 		make libssl-dev libcurl4-openssl-dev libexpat-dev wget sudo default-jre \
    - 		tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl \
    - 		libemail-valid-perl libio-pty-perl libio-socket-ssl-perl libnet-smtp-ssl-perl libdbd-sqlite3-perl libcgi-pm-perl \
    --		${CC_PACKAGE:-${CC:-gcc}} $PYTHON_PACKAGE
    -+		${CC_PACKAGE:-${CC:-gcc}} $PYTHON_PACKAGE clang-format
    +@@ ci/install-dependencies.sh: macos-*)
    + esac
      
    - 	mkdir --parents "$CUSTOM_PATH"
    - 	wget --quiet --directory-prefix="$CUSTOM_PATH" \
    + case "$jobname" in
    ++ClangFormat)
    ++	sudo apt-get -q update
    ++	sudo apt-get -q -y install clang-format
    ++	;;
    + StaticAnalysis)
    + 	sudo apt-get -q update
    + 	sudo apt-get -q -y install coccinelle libcurl4-openssl-dev libssl-dev \
     
      ## ci/run-style-check.sh (new) ##
     @@
7:  4d08c570bb ! 5:  a38cde03a8 check-whitespace: detect if no base_commit is provided
    @@ Commit message
     
         [1]: https://docs.gitlab.com/ee/ci/variables/predefined_variables.html#predefined-variables-for-merge-request-pipelines
     
    +    Helped-by: Junio C Hamano <gitster@pobox.com>
         Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
     
      ## .gitlab-ci.yml ##
    @@ .gitlab-ci.yml: check-whitespace:
        script:
     -    - ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
     +    - |
    -+      if test -n "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
    -+      then
    -+        ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
    -+      elif test -n "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
    ++      R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA?}}
    ++
    ++      if test -z "$R"
     +      then
    -+        ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
    -+      else
    -+        echo "CI_MERGE_REQUEST_DIFF_BASE_SHA should always exist!"; exit 1
    ++        echo "CI_MERGE_REQUEST_DIFF_BASE_SHA should always exist!"
    ++        exit 1
     +      fi
    ++      ./ci/check-whitespace.sh "$R"
        rules:
          - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
      
8:  2b39431f93 ! 6:  008e77bd0a ci/style-check: add `RemoveBracesLLVM` to '.clang-format'
    @@ Metadata
     Author: Karthik Nayak <karthik.188@gmail.com>
     
      ## Commit message ##
    -    ci/style-check: add `RemoveBracesLLVM` to '.clang-format'
    +    ci/style-check: add `RemoveBracesLLVM` in CI job
     
    -    For 'clang-format' setting  'RemoveBracesLLVM' to 'true', adds a check
    +    For 'clang-format', setting 'RemoveBracesLLVM' to 'true', adds a check
         to ensure we avoid curly braces for single-statement bodies in
         conditional blocks.
     
    @@ Commit message
         rule, adding it to the in-tree '.clang-format' could affect end-users.
         Let's only add it to the CI jobs for now. With time, we can evaluate
         its efficacy and decide if we want to add it to '.clang-format' or
    -    retract it entirely.
    +    retract it entirely. We do so, by adding the existing rules in
    +    '.clang-format' and this rule to a temp file outside the working tree,
    +    which is then used by 'git clang-format'. This ensures we don't murk
    +    with files in-tree.
     
         [1]: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#removebracesllvm
     
    @@ ci/run-style-check.sh
      
      baseCommit=$1
      
    +-git clang-format --style file --diff --extensions c,h "$baseCommit"
     +# Remove optional braces of control statements (if, else, for, and while)
     +# according to the LLVM coding style. This avoids braces on simple
     +# single-statement bodies of statements but keeps braces if one side of
    @@ ci/run-style-check.sh
     +# While also ensuring that end-users are not affected directly.
     +#
     +# [1]: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#removebracesllvm
    -+echo "RemoveBracesLLVM: true" >> .clang-format
    ++{
    ++	cat .clang-format
    ++	echo "RemoveBracesLLVM: true"
    ++} >/tmp/clang-format-rules
     +
    - git clang-format --style file --diff --extensions c,h "$baseCommit"
    ++git clang-format --style=file:/tmp/clang-format-rules \
    ++	--diff --extensions c,h "$baseCommit"
-- 
2.45.2

[PATCH v5 1/6] clang-format: indent preprocessor directives after hash

[Karthik Nayak]

We do not have a rule around the indentation of preprocessor directives.
This was also discussed on the list [1], noting how there is often
inconsistency in the styling. While there was discussion, there was no
conclusion around what is the preferred style here. One style being
indenting after the hash:

    #if FOO
    #  if BAR
    #    include <foo>
    #  endif
    #endif

The other being before the hash:

    #if FOO
      #if BAR
        #include <foo>
      #endif
    #endif

Let's pick the former and add 'IndentPPDirectives: AfterHash' value to
our '.clang-format'. There is no clear reason to pick one over the
other, but it would definitely be nicer to be consistent.

[1]: https://lore.kernel.org/r/xmqqwmmm1bw6.fsf@gitster.g

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .clang-format | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/.clang-format b/.clang-format
index 3ed4fac753..5e128519bf 100644
--- a/.clang-format
+++ b/.clang-format
@@ -96,6 +96,12 @@ BreakStringLiterals: false
 # Switch statement body is always indented one level more than case labels.
 IndentCaseLabels: false
 
+# Indents directives before the hash.
+# #if FOO
+# #  include <foo>
+# #endif
+IndentPPDirectives: AfterHash
+
 # Don't indent a function definition or declaration if it is wrapped after the
 # type
 IndentWrappedFunctionNames: false
-- 
2.45.2

[PATCH v5 2/6] clang-format: avoid spacing around bitfield colon

[Karthik Nayak]

The spacing around colons is currently not standardized and as such we
have the following practices in our code base:
- Spacing around the colon `int bf : 1`: 146 instances
- No spacing around the colon `int bf:1`: 148 instances
- Spacing before the colon `int bf :1`: 6 instances
- Spacing after the colon `int bf: 1`: 12 instances

Let's formalize this by picking the most followed pattern and add the
corresponding style to '.clang-format'.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .clang-format | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/.clang-format b/.clang-format
index 5e128519bf..803b274dd5 100644
--- a/.clang-format
+++ b/.clang-format
@@ -72,6 +72,10 @@ AlwaysBreakAfterReturnType: None
 BinPackArguments: true
 BinPackParameters: true
 
+# Add no space around the bit field
+# unsigned bf:2;
+BitFieldColonSpacing: None
+
 # Attach braces to surrounding context except break before braces on function
 # definitions.
 # void foo()
-- 
2.45.2

[PATCH v5 3/6] clang-format: formalize some of the spacing rules

[Karthik Nayak]

There are some spacing rules that we follow in the project and it makes
sense to formalize them:
* Ensure there is no space inserted after the logical not '!' operator.
* Ensure there is no space before the case statement's colon.
* Ensure there is no space before the first bracket '[' of an array.
* Ensure there is no space in empty blocks.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .clang-format | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/.clang-format b/.clang-format
index 803b274dd5..3c8018a1dd 100644
--- a/.clang-format
+++ b/.clang-format
@@ -118,11 +118,18 @@ PointerAlignment: Right
 # x = (int32)y;    not    x = (int32) y;
 SpaceAfterCStyleCast: false
 
+# No space is inserted after the logical not operator
+SpaceAfterLogicalNot: false
+
 # Insert spaces before and after assignment operators
 # int a = 5;    not    int a=5;
 # a += 42;             a+=42;
 SpaceBeforeAssignmentOperators: true
 
+# Spaces will be removed before case colon.
+# case 1: break;    not     case 1 : break;
+SpaceBeforeCaseColon: false
+
 # Put a space before opening parentheses only after control statement keywords.
 # void f() {
 #   if (true) {
@@ -134,6 +141,14 @@ SpaceBeforeParens: ControlStatements
 # Don't insert spaces inside empty '()'
 SpaceInEmptyParentheses: false
 
+# No space before first '[' in arrays
+# int a[5][5];     not      int a [5][5];
+SpaceBeforeSquareBrackets: false
+
+# No space will be inserted into {}
+# while (true) {}    not    while (true) { }
+SpaceInEmptyBlock: false
+
 # The number of spaces before trailing line comments (// - comments).
 # This does not affect trailing block comments (/* - comments).
 SpacesBeforeTrailingComments: 1
-- 
2.45.2

[PATCH v5 4/6] ci: run style check on GitHub and GitLab

[Karthik Nayak]

We don't run style checks on our CI, even though we have a
'.clang-format' setup in the repository. Let's add one, the job will
validate only against the new commits added and will only run on merge
requests. Since we're introducing it for the first time, let's allow
this job to fail, so we can validate if this is useful and eventually
enforce it.

For GitHub, we allow the job to pass by adding 'continue-on-error: true'
to the workflow. This means the job would show as passed, even if the
style check failed. To know the status of the job, users have to
manually check the logs.

For GitLab, we allow the job to pass by adding 'allow_failure: true', to
the job. Unlike GitHub, here the job will show as failed with a yellow
warning symbol, but the pipeline would still show as passed.

Also for GitLab, we use the 'CI_MERGE_REQUEST_TARGET_BRANCH_SHA'
variable by default to obtain the base SHA of the merged pipeline (which
is only available for merged pipelines [1]). Otherwise we use the
'CI_MERGE_REQUEST_DIFF_BASE_SHA' variable.

[1]: https://docs.gitlab.com/ee/ci/variables/predefined_variables.html#predefined-variables-for-merge-request-pipelines

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .github/workflows/check-style.yml | 34 +++++++++++++++++++++++++++++++
 .gitlab-ci.yml                    | 24 ++++++++++++++++++++++
 ci/install-dependencies.sh        |  4 ++++
 ci/run-style-check.sh             |  8 ++++++++
 4 files changed, 70 insertions(+)
 create mode 100644 .github/workflows/check-style.yml
 create mode 100755 ci/run-style-check.sh

diff --git a/.github/workflows/check-style.yml b/.github/workflows/check-style.yml
new file mode 100644
index 0000000000..c052a5df23
--- /dev/null
+++ b/.github/workflows/check-style.yml
@@ -0,0 +1,34 @@
+name: check-style
+
+# Get the repository with all commits to ensure that we can analyze
+# all of the commits contributed via the Pull Request.
+
+on:
+  pull_request:
+    types: [opened, synchronize]
+
+# Avoid unnecessary builds. Unlike the main CI jobs, these are not
+# ci-configurable (but could be).
+concurrency:
+  group: ${{ github.workflow }}-${{ github.ref }}
+  cancel-in-progress: true
+
+jobs:
+  check-style:
+    env:
+      CC: clang
+      jobname: ClangFormat
+    runs-on: ubuntu-latest
+    steps:
+    - uses: actions/checkout@v4
+      with:
+        fetch-depth: 0
+
+    - run: ci/install-dependencies.sh
+
+    - name: git clang-format
+      continue-on-error: true
+      id: check_out
+      run: |
+        ./ci/run-style-check.sh \
+          "${{github.event.pull_request.base.sha}}"
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 37b991e080..817266226e 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -123,6 +123,30 @@ check-whitespace:
   rules:
     - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
 
+check-style:
+  image: ubuntu:latest
+  allow_failure: true
+  variables:
+    CC: clang
+    jobname: ClangFormat
+  before_script:
+    - ./ci/install-dependencies.sh
+  # Since $CI_MERGE_REQUEST_TARGET_BRANCH_SHA is only defined for merged
+  # pipelines, we fallback to $CI_MERGE_REQUEST_DIFF_BASE_SHA, which should
+  # be defined in all pipelines.
+  script:
+    - |
+      R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA?}}
+
+      if test -z "$R"
+      then
+        echo "CI_MERGE_REQUEST_DIFF_BASE_SHA should always exist!"
+        exit 1
+      fi
+      ./ci/run-style-check.sh "$R"
+  rules:
+    - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
+
 documentation:
   image: ubuntu:latest
   variables:
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 6ec0f85972..fb34ced8f0 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -87,6 +87,10 @@ macos-*)
 esac
 
 case "$jobname" in
+ClangFormat)
+	sudo apt-get -q update
+	sudo apt-get -q -y install clang-format
+	;;
 StaticAnalysis)
 	sudo apt-get -q update
 	sudo apt-get -q -y install coccinelle libcurl4-openssl-dev libssl-dev \
diff --git a/ci/run-style-check.sh b/ci/run-style-check.sh
new file mode 100755
index 0000000000..76dd37d22b
--- /dev/null
+++ b/ci/run-style-check.sh
@@ -0,0 +1,8 @@
+#!/bin/sh
+#
+# Perform style check
+#
+
+baseCommit=$1
+
+git clang-format --style file --diff --extensions c,h "$baseCommit"
-- 
2.45.2

[PATCH v5 5/6] check-whitespace: detect if no base_commit is provided

[Karthik Nayak]

The 'check-whitespace' CI script exits gracefully if no base commit is
provided or if an invalid revision is provided. This is not good because
if a particular CI provides an incorrect base_commit, it would fail
successfully.

This is exactly the case with the GitLab CI. The CI is using the
"$CI_MERGE_REQUEST_TARGET_BRANCH_SHA" variable to get the base commit
SHA, but variable is only defined for _merged_ pipelines. So it is empty
for regular pipelines [1]. This should've failed the check-whitespace
job.

Let's fallback to 'CI_MERGE_REQUEST_DIFF_BASE_SHA' if
"CI_MERGE_REQUEST_TARGET_BRANCH_SHA" isn't available in GitLab CI,
similar to the previous commit. Let's also add a check for incorrect
base_commit in the 'check-whitespace.sh' script. While here, fix a small
typo too.

[1]: https://docs.gitlab.com/ee/ci/variables/predefined_variables.html#predefined-variables-for-merge-request-pipelines

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .gitlab-ci.yml         | 13 ++++++++++++-
 ci/check-whitespace.sh | 10 ++++++++--
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 817266226e..320b78b9ae 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -118,8 +118,19 @@ check-whitespace:
   image: ubuntu:latest
   before_script:
     - ./ci/install-dependencies.sh
+  # Since $CI_MERGE_REQUEST_TARGET_BRANCH_SHA is only defined for merged
+  # pipelines, we fallback to $CI_MERGE_REQUEST_DIFF_BASE_SHA, which should
+  # be defined in all pipelines.
   script:
-    - ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
+    - |
+      R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA?}}
+
+      if test -z "$R"
+      then
+        echo "CI_MERGE_REQUEST_DIFF_BASE_SHA should always exist!"
+        exit 1
+      fi
+      ./ci/check-whitespace.sh "$R"
   rules:
     - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
 
diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh
index db399097a5..c40804394c 100755
--- a/ci/check-whitespace.sh
+++ b/ci/check-whitespace.sh
@@ -9,7 +9,7 @@ baseCommit=$1
 outputFile=$2
 url=$3
 
-if test "$#" -ne 1 && test "$#" -ne 3
+if test "$#" -ne 1 && test "$#" -ne 3 || test -z "$1"
 then
 	echo "USAGE: $0 <BASE_COMMIT> [<OUTPUT_FILE> <URL>]"
 	exit 1
@@ -21,6 +21,12 @@ commitText=
 commitTextmd=
 goodParent=
 
+if ! git rev-parse --quiet --verify "${baseCommit}"
+then
+    echo "Invalid <BASE_COMMIT> '${baseCommit}'"
+    exit 1
+fi
+
 while read dash sha etc
 do
 	case "${dash}" in
@@ -67,7 +73,7 @@ then
 		goodParent=${baseCommit: 0:7}
 	fi
 
-	echo "A whitespace issue was found in onen of more of the commits."
+	echo "A whitespace issue was found in one or more of the commits."
 	echo "Run the following command to resolve whitespace issues:"
 	echo "git rebase --whitespace=fix ${goodParent}"
 
-- 
2.45.2

[PATCH v5 6/6] ci/style-check: add `RemoveBracesLLVM` in CI job

[Karthik Nayak]

For 'clang-format', setting 'RemoveBracesLLVM' to 'true', adds a check
to ensure we avoid curly braces for single-statement bodies in
conditional blocks.

However, the option does come with two warnings [1]:

    This option will be renamed and expanded to support other styles.

and

    Setting this option to true could lead to incorrect code formatting
    due to clang-format’s lack of complete semantic information. As
    such, extra care should be taken to review code changes made by
    this option.

The latter seems to be of concern. While we want to experiment with the
rule, adding it to the in-tree '.clang-format' could affect end-users.
Let's only add it to the CI jobs for now. With time, we can evaluate
its efficacy and decide if we want to add it to '.clang-format' or
retract it entirely. We do so, by adding the existing rules in
'.clang-format' and this rule to a temp file outside the working tree,
which is then used by 'git clang-format'. This ensures we don't murk
with files in-tree.

[1]: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#removebracesllvm

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 ci/run-style-check.sh | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/ci/run-style-check.sh b/ci/run-style-check.sh
index 76dd37d22b..6cd4b1d934 100755
--- a/ci/run-style-check.sh
+++ b/ci/run-style-check.sh
@@ -5,4 +5,21 @@
 
 baseCommit=$1
 
-git clang-format --style file --diff --extensions c,h "$baseCommit"
+# Remove optional braces of control statements (if, else, for, and while)
+# according to the LLVM coding style. This avoids braces on simple
+# single-statement bodies of statements but keeps braces if one side of
+# if/else if/.../else cascade has multi-statement body.
+#
+# As this rule comes with a warning [1], we want to experiment with it
+# before adding it in-tree. since the CI job for the style check is allowed
+# to fail, appending the rule here allows us to validate its efficacy.
+# While also ensuring that end-users are not affected directly.
+#
+# [1]: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#removebracesllvm
+{
+	cat .clang-format
+	echo "RemoveBracesLLVM: true"
+} >/tmp/clang-format-rules
+
+git clang-format --style=file:/tmp/clang-format-rules \
+	--diff --extensions c,h "$baseCommit"
-- 
2.45.2

Re: [PATCH v3 8/8] ci/style-check: add `RemoveBracesLLVM` to '.clang-format'

[Karthik Nayak]

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

Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>>> If we know we can write into /tmp/clang-format-rules file, then the
>>> CI script can do something like
>>>
>>> 	{
>>> 		cat .clang-format
>>> 		echo echo "RemoveBracesLLVM: true"
>>> 	} >/tmp/clang-format-rules
>>> 	git clang-format --style=file:/tmp/clang-format-rules \
>>> 		 --diff --extensions c,h "$baseCommit"
>>>
>>> right?  Then "git status" would even say "there is no untracked
>>> cruft" (although I do not know we *need* to keep the working tree
>>> that clean, without untracked cruft).
>>
>> Yes this is the best solution.
>
> FWIW, I think an in-tree throw-away file is a better option, simply
> because we _know_ that the working tree can be written (by the fact
> that we can do "make" and have compilers write *.o and other cruft),
> but we do not know if the CI environment allows us to write to /tmp
> or /var/tmp or /usr/local/tmp and you do not want to run around and
> see if there is a suitable temporary directory you can use.

Sorry I got a bit busy with work. But I did end up testing out writing
to a temp file and it works on both GitHub and GitLab CIs. Also I found
some of the rules are too new for the clang-format in GitHub, so I
removed some of them.

Overall, I've sent a new version with these changes.

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

Re: [PATCH v3 8/8] ci/style-check: add `RemoveBracesLLVM` to '.clang-format'

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> Sorry I got a bit busy with work. But I did end up testing out writing
> to a temp file and it works on both GitHub and GitLab CIs. Also I found
> some of the rules are too new for the clang-format in GitHub, so I
> removed some of them.

Thanks, both are really appreciated, especially the latter about
checking the features that are available.

Re: [PATCH v5 4/6] ci: run style check on GitHub and GitLab

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> +  # Since $CI_MERGE_REQUEST_TARGET_BRANCH_SHA is only defined for merged
> +  # pipelines, we fallback to $CI_MERGE_REQUEST_DIFF_BASE_SHA, which should
> +  # be defined in all pipelines.
> +  script:
> +    - |
> +      R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA?}}
> +
> +      if test -z "$R"
> +      then
> +        echo "CI_MERGE_REQUEST_DIFF_BASE_SHA should always exist!"
> +        exit 1
> +      fi

If you update the assignment to R to

    R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA:?}}

an unset CI_MERGE_REQUEST_DIFF_BASE_SHA1 or an empty value in it
will be rejected there.  Then you can lose the if/then/fi here.

> +      ./ci/run-style-check.sh "$R"
> +  rules:
> +    - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
> +
>  documentation:
>    image: ubuntu:latest
>    variables:
> diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
> index 6ec0f85972..fb34ced8f0 100755
> --- a/ci/install-dependencies.sh
> +++ b/ci/install-dependencies.sh
> @@ -87,6 +87,10 @@ macos-*)
>  esac
>  
>  case "$jobname" in
> +ClangFormat)
> +	sudo apt-get -q update
> +	sudo apt-get -q -y install clang-format
> +	;;
>  StaticAnalysis)
>  	sudo apt-get -q update
>  	sudo apt-get -q -y install coccinelle libcurl4-openssl-dev libssl-dev \
> diff --git a/ci/run-style-check.sh b/ci/run-style-check.sh
> new file mode 100755
> index 0000000000..76dd37d22b
> --- /dev/null
> +++ b/ci/run-style-check.sh
> @@ -0,0 +1,8 @@
> +#!/bin/sh
> +#
> +# Perform style check
> +#
> +
> +baseCommit=$1
> +
> +git clang-format --style file --diff --extensions c,h "$baseCommit"

Re: [PATCH v5 5/6] check-whitespace: detect if no base_commit is provided

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> +    - |
> +      R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA?}}
> +
> +      if test -z "$R"
> +      then
> +        echo "CI_MERGE_REQUEST_DIFF_BASE_SHA should always exist!"
> +        exit 1
> +      fi

The same comment applies as the previous step.

	R=${A-${B:?}} || exit

should be sufficient.

A demonstration.

        $ unset A; unset B; C=${A-${B:?}} && echo "C=$C"
        bash: B: parameter null or not set
        $ A=a; unset B; C=${A-${B:?}} && echo "C=$C"
        C=a
        $ unset A; B=; C=${A-${B:?}} && echo "C=$C"
        bash: B: parameter null or not set
        $ unset A; B=b; C=${A-${B:?}} && echo "C=$C"
        C=b
        $ A=a; B=b; C=${A-${B:?}} && echo "C=$C"
        C=a

Note that the broken case we do not see C=<value> becaues the
assignment fails with non-zero status.

Thanks.

Re: [PATCH v5 0/6] : add more rules and enable on CI

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> Changes against v4:
> - While testing out temp file as source for the CI, I noticed that version
> of clang-format on GitHub is much older than that of GitLab, in accordance
> with that, I removed some of the rules which are newer and not supported.

Let's see what we lost relative to the previous round.
 
        -# Insert a newline at end of file if missing
        -InsertNewlineAtEOF: true

I guess we can do "git diff --check $BASE...HEAD", if we wanted to,
which will trigger 'blank-at-eol' and other whitespace checks.

        +# Don't insert spaces inside empty '()'
        +SpaceInEmptyParentheses: false
        +
        -SpacesInParens: Custom
        -SpacesInParensOptions:
        -  # Don't insert spaces inside empty '()'
        -  InEmptyParentheses: false
        +# Don't insert spaces after '(' or before ')'
        +# f(arg);    not    f( arg );
        +SpacesInParentheses: false
 
This was to express what we used an older construct to express by
using a newer construct?  So we are not losing anything here.

So, I guess loss of changes to a few rules is nothing to shed tears
on.

Unrelated to the rules part, this caught my eyes, but

        diff --git c/ci/install-dependencies.sh w/ci/install-dependencies.sh
        index 8b01bfb89f..fb34ced8f0 100755
        --- c/ci/install-dependencies.sh
        +++ w/ci/install-dependencies.sh
        -		${CC_PACKAGE:-${CC:-gcc}} $PYTHON_PACKAGE clang-format
        +		${CC_PACKAGE:-${CC:-gcc}} $PYTHON_PACKAGE
 
this is to stop installing clang-format everywhere because we are
already installing it in ClangFormat job anyway?  A good change.

Thanks.

Re: [PATCH v5 4/6] ci: run style check on GitHub and GitLab

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
> ...
>> +      R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA?}}
>> +
>> +      if test -z "$R"
>> +      then
>> +        echo "CI_MERGE_REQUEST_DIFF_BASE_SHA should always exist!"
>> +        exit 1
>> +      fi
>
> If you update the assignment to R to
>
>     R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA:?}}
>
> an unset CI_MERGE_REQUEST_DIFF_BASE_SHA1 or an empty value in it
> will be rejected there.  Then you can lose the if/then/fi here.

Actually, you also need to pay attention to the exit status of the
assignment, i.e.

    R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA:?}} ||
    exit

Re: [PATCH v5 5/6] check-whitespace: detect if no base_commit is provided

[Karthik Nayak]

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

Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +    - |
>> +      R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA?}}
>> +
>> +      if test -z "$R"
>> +      then
>> +        echo "CI_MERGE_REQUEST_DIFF_BASE_SHA should always exist!"
>> +        exit 1
>> +      fi
>
> The same comment applies as the previous step.
>
> 	R=${A-${B:?}} || exit
>
> should be sufficient.
>
> A demonstration.
>
>         $ unset A; unset B; C=${A-${B:?}} && echo "C=$C"
>         bash: B: parameter null or not set
>         $ A=a; unset B; C=${A-${B:?}} && echo "C=$C"
>         C=a
>         $ unset A; B=; C=${A-${B:?}} && echo "C=$C"
>         bash: B: parameter null or not set
>         $ unset A; B=b; C=${A-${B:?}} && echo "C=$C"
>         C=b
>         $ A=a; B=b; C=${A-${B:?}} && echo "C=$C"
>         C=a
>
> Note that the broken case we do not see C=<value> becaues the
> assignment fails with non-zero status.
>
> Thanks.

Thanks Junio for explaining with examples, really nice of you! I'm on
the fence with this, even the existing change from the previous more
verbose code. I know this is shorter, but it is always more readable to
use the longer version with 'test'. I find it hard to remember the
specifics. But I don't really care which one makes it in the end.

Let me know if you think it is worth a reroll.

Thanks

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

Re: [PATCH v5 5/6] check-whitespace: detect if no base_commit is provided

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> Thanks Junio for explaining with examples, really nice of you! I'm on
> the fence with this, even the existing change from the previous more
> verbose code. I know this is shorter, but it is always more readable to
> use the longer version with 'test'. I find it hard to remember the
> specifics.

You'd never remember unless you practice, but it boils down to one
question: is it reasonable to expect that most developers who need
to touch this code find it worth to learn to read and write shell
scripts well in this day and age?  The answer is probably no.

As you may remember, this R=${A-${B?}} dance started at

  https://lore.kernel.org/git/xmqqwmlpb8er.fsf@gitster.g/

where I said:

    ...
    in a separate "after the dust settles" clean-up #leftoverbits topic.

    We could replace the first 7 lines with a single-liner

       R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA?}}

    if we wanted to, but all of that will be mere clean-up changes.

Even the longhand to set a single R with if/elif cascade so that we
can have a single location that invokes ci/run-style-check.sh was
considered extra clean-up for #leftoverbits at least by me.

But after seeing you used the ${A-${B?}} dance, which is more
advanced than the #leftoverbits clean-up, I thought you were
interested in using such a construct that pursues parameter
expansion mastery, and that was the primary reason why the
demonstration in the message you are responding to was added.

I personally do not care too deeply which one to use wrt the
readability, but

	R=${A-${B?}}
	if test -z "$R"
	then
		error
	fi

looks strange and inconsistent by spreading the error check to two
places.  The code would be better off if it were

	R=${A-$B}
	if test -z "$R"
	then
		error
	fi

(or with R=${A:-$B}) instead.  Then it makes it clear that the
author wanted to take care of the error case with the if part.

Re: [PATCH v5 5/6] check-whitespace: detect if no base_commit is provided

[Karthik Nayak]

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

Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Thanks Junio for explaining with examples, really nice of you! I'm on
>> the fence with this, even the existing change from the previous more
>> verbose code. I know this is shorter, but it is always more readable to
>> use the longer version with 'test'. I find it hard to remember the
>> specifics.
>
> You'd never remember unless you practice, but it boils down to one
> question: is it reasonable to expect that most developers who need
> to touch this code find it worth to learn to read and write shell
> scripts well in this day and age?  The answer is probably no.
>
> As you may remember, this R=${A-${B?}} dance started at
>
>   https://lore.kernel.org/git/xmqqwmlpb8er.fsf@gitster.g/
>
> where I said:
>
>     ...
>     in a separate "after the dust settles" clean-up #leftoverbits topic.
>
>     We could replace the first 7 lines with a single-liner
>
>        R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA?}}
>
>     if we wanted to, but all of that will be mere clean-up changes.
>
> Even the longhand to set a single R with if/elif cascade so that we
> can have a single location that invokes ci/run-style-check.sh was
> considered extra clean-up for #leftoverbits at least by me.
>
> But after seeing you used the ${A-${B?}} dance, which is more
> advanced than the #leftoverbits clean-up, I thought you were
> interested in using such a construct that pursues parameter
> expansion mastery, and that was the primary reason why the
> demonstration in the message you are responding to was added.
>
> I personally do not care too deeply which one to use wrt the
> readability, but
>
> 	R=${A-${B?}}
> 	if test -z "$R"
> 	then
> 		error
> 	fi
>
> looks strange and inconsistent by spreading the error check to two
> places.  The code would be better off if it were
>
> 	R=${A-$B}
> 	if test -z "$R"
> 	then
> 		error
> 	fi
>
> (or with R=${A:-$B}) instead.  Then it makes it clear that the
> author wanted to take care of the error case with the if part.

Yeah fair enough, we are deep into it and might and makes sense to make
it more consistent. I'll incorporate your suggestion and send in a new
version.

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

[PATCH v6 0/6] clang-format: add more rules and enable on CI

[Karthik Nayak]

This is v6 of my series to add clang-format to the CI.

The series was mostly inspired by a patch sent recently to 'include
kh_foreach* macros in ForEachMacros' [1]. I was wondering why we don't
run the formatting on CI and reduce some of the workload of reviewers.

We have a '.clang-format' file to provide rules for code formatting.
The commits 1-3 aims to add more rules to the file.

Commit 4 enables CI action on GitHub and GitLab to also check for the
code formatting. Currently, it is allowed to fail. This way we can build
confidence and fine tune the values as needed and finally enforce the
check in a future patch. I'm not well versed with GitHub workflows, and
I mostly tried to copy existing work there. Expecting some feedback in
that section!

Commit 5 fixes an existing issue with the 'check-whitespace' job, which
is failing as success in the GitLab CI.

Commit 6 adds the `RemoveBracesLLVM` only in the context of the CI. If we
decide against it, we could drop this commit from the series.

1: https://lore.kernel.org/git/4e7893f5-2dd9-46cf-8a64-cf780f4e1730@web.de/

Changes from v5:
- Used 'C=${A-${B:?}}' instead of 'C=${A-${B?}}' for obtaining the base OID
in GitLab CI script. The difference being that the former also checks
for B being empty. With this we can simplify the script from

    R=${A-${B?}}
    if test -z "$R"
    then
        error
    fi

to 
    R=${A-${B:?}} || exit


* CI without issues
  - GitLab: https://gitlab.com/gitlab-org/git/-/jobs/7394162598
  - GitHub: https://github.com/KarthikNayak/git/actions/runs/10037207688/job/27736512636?pr=1
* CI with issues detected
  - GitLab: https://gitlab.com/gitlab-org/git/-/jobs/7394406605
  - GitHub: https://github.com/KarthikNayak/git/actions/runs/10037229508/job/27736859486

Karthik Nayak (6):
  clang-format: indent preprocessor directives after hash
  clang-format: avoid spacing around bitfield colon
  clang-format: formalize some of the spacing rules
  ci: run style check on GitHub and GitLab
  check-whitespace: detect if no base_commit is provided
  ci/style-check: add `RemoveBracesLLVM` in CI job

 .clang-format                     | 25 +++++++++++++++++++++++
 .github/workflows/check-style.yml | 34 +++++++++++++++++++++++++++++++
 .gitlab-ci.yml                    | 25 ++++++++++++++++++++++-
 ci/check-whitespace.sh            | 10 +++++++--
 ci/install-dependencies.sh        |  4 ++++
 ci/run-style-check.sh             | 25 +++++++++++++++++++++++
 6 files changed, 120 insertions(+), 3 deletions(-)
 create mode 100644 .github/workflows/check-style.yml
 create mode 100755 ci/run-style-check.sh

Range-diff against v5:
1:  6cf91ffc86 = 1:  6cf91ffc86 clang-format: indent preprocessor directives after hash
2:  beb002885f = 2:  beb002885f clang-format: avoid spacing around bitfield colon
3:  3922529001 = 3:  3922529001 clang-format: formalize some of the spacing rules
4:  ae23eb5af8 ! 4:  14718625ad ci: run style check on GitHub and GitLab
    @@ .gitlab-ci.yml: check-whitespace:
     +  # be defined in all pipelines.
     +  script:
     +    - |
    -+      R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA?}}
    -+
    -+      if test -z "$R"
    -+      then
    -+        echo "CI_MERGE_REQUEST_DIFF_BASE_SHA should always exist!"
    -+        exit 1
    -+      fi
    ++      R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA:?}} || exit
     +      ./ci/run-style-check.sh "$R"
     +  rules:
     +    - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
5:  a38cde03a8 ! 5:  fb8c30852b check-whitespace: detect if no base_commit is provided
    @@ .gitlab-ci.yml: check-whitespace:
        script:
     -    - ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
     +    - |
    -+      R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA?}}
    -+
    -+      if test -z "$R"
    -+      then
    -+        echo "CI_MERGE_REQUEST_DIFF_BASE_SHA should always exist!"
    -+        exit 1
    -+      fi
    ++      R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA:?}} || exit
     +      ./ci/check-whitespace.sh "$R"
        rules:
          - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
6:  008e77bd0a = 6:  08c5bb777d ci/style-check: add `RemoveBracesLLVM` in CI job
-- 
2.45.2

[PATCH v6 1/6] clang-format: indent preprocessor directives after hash

[Karthik Nayak]

We do not have a rule around the indentation of preprocessor directives.
This was also discussed on the list [1], noting how there is often
inconsistency in the styling. While there was discussion, there was no
conclusion around what is the preferred style here. One style being
indenting after the hash:

    #if FOO
    #  if BAR
    #    include <foo>
    #  endif
    #endif

The other being before the hash:

    #if FOO
      #if BAR
        #include <foo>
      #endif
    #endif

Let's pick the former and add 'IndentPPDirectives: AfterHash' value to
our '.clang-format'. There is no clear reason to pick one over the
other, but it would definitely be nicer to be consistent.

[1]: https://lore.kernel.org/r/xmqqwmmm1bw6.fsf@gitster.g

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .clang-format | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/.clang-format b/.clang-format
index 3ed4fac753..5e128519bf 100644
--- a/.clang-format
+++ b/.clang-format
@@ -96,6 +96,12 @@ BreakStringLiterals: false
 # Switch statement body is always indented one level more than case labels.
 IndentCaseLabels: false
 
+# Indents directives before the hash.
+# #if FOO
+# #  include <foo>
+# #endif
+IndentPPDirectives: AfterHash
+
 # Don't indent a function definition or declaration if it is wrapped after the
 # type
 IndentWrappedFunctionNames: false
-- 
2.45.2

[PATCH v6 2/6] clang-format: avoid spacing around bitfield colon

[Karthik Nayak]

The spacing around colons is currently not standardized and as such we
have the following practices in our code base:
- Spacing around the colon `int bf : 1`: 146 instances
- No spacing around the colon `int bf:1`: 148 instances
- Spacing before the colon `int bf :1`: 6 instances
- Spacing after the colon `int bf: 1`: 12 instances

Let's formalize this by picking the most followed pattern and add the
corresponding style to '.clang-format'.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .clang-format | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/.clang-format b/.clang-format
index 5e128519bf..803b274dd5 100644
--- a/.clang-format
+++ b/.clang-format
@@ -72,6 +72,10 @@ AlwaysBreakAfterReturnType: None
 BinPackArguments: true
 BinPackParameters: true
 
+# Add no space around the bit field
+# unsigned bf:2;
+BitFieldColonSpacing: None
+
 # Attach braces to surrounding context except break before braces on function
 # definitions.
 # void foo()
-- 
2.45.2

[PATCH v6 3/6] clang-format: formalize some of the spacing rules

[Karthik Nayak]

There are some spacing rules that we follow in the project and it makes
sense to formalize them:
* Ensure there is no space inserted after the logical not '!' operator.
* Ensure there is no space before the case statement's colon.
* Ensure there is no space before the first bracket '[' of an array.
* Ensure there is no space in empty blocks.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .clang-format | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/.clang-format b/.clang-format
index 803b274dd5..3c8018a1dd 100644
--- a/.clang-format
+++ b/.clang-format
@@ -118,11 +118,18 @@ PointerAlignment: Right
 # x = (int32)y;    not    x = (int32) y;
 SpaceAfterCStyleCast: false
 
+# No space is inserted after the logical not operator
+SpaceAfterLogicalNot: false
+
 # Insert spaces before and after assignment operators
 # int a = 5;    not    int a=5;
 # a += 42;             a+=42;
 SpaceBeforeAssignmentOperators: true
 
+# Spaces will be removed before case colon.
+# case 1: break;    not     case 1 : break;
+SpaceBeforeCaseColon: false
+
 # Put a space before opening parentheses only after control statement keywords.
 # void f() {
 #   if (true) {
@@ -134,6 +141,14 @@ SpaceBeforeParens: ControlStatements
 # Don't insert spaces inside empty '()'
 SpaceInEmptyParentheses: false
 
+# No space before first '[' in arrays
+# int a[5][5];     not      int a [5][5];
+SpaceBeforeSquareBrackets: false
+
+# No space will be inserted into {}
+# while (true) {}    not    while (true) { }
+SpaceInEmptyBlock: false
+
 # The number of spaces before trailing line comments (// - comments).
 # This does not affect trailing block comments (/* - comments).
 SpacesBeforeTrailingComments: 1
-- 
2.45.2

[PATCH v6 4/6] ci: run style check on GitHub and GitLab

[Karthik Nayak]

We don't run style checks on our CI, even though we have a
'.clang-format' setup in the repository. Let's add one, the job will
validate only against the new commits added and will only run on merge
requests. Since we're introducing it for the first time, let's allow
this job to fail, so we can validate if this is useful and eventually
enforce it.

For GitHub, we allow the job to pass by adding 'continue-on-error: true'
to the workflow. This means the job would show as passed, even if the
style check failed. To know the status of the job, users have to
manually check the logs.

For GitLab, we allow the job to pass by adding 'allow_failure: true', to
the job. Unlike GitHub, here the job will show as failed with a yellow
warning symbol, but the pipeline would still show as passed.

Also for GitLab, we use the 'CI_MERGE_REQUEST_TARGET_BRANCH_SHA'
variable by default to obtain the base SHA of the merged pipeline (which
is only available for merged pipelines [1]). Otherwise we use the
'CI_MERGE_REQUEST_DIFF_BASE_SHA' variable.

[1]: https://docs.gitlab.com/ee/ci/variables/predefined_variables.html#predefined-variables-for-merge-request-pipelines

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .github/workflows/check-style.yml | 34 +++++++++++++++++++++++++++++++
 .gitlab-ci.yml                    | 18 ++++++++++++++++
 ci/install-dependencies.sh        |  4 ++++
 ci/run-style-check.sh             |  8 ++++++++
 4 files changed, 64 insertions(+)
 create mode 100644 .github/workflows/check-style.yml
 create mode 100755 ci/run-style-check.sh

diff --git a/.github/workflows/check-style.yml b/.github/workflows/check-style.yml
new file mode 100644
index 0000000000..c052a5df23
--- /dev/null
+++ b/.github/workflows/check-style.yml
@@ -0,0 +1,34 @@
+name: check-style
+
+# Get the repository with all commits to ensure that we can analyze
+# all of the commits contributed via the Pull Request.
+
+on:
+  pull_request:
+    types: [opened, synchronize]
+
+# Avoid unnecessary builds. Unlike the main CI jobs, these are not
+# ci-configurable (but could be).
+concurrency:
+  group: ${{ github.workflow }}-${{ github.ref }}
+  cancel-in-progress: true
+
+jobs:
+  check-style:
+    env:
+      CC: clang
+      jobname: ClangFormat
+    runs-on: ubuntu-latest
+    steps:
+    - uses: actions/checkout@v4
+      with:
+        fetch-depth: 0
+
+    - run: ci/install-dependencies.sh
+
+    - name: git clang-format
+      continue-on-error: true
+      id: check_out
+      run: |
+        ./ci/run-style-check.sh \
+          "${{github.event.pull_request.base.sha}}"
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 37b991e080..2886f6e182 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -123,6 +123,24 @@ check-whitespace:
   rules:
     - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
 
+check-style:
+  image: ubuntu:latest
+  allow_failure: true
+  variables:
+    CC: clang
+    jobname: ClangFormat
+  before_script:
+    - ./ci/install-dependencies.sh
+  # Since $CI_MERGE_REQUEST_TARGET_BRANCH_SHA is only defined for merged
+  # pipelines, we fallback to $CI_MERGE_REQUEST_DIFF_BASE_SHA, which should
+  # be defined in all pipelines.
+  script:
+    - |
+      R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA:?}} || exit
+      ./ci/run-style-check.sh "$R"
+  rules:
+    - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
+
 documentation:
   image: ubuntu:latest
   variables:
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 6ec0f85972..fb34ced8f0 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -87,6 +87,10 @@ macos-*)
 esac
 
 case "$jobname" in
+ClangFormat)
+	sudo apt-get -q update
+	sudo apt-get -q -y install clang-format
+	;;
 StaticAnalysis)
 	sudo apt-get -q update
 	sudo apt-get -q -y install coccinelle libcurl4-openssl-dev libssl-dev \
diff --git a/ci/run-style-check.sh b/ci/run-style-check.sh
new file mode 100755
index 0000000000..76dd37d22b
--- /dev/null
+++ b/ci/run-style-check.sh
@@ -0,0 +1,8 @@
+#!/bin/sh
+#
+# Perform style check
+#
+
+baseCommit=$1
+
+git clang-format --style file --diff --extensions c,h "$baseCommit"
-- 
2.45.2

[PATCH v6 5/6] check-whitespace: detect if no base_commit is provided

[Karthik Nayak]

The 'check-whitespace' CI script exits gracefully if no base commit is
provided or if an invalid revision is provided. This is not good because
if a particular CI provides an incorrect base_commit, it would fail
successfully.

This is exactly the case with the GitLab CI. The CI is using the
"$CI_MERGE_REQUEST_TARGET_BRANCH_SHA" variable to get the base commit
SHA, but variable is only defined for _merged_ pipelines. So it is empty
for regular pipelines [1]. This should've failed the check-whitespace
job.

Let's fallback to 'CI_MERGE_REQUEST_DIFF_BASE_SHA' if
"CI_MERGE_REQUEST_TARGET_BRANCH_SHA" isn't available in GitLab CI,
similar to the previous commit. Let's also add a check for incorrect
base_commit in the 'check-whitespace.sh' script. While here, fix a small
typo too.

[1]: https://docs.gitlab.com/ee/ci/variables/predefined_variables.html#predefined-variables-for-merge-request-pipelines

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .gitlab-ci.yml         |  7 ++++++-
 ci/check-whitespace.sh | 10 ++++++++--
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 2886f6e182..2589098eff 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -118,8 +118,13 @@ check-whitespace:
   image: ubuntu:latest
   before_script:
     - ./ci/install-dependencies.sh
+  # Since $CI_MERGE_REQUEST_TARGET_BRANCH_SHA is only defined for merged
+  # pipelines, we fallback to $CI_MERGE_REQUEST_DIFF_BASE_SHA, which should
+  # be defined in all pipelines.
   script:
-    - ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
+    - |
+      R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA:?}} || exit
+      ./ci/check-whitespace.sh "$R"
   rules:
     - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
 
diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh
index db399097a5..c40804394c 100755
--- a/ci/check-whitespace.sh
+++ b/ci/check-whitespace.sh
@@ -9,7 +9,7 @@ baseCommit=$1
 outputFile=$2
 url=$3
 
-if test "$#" -ne 1 && test "$#" -ne 3
+if test "$#" -ne 1 && test "$#" -ne 3 || test -z "$1"
 then
 	echo "USAGE: $0 <BASE_COMMIT> [<OUTPUT_FILE> <URL>]"
 	exit 1
@@ -21,6 +21,12 @@ commitText=
 commitTextmd=
 goodParent=
 
+if ! git rev-parse --quiet --verify "${baseCommit}"
+then
+    echo "Invalid <BASE_COMMIT> '${baseCommit}'"
+    exit 1
+fi
+
 while read dash sha etc
 do
 	case "${dash}" in
@@ -67,7 +73,7 @@ then
 		goodParent=${baseCommit: 0:7}
 	fi
 
-	echo "A whitespace issue was found in onen of more of the commits."
+	echo "A whitespace issue was found in one or more of the commits."
 	echo "Run the following command to resolve whitespace issues:"
 	echo "git rebase --whitespace=fix ${goodParent}"
 
-- 
2.45.2

[PATCH v6 6/6] ci/style-check: add `RemoveBracesLLVM` in CI job

[Karthik Nayak]

For 'clang-format', setting 'RemoveBracesLLVM' to 'true', adds a check
to ensure we avoid curly braces for single-statement bodies in
conditional blocks.

However, the option does come with two warnings [1]:

    This option will be renamed and expanded to support other styles.

and

    Setting this option to true could lead to incorrect code formatting
    due to clang-format’s lack of complete semantic information. As
    such, extra care should be taken to review code changes made by
    this option.

The latter seems to be of concern. While we want to experiment with the
rule, adding it to the in-tree '.clang-format' could affect end-users.
Let's only add it to the CI jobs for now. With time, we can evaluate
its efficacy and decide if we want to add it to '.clang-format' or
retract it entirely. We do so, by adding the existing rules in
'.clang-format' and this rule to a temp file outside the working tree,
which is then used by 'git clang-format'. This ensures we don't murk
with files in-tree.

[1]: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#removebracesllvm

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 ci/run-style-check.sh | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/ci/run-style-check.sh b/ci/run-style-check.sh
index 76dd37d22b..6cd4b1d934 100755
--- a/ci/run-style-check.sh
+++ b/ci/run-style-check.sh
@@ -5,4 +5,21 @@
 
 baseCommit=$1
 
-git clang-format --style file --diff --extensions c,h "$baseCommit"
+# Remove optional braces of control statements (if, else, for, and while)
+# according to the LLVM coding style. This avoids braces on simple
+# single-statement bodies of statements but keeps braces if one side of
+# if/else if/.../else cascade has multi-statement body.
+#
+# As this rule comes with a warning [1], we want to experiment with it
+# before adding it in-tree. since the CI job for the style check is allowed
+# to fail, appending the rule here allows us to validate its efficacy.
+# While also ensuring that end-users are not affected directly.
+#
+# [1]: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#removebracesllvm
+{
+	cat .clang-format
+	echo "RemoveBracesLLVM: true"
+} >/tmp/clang-format-rules
+
+git clang-format --style=file:/tmp/clang-format-rules \
+	--diff --extensions c,h "$baseCommit"
-- 
2.45.2

Re: [PATCH v6 1/6] clang-format: indent preprocessor directives after hash

[Patrick Steinhardt]

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

On Tue, Jul 23, 2024 at 10:21:06AM +0200, Karthik Nayak wrote:
> We do not have a rule around the indentation of preprocessor directives.
> This was also discussed on the list [1], noting how there is often
> inconsistency in the styling. While there was discussion, there was no
> conclusion around what is the preferred style here. One style being
> indenting after the hash:
> 
>     #if FOO
>     #  if BAR
>     #    include <foo>
>     #  endif
>     #endif
> 
> The other being before the hash:
> 
>     #if FOO
>       #if BAR
>         #include <foo>
>       #endif
>     #endif
> 
> Let's pick the former and add 'IndentPPDirectives: AfterHash' value to
> our '.clang-format'. There is no clear reason to pick one over the
> other, but it would definitely be nicer to be consistent.
> 
> [1]: https://lore.kernel.org/r/xmqqwmmm1bw6.fsf@gitster.g
> 
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>

This doesn't necessarily have to be part of this patch series, but I
think that we should document this as part of our CodingGuidelines, as
well. I planned to send an update to the coding guidelines soon anyway,
so I can handle that once this topic lands.

Patrick

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

Re: [PATCH v6 1/6] clang-format: indent preprocessor directives after hash

[Karthik Nayak]

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

Patrick Steinhardt <ps@pks.im> writes:

> On Tue, Jul 23, 2024 at 10:21:06AM +0200, Karthik Nayak wrote:
>> We do not have a rule around the indentation of preprocessor directives.
>> This was also discussed on the list [1], noting how there is often
>> inconsistency in the styling. While there was discussion, there was no
>> conclusion around what is the preferred style here. One style being
>> indenting after the hash:
>>
>>     #if FOO
>>     #  if BAR
>>     #    include <foo>
>>     #  endif
>>     #endif
>>
>> The other being before the hash:
>>
>>     #if FOO
>>       #if BAR
>>         #include <foo>
>>       #endif
>>     #endif
>>
>> Let's pick the former and add 'IndentPPDirectives: AfterHash' value to
>> our '.clang-format'. There is no clear reason to pick one over the
>> other, but it would definitely be nicer to be consistent.
>>
>> [1]: https://lore.kernel.org/r/xmqqwmmm1bw6.fsf@gitster.g
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>
> This doesn't necessarily have to be part of this patch series, but I
> think that we should document this as part of our CodingGuidelines, as
> well. I planned to send an update to the coding guidelines soon anyway,
> so I can handle that once this topic lands.
>
> Patrick

Yeah, that would be great indeed. I think this series is now merged to
next.

Thank Patrick

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

Re: [PATCH v6 6/6] ci/style-check: add `RemoveBracesLLVM` in CI job

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> For 'clang-format', setting 'RemoveBracesLLVM' to 'true', adds a check
> to ensure we avoid curly braces for single-statement bodies in
> conditional blocks.
>
> However, the option does come with two warnings [1]:
>
>     This option will be renamed and expanded to support other styles.
>
> and
>
>     Setting this option to true could lead to incorrect code formatting
>     due to clang-format’s lack of complete semantic information. As
>     such, extra care should be taken to review code changes made by
>     this option.
>
> The latter seems to be of concern. While we want to experiment with the
> rule, adding it to the in-tree '.clang-format' could affect end-users.
> Let's only add it to the CI jobs for now. With time, we can evaluate
> its efficacy and decide if we want to add it to '.clang-format' or
> retract it entirely. We do so, by adding the existing rules in
> '.clang-format' and this rule to a temp file outside the working tree,
> which is then used by 'git clang-format'. This ensures we don't murk
> with files in-tree.
>
> [1]: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#removebracesllvm
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  ci/run-style-check.sh | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)

Have we done enough experiment by now and don't we want to move it
to the set of rules in our tree?

How often do people use "git clang-format" before they submit their
series these days, by the way?

> diff --git a/ci/run-style-check.sh b/ci/run-style-check.sh
> index 76dd37d22b..6cd4b1d934 100755
> --- a/ci/run-style-check.sh
> +++ b/ci/run-style-check.sh
> @@ -5,4 +5,21 @@
>  
>  baseCommit=$1
>  
> -git clang-format --style file --diff --extensions c,h "$baseCommit"
> +# Remove optional braces of control statements (if, else, for, and while)
> +# according to the LLVM coding style. This avoids braces on simple
> +# single-statement bodies of statements but keeps braces if one side of
> +# if/else if/.../else cascade has multi-statement body.
> +#
> +# As this rule comes with a warning [1], we want to experiment with it
> +# before adding it in-tree. since the CI job for the style check is allowed
> +# to fail, appending the rule here allows us to validate its efficacy.
> +# While also ensuring that end-users are not affected directly.
> +#
> +# [1]: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#removebracesllvm
> +{
> +	cat .clang-format
> +	echo "RemoveBracesLLVM: true"
> +} >/tmp/clang-format-rules
> +
> +git clang-format --style=file:/tmp/clang-format-rules \
> +	--diff --extensions c,h "$baseCommit"

Re: [PATCH v6 6/6] ci/style-check: add `RemoveBracesLLVM` in CI job

[Karthik Nayak]

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

Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> For 'clang-format', setting 'RemoveBracesLLVM' to 'true', adds a check
>> to ensure we avoid curly braces for single-statement bodies in
>> conditional blocks.
>>
>> However, the option does come with two warnings [1]:
>>
>>     This option will be renamed and expanded to support other styles.
>>
>> and
>>
>>     Setting this option to true could lead to incorrect code formatting
>>     due to clang-format’s lack of complete semantic information. As
>>     such, extra care should be taken to review code changes made by
>>     this option.
>>
>> The latter seems to be of concern. While we want to experiment with the
>> rule, adding it to the in-tree '.clang-format' could affect end-users.
>> Let's only add it to the CI jobs for now. With time, we can evaluate
>> its efficacy and decide if we want to add it to '.clang-format' or
>> retract it entirely. We do so, by adding the existing rules in
>> '.clang-format' and this rule to a temp file outside the working tree,
>> which is then used by 'git clang-format'. This ensures we don't murk
>> with files in-tree.
>>
>> [1]: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#removebracesllvm
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>>  ci/run-style-check.sh | 19 ++++++++++++++++++-
>>  1 file changed, 18 insertions(+), 1 deletion(-)
>
> Have we done enough experiment by now and don't we want to move it
> to the set of rules in our tree?
>

Christian and I were talking about clang-format today. So great to see
that you too were thinking about it. I think this particular rule should
be okay to move our tree. For this particular rule, I found one spot it
fails:

  $ ./ci/run-style-check.sh @~1
  diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
  index 1809539201..341d9eac0d 100644
  --- a/builtin/receive-pack.c
  +++ b/builtin/receive-pack.c
  @@ -1929,12 +1929,11 @@ static void
execute_commands_non_atomic(struct command *commands,
   			goto cleanup;

   	failure:
  -		for (cmd = commands; cmd; cmd = cmd->next) {
  +		for (cmd = commands; cmd; cmd = cmd->next)
   			if (reported_error)
   				cmd->error_string = reported_error;
   			else if (strmap_contains(&failed_refs, cmd->ref_name))
   				cmd->error_string = strmap_get(&failed_refs, cmd->ref_name);
  -		}

   	cleanup:
   		ref_transaction_free(transaction);

We generally add brackets in such scenarios, but the style-check
suggests to use them instead.

> How often do people use "git clang-format" before they submit their
> series these days, by the way?
>

To be honest, I don't think anyone does. As far as I've seen there is
too many false positives currently.

>> diff --git a/ci/run-style-check.sh b/ci/run-style-check.sh
>> index 76dd37d22b..6cd4b1d934 100755
>> --- a/ci/run-style-check.sh
>> +++ b/ci/run-style-check.sh
>> @@ -5,4 +5,21 @@
>>
>>  baseCommit=$1
>>
>> -git clang-format --style file --diff --extensions c,h "$baseCommit"
>> +# Remove optional braces of control statements (if, else, for, and while)
>> +# according to the LLVM coding style. This avoids braces on simple
>> +# single-statement bodies of statements but keeps braces if one side of
>> +# if/else if/.../else cascade has multi-statement body.
>> +#
>> +# As this rule comes with a warning [1], we want to experiment with it
>> +# before adding it in-tree. since the CI job for the style check is allowed
>> +# to fail, appending the rule here allows us to validate its efficacy.
>> +# While also ensuring that end-users are not affected directly.
>> +#
>> +# [1]: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#removebracesllvm
>> +{
>> +	cat .clang-format
>> +	echo "RemoveBracesLLVM: true"
>> +} >/tmp/clang-format-rules
>> +
>> +git clang-format --style=file:/tmp/clang-format-rules \
>> +	--diff --extensions c,h "$baseCommit"

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

Re: [PATCH v6 6/6] ci/style-check: add `RemoveBracesLLVM` in CI job

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

>    	failure:
>   -		for (cmd = commands; cmd; cmd = cmd->next) {
>   +		for (cmd = commands; cmd; cmd = cmd->next)
>    			if (reported_error)
>    				cmd->error_string = reported_error;
>    			else if (strmap_contains(&failed_refs, cmd->ref_name))
>    				cmd->error_string = strmap_get(&failed_refs, cmd->ref_name);
>   -		}
>
>    	cleanup:
>    		ref_transaction_free(transaction);
>
> We generally add brackets in such scenarios, but the style-check
> suggests to use them instead.

The above falls into "once the code is written" category I mentioned
in the other thread.  I do not mind the original, and I do not think
the suggested change makes it significantly worse, but there is no
significant improvement, either.  If the original is in the public
tree, it is not worth the patch churn to update it to match the
suggestion, but if the original is something you are yet to send it
out to the public consumption, it would be a good idea to take the
suggestion before sending it out.