Justin Tobler <jltobler@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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
