patch, 8 partsclang-format: add more rules and enable on 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/

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

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(+)

On Mon, Jul 8, 2024 at 2:23 AM Karthik Nayak <karthik.188@gmail.com> wrote:

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

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

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/

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

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.

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

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(-)

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(+)

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(+)

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

Makes sense.

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(-)

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

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.

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(+)

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

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

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

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

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.

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.

Makes sense, will add.

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

Hi Karthik

On 08/07/2024 21:25, Karthik Nayak wrote:

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?

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

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

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]

On 13/07/2024 13:30, Karthik Nayak wrote:

 >

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

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

phillip.wood123@gmail.com writes:

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

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

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(+)

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

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.

"color" -> "colon".

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.

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

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.

Will fix, thanks!

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]

On 24/07/08 11:23AM, Karthik Nayak wrote:

s/color/colon

Justin Tobler <jltobler@gmail.com> writes:

Thanks, will fix in the next version!

On Mon, Jul 8, 2024 at 5:24 AM Karthik Nayak <karthik.188@gmail.com> wrote:

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

Eric Sunshine <sunshine@sunshineco.com> writes:

Will fix in v2.

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

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

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.

Very good idea.

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).

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.

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

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!

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

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(-)

On 24/07/08 03:52PM, Junio C Hamano wrote:

s/envionment/environment

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

On 2024-07-08 at 22:52:11, Junio C Hamano wrote:

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.

On 24/07/08 11:23AM, Karthik Nayak wrote:

[snip]

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.

Justin Tobler <jltobler@gmail.com> writes:

I'm not sure I follow.

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?

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.

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

On 24/07/08 02:16PM, Karthik Nayak wrote:

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

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

Justin Tobler <jltobler@gmail.com> writes:

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

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.

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

On 24/07/09 01:44AM, Karthik Nayak wrote:

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?

Correct, this is my understanding.

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.

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

Justin Tobler <jltobler@gmail.com> writes:

[snip]

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.

Not that I remember.

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

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(-)

On Mon, Jul 8, 2024 at 2:24 AM Karthik Nayak <karthik.188@gmail.com> wrote:

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

On 7/8/24 12:18 PM, Chris Torek wrote:

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".

On Mon, Jul 8, 2024 at 3:35 AM Georg Pfuetzenreuter <georg@syscid.com> wrote:

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

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

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

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

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.

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

"exists" -> "exits"

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 ||.

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?

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?

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?

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

Will fix.

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.

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

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

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.

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

It would make the intent a lot clearer.  Good.

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?

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

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.

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

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.

On 24/07/08 11:23AM, Karthik Nayak wrote:

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.

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

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

There is another preexisting typo:

s/one of/one or/

Justin Tobler <jltobler@gmail.com> writes:

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]

It is required, as commented above

Thanks. will add

On Mon, Jul 8, 2024 at 2:23 AM Karthik Nayak <karthik.188@gmail.com> wrote:

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

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

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(+)

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(+)

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(+)

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(-)

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(+)

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

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.

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

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.

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 :)

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

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.

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

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

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

Best Wishes

Phillip

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(+)

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(-)

On 24/07/11 10:30AM, Karthik Nayak wrote:

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.

[snip]

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

-Justin

Justin Tobler <jltobler@gmail.com> writes:

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?

Thanks for a review.  Very much appreciated.

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

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

Thanks both of you!

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

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.

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

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

Aside from a few comments, the series looked good.

Will queue.  Thanks.

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'

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(+)

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(+)

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(+)

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(-)

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(+)

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

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

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.

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(-)

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(+)

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

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.

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

Will change.

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.

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.

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

On 13/07/2024 17:46, Karthik Nayak wrote:

[snip]

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

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

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.

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

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?

Junio C Hamano <gitster@pobox.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?

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!

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

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).

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

Yes. I was referring to a tracked file.

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.

Yes this is the best solution.

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

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.

Junio C Hamano <gitster@pobox.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.

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

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

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

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"

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(+)

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(+)

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(+)

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(-)

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(+)

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

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(-)

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(+)

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

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.

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

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

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"

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(+)

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(+)

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(+)

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

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

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.

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

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

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(-)

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

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.

Junio C Hamano <gitster@pobox.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. 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

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

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.

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

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.

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(-)

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

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.

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

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(+)

On Tue, Jul 23, 2024 at 10:21:06AM +0200, Karthik Nayak wrote:

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

Patrick Steinhardt <ps@pks.im> writes:

Yeah, that would be great indeed. I think this series is now merged to next.

Thank Patrick

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(+)

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(+)

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

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(-)

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(-)

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

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?

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

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.

To be honest, I don't think anyone does. As far as I've seen there is too many false positives currently.

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

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.