From: Toon Claes <toon@iotcl•com>
To: Justin Tobler <jltobler@gmail•com>, git@vger•kernel.org
Cc: Justin Tobler <jltobler@gmail•com>
Subject: Re: [PATCH] ci: fix base commit fallback for check-whitespace and check-style
Date: Tue, 04 Feb 2025 17:17:24 +0100 [thread overview]
Message-ID: <87ldul4rcb.fsf@iotcl.com> (raw)
In-Reply-To: <20250131173938.3592899-1-jltobler@gmail.com>
Justin Tobler <jltobler@gmail•com> writes:
> The check-whitespace and check-style CI scripts require a base commit.
> In GitLab CI, the base commit can be provided by several different
> predefined CI variables depending on the type of pipeline being
> performed.
>
> In 30c4f7e350 (check-whitespace: detect if no base_commit is provided,
> 2024-07-23), the GitLab check-whitespace CI job was modified to support
> CI_MERGE_REQUEST_DIFF_BASE_SHA as a fallback base commit if
> CI_MERGE_REQUEST_TARGET_BRANCH_SHA was not provided. The same fallback
> strategy was also implemented for the GitLab check-style CI job in
> bce7e52d4e (ci: run style check on GitHub and GitLab, 2024-07-23).
>
> The base commit fallback is implemented using shell parameter expansion
> where, if the first variable is unset, the second variable is used as
> fallback. In GitLab CI, these variables can be set but null. This has
> the unintended effect of selecting an empty first variable which results
> in CI jobs providing an invalid base commit and failing.
I didn't know using this expansion was possible without a colon, but as
you pointed out (off-list), it is. From [1]:
Omitting the colon results in a test only for a parameter that is
unset. Put another way, if the colon is included, the operator tests
for both parameter’s existence and that its value is not null; if
the colon is omitted, the operator tests only for existence.
> Fix the issue by defaulting to the fallback variable if the first is
> unset or null.
Yeah, makes sense to do this.
> Signed-off-by: Justin Tobler <jltobler@gmail•com>
> ---
> Successful GitLab CI check-whitespace and check-style runs:
> - https://gitlab.com/gitlab-org/git/-/jobs/9011117606
> - https://gitlab.com/gitlab-org/git/-/jobs/9011117607
> ---
> .gitlab-ci.yml | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index 9254e01583..273a8bad39 100644
> --- a/.gitlab-ci.yml
> +++ b/.gitlab-ci.yml
> @@ -183,7 +183,7 @@ check-whitespace:
> # be defined in all pipelines.
> script:
> - |
> - R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA:?}} || exit
> + 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'
> @@ -203,7 +203,7 @@ check-style:
> # be defined in all pipelines.
> script:
> - |
> - R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA:?}} || exit
> + 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'
>
> base-commit: 3b0d05c4a79d0e441283680a864529b02dca5f08
> --
> 2.48.1.157.g3b0d05c4a7
Simple fix, but thank you so much for figuring out and submitting this
patch. I approve!
[1]: https://www.gnu.org/software/bash/manual/html_node/Shell-Parameter-Expansion.html
--
Toon
prev parent reply other threads:[~2025-02-04 16:17 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-01-31 17:39 [PATCH] ci: fix base commit fallback for check-whitespace and check-style Justin Tobler
2025-02-04 16:17 ` Toon Claes [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87ldul4rcb.fsf@iotcl.com \
--to=toon@iotcl$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=jltobler@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox