{"thread":{"id":"62879","subject":"[PATCH] ci: fix base commit fallback for check-whitespace and check-style","startedAt":"2025-01-31T17:43:54Z","lastAt":"2025-02-04T16:17:48Z","messageCount":2,"participants":["Justin Tobler","Toon Claes"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"511580","messageId":"20250131173938.3592899-1-jltobler@gmail.com","threadId":"62879","inReplyTo":null,"subject":"[PATCH] ci: fix base commit fallback for check-whitespace and check-style","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-01-31T17:39:38Z","receivedAt":"2025-01-31T17:43:54Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"The check-whitespace and check-style CI scripts require a base commit.\nIn GitLab CI, the base commit can be provided by several different\npredefined CI variables depending on the type of pipeline being\nperformed.\n\nIn 30c4f7e350 (check-whitespace: detect if no base_commit is provided,\n2024-07-23), the GitLab check-whitespace CI job was modified to support\nCI_MERGE_REQUEST_DIFF_BASE_SHA as a fallback base commit if\nCI_MERGE_REQUEST_TARGET_BRANCH_SHA was not provided. The same fallback\nstrategy was also implemented for the GitLab check-style CI job in\nbce7e52d4e (ci: run style check on GitHub and GitLab, 2024-07-23).\n\nThe base commit fallback is implemented using shell parameter expansion\nwhere, if the first variable is unset, the second variable is used as\nfallback. In GitLab CI, these variables can be set but null. This has\nthe unintended effect of selecting an empty first variable which results\nin CI jobs providing an invalid base commit and failing.\n\nFix the issue by defaulting to the fallback variable if the first is\nunset or null.\n\nSigned-off-by: Justin Tobler \n---\nSuccessful GitLab CI check-whitespace and check-style runs:\n - https://gitlab.com/gitlab-org/git/-/jobs/9011117606\n - https://gitlab.com/gitlab-org/git/-/jobs/9011117607\n---\n .gitlab-ci.yml | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/.gitlab-ci.yml b/.gitlab-ci.yml\nindex 9254e01583..273a8bad39 100644\n--- a/.gitlab-ci.yml\n+++ b/.gitlab-ci.yml\n@@ -183,7 +183,7 @@ check-whitespace:\n # be defined in all pipelines.\n script:\n - |\n- R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA:?}} || exit\n+ R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA:-${CI_MERGE_REQUEST_DIFF_BASE_SHA:?}} || exit\n ./ci/check-whitespace.sh \"$R\"\n rules:\n - if: $CI_PIPELINE_SOURCE == 'merge_request_event'\n@@ -203,7 +203,7 @@ check-style:\n # be defined in all pipelines.\n script:\n - |\n- R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA:?}} || exit\n+ R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA:-${CI_MERGE_REQUEST_DIFF_BASE_SHA:?}} || exit\n ./ci/run-style-check.sh \"$R\"\n rules:\n - if: $CI_PIPELINE_SOURCE == 'merge_request_event'\n\nbase-commit: 3b0d05c4a79d0e441283680a864529b02dca5f08\n-- \n2.48.1.157.g3b0d05c4a7\n\n"},{"id":"511816","messageId":"87ldul4rcb.fsf@iotcl.com","threadId":"62879","inReplyTo":"20250131173938.3592899-1-jltobler@gmail.com","subject":"Re: [PATCH] ci: fix base commit fallback for check-whitespace and check-style","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2025-02-04T16:17:24Z","receivedAt":"2025-02-04T16:17:48Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"Justin Tobler writes:\n\n> The check-whitespace and check-style CI scripts require a base commit.\n> In GitLab CI, the base commit can be provided by several different\n> predefined CI variables depending on the type of pipeline being\n> performed.\n>\n> In 30c4f7e350 (check-whitespace: detect if no base_commit is provided,\n> 2024-07-23), the GitLab check-whitespace CI job was modified to support\n> CI_MERGE_REQUEST_DIFF_BASE_SHA as a fallback base commit if\n> CI_MERGE_REQUEST_TARGET_BRANCH_SHA was not provided. The same fallback\n> strategy was also implemented for the GitLab check-style CI job in\n> bce7e52d4e (ci: run style check on GitHub and GitLab, 2024-07-23).\n>\n> The base commit fallback is implemented using shell parameter expansion\n> where, if the first variable is unset, the second variable is used as\n> fallback. In GitLab CI, these variables can be set but null. This has\n> the unintended effect of selecting an empty first variable which results\n> in CI jobs providing an invalid base commit and failing.\n\nI didn't know using this expansion was possible without a colon, but as\nyou pointed out (off-list), it is. From [1]:\n\n Omitting the colon results in a test only for a parameter that is\n unset. Put another way, if the colon is included, the operator tests\n for both parameter’s existence and that its value is not null; if\n the colon is omitted, the operator tests only for existence.\n\n> Fix the issue by defaulting to the fallback variable if the first is\n> unset or null.\n\nYeah, makes sense to do this.\n\n\n> Signed-off-by: Justin Tobler \n> ---\n> Successful GitLab CI check-whitespace and check-style runs:\n> - https://gitlab.com/gitlab-org/git/-/jobs/9011117606\n> - https://gitlab.com/gitlab-org/git/-/jobs/9011117607\n> ---\n> .gitlab-ci.yml | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml\n> index 9254e01583..273a8bad39 100644\n> --- a/.gitlab-ci.yml\n> +++ b/.gitlab-ci.yml\n> @@ -183,7 +183,7 @@ check-whitespace:\n> # be defined in all pipelines.\n> script:\n> - |\n> - R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA:?}} || exit\n> + R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA:-${CI_MERGE_REQUEST_DIFF_BASE_SHA:?}} || exit\n> ./ci/check-whitespace.sh \"$R\"\n> rules:\n> - if: $CI_PIPELINE_SOURCE == 'merge_request_event'\n> @@ -203,7 +203,7 @@ check-style:\n> # be defined in all pipelines.\n> script:\n> - |\n> - R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA:?}} || exit\n> + R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA:-${CI_MERGE_REQUEST_DIFF_BASE_SHA:?}} || exit\n> ./ci/run-style-check.sh \"$R\"\n> rules:\n> - if: $CI_PIPELINE_SOURCE == 'merge_request_event'\n>\n> base-commit: 3b0d05c4a79d0e441283680a864529b02dca5f08\n> -- \n> 2.48.1.157.g3b0d05c4a7\n\nSimple fix, but thank you so much for figuring out and submitting this\npatch. I approve!\n\n[1]: https://www.gnu.org/software/bash/manual/html_node/Shell-Parameter-Expansion.html\n\n--\nToon\n\n"}]}