diff mbox series

[v2,8/8] check-whitespace: detect if no base_commit is provided

Message ID 20240711083043.1732288-9-karthik.188@gmail.com (mailing list archive)
State Superseded
Headers show
Series clang-format: add more rules and enable on CI | expand

Commit Message

karthik nayak July 11, 2024, 8:30 a.m. UTC
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(-)

Comments

Justin Tobler July 11, 2024, 2:48 p.m. UTC | #1
On 24/07/11 10:30AM, Karthik Nayak wrote:
> 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(-)
> 
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index dc43fc8ba8..c5a18f655a 100644
> --- a/.gitlab-ci.yml
> +++ b/.gitlab-ci.yml
> @@ -119,7 +119,12 @@ check-whitespace:
>    before_script:
>      - ./ci/install-dependencies.sh
>    script:
> -    - ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
> +    - |
> +      if [ -z ${CI_MERGE_REQUEST_TARGET_BRANCH_SHA} ]; then
> +        ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
> +      else
> +        ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
> +      fi

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.

>    rules:
>      - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
>  
[snip]

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

-Justin
Junio C Hamano July 11, 2024, 4:16 p.m. UTC | #2
Justin Tobler <jltobler@gmail.com> writes:

>> +      if [ -z ${CI_MERGE_REQUEST_TARGET_BRANCH_SHA} ]; then
>> +        ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
>> +      else
>> +        ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
>> +      fi
>
> 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.

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?

>>    rules:
>>      - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
>>  
> [snip]
>
> Overall the GitLab CI changes look good to me. Thanks :)

Thanks for a review.  Very much appreciated.
karthik nayak July 12, 2024, 8:51 a.m. UTC | #3
Junio C Hamano <gitster@pobox.com> writes:

> Justin Tobler <jltobler@gmail.com> writes:
>
>>> +      if [ -z ${CI_MERGE_REQUEST_TARGET_BRANCH_SHA} ]; then
>>> +        ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
>>> +      else
>>> +        ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
>>> +      fi
>>
>> 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.
>
> 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?
>

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

>>>    rules:
>>>      - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
>>>
>> [snip]
>>
>> Overall the GitLab CI changes look good to me. Thanks :)
>
> Thanks for a review.  Very much appreciated.

Thanks both of you!
Junio C Hamano July 12, 2024, 3:12 p.m. UTC | #4
Karthik Nayak <karthik.188@gmail.com> writes:

>> 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 ...
>> shouldn't it?
>>
>
> 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

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.
diff mbox series

Patch

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index dc43fc8ba8..c5a18f655a 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -119,7 +119,12 @@  check-whitespace:
   before_script:
     - ./ci/install-dependencies.sh
   script:
-    - ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
+    - |
+      if [ -z ${CI_MERGE_REQUEST_TARGET_BRANCH_SHA} ]; then
+        ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
+      else
+        ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
+      fi
   rules:
     - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
 
diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh
index db399097a5..c40804394c 100755
--- a/ci/check-whitespace.sh
+++ b/ci/check-whitespace.sh
@@ -9,7 +9,7 @@  baseCommit=$1
 outputFile=$2
 url=$3
 
-if test "$#" -ne 1 && test "$#" -ne 3
+if test "$#" -ne 1 && test "$#" -ne 3 || test -z "$1"
 then
 	echo "USAGE: $0 <BASE_COMMIT> [<OUTPUT_FILE> <URL>]"
 	exit 1
@@ -21,6 +21,12 @@  commitText=
 commitTextmd=
 goodParent=
 
+if ! git rev-parse --quiet --verify "${baseCommit}"
+then
+    echo "Invalid <BASE_COMMIT> '${baseCommit}'"
+    exit 1
+fi
+
 while read dash sha etc
 do
 	case "${dash}" in
@@ -67,7 +73,7 @@  then
 		goodParent=${baseCommit: 0:7}
 	fi
 
-	echo "A whitespace issue was found in onen of more of the commits."
+	echo "A whitespace issue was found in one or more of the commits."
 	echo "Run the following command to resolve whitespace issues:"
 	echo "git rebase --whitespace=fix ${goodParent}"