Re: [PATCH] ci: fix base commit fallback for check-whitespace and check-style

From: Toon Claes <toon@iotcl.com>
Date: Feb 4, 2025, 16:17 UTC
Message-ID: <87ldul4rcb.fsf@iotcl.com>
In-Reply-To: <20250131173938.3592899-1-jltobler@gmail.com>

Justin Tobler <jltobler@gmail.com> writes:

Show 17 quoted lines

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

Show 35 quoted lines

> 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