diff mbox series

[v5,5/6] check-whitespace: detect if no base_commit is provided

Message ID 20240718081605.452366-6-karthik.188@gmail.com (mailing list archive)
State New, archived
Headers show
Series : add more rules and enable on CI | expand

Commit Message

karthik nayak July 18, 2024, 8:16 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

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .gitlab-ci.yml         | 13 ++++++++++++-
 ci/check-whitespace.sh | 10 ++++++++--
 2 files changed, 20 insertions(+), 3 deletions(-)

Comments

Junio C Hamano July 18, 2024, 3 p.m. UTC | #1
Karthik Nayak <karthik.188@gmail.com> writes:

> +    - |
> +      R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA?}}
> +
> +      if test -z "$R"
> +      then
> +        echo "CI_MERGE_REQUEST_DIFF_BASE_SHA should always exist!"
> +        exit 1
> +      fi

The same comment applies as the previous step.

	R=${A-${B:?}} || exit

should be sufficient.

A demonstration.

        $ unset A; unset B; C=${A-${B:?}} && echo "C=$C"
        bash: B: parameter null or not set
        $ A=a; unset B; C=${A-${B:?}} && echo "C=$C"
        C=a
        $ unset A; B=; C=${A-${B:?}} && echo "C=$C"
        bash: B: parameter null or not set
        $ unset A; B=b; C=${A-${B:?}} && echo "C=$C"
        C=b
        $ A=a; B=b; C=${A-${B:?}} && echo "C=$C"
        C=a

Note that the broken case we do not see C=<value> becaues the
assignment fails with non-zero status.

Thanks.
karthik nayak July 19, 2024, 8:33 a.m. UTC | #2
Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +    - |
>> +      R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA?}}
>> +
>> +      if test -z "$R"
>> +      then
>> +        echo "CI_MERGE_REQUEST_DIFF_BASE_SHA should always exist!"
>> +        exit 1
>> +      fi
>
> The same comment applies as the previous step.
>
> 	R=${A-${B:?}} || exit
>
> should be sufficient.
>
> A demonstration.
>
>         $ unset A; unset B; C=${A-${B:?}} && echo "C=$C"
>         bash: B: parameter null or not set
>         $ A=a; unset B; C=${A-${B:?}} && echo "C=$C"
>         C=a
>         $ unset A; B=; C=${A-${B:?}} && echo "C=$C"
>         bash: B: parameter null or not set
>         $ unset A; B=b; C=${A-${B:?}} && echo "C=$C"
>         C=b
>         $ A=a; B=b; C=${A-${B:?}} && echo "C=$C"
>         C=a
>
> Note that the broken case we do not see C=<value> becaues the
> assignment fails with non-zero status.
>
> Thanks.

Thanks Junio for explaining with examples, really nice of you! I'm on
the fence with this, even the existing change from the previous more
verbose code. I know this is shorter, but it is always more readable to
use the longer version with 'test'. I find it hard to remember the
specifics. But I don't really care which one makes it in the end.

Let me know if you think it is worth a reroll.

Thanks
Junio C Hamano July 19, 2024, 3:03 p.m. UTC | #3
Karthik Nayak <karthik.188@gmail.com> writes:

> Thanks Junio for explaining with examples, really nice of you! I'm on
> the fence with this, even the existing change from the previous more
> verbose code. I know this is shorter, but it is always more readable to
> use the longer version with 'test'. I find it hard to remember the
> specifics.

You'd never remember unless you practice, but it boils down to one
question: is it reasonable to expect that most developers who need
to touch this code find it worth to learn to read and write shell
scripts well in this day and age?  The answer is probably no.

As you may remember, this R=${A-${B?}} dance started at

  https://lore.kernel.org/git/xmqqwmlpb8er.fsf@gitster.g/

where I said:

    ...
    in a separate "after the dust settles" clean-up #leftoverbits topic.

    We could replace the first 7 lines with a single-liner

       R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA?}}

    if we wanted to, but all of that will be mere clean-up changes.

Even the longhand to set a single R with if/elif cascade so that we
can have a single location that invokes ci/run-style-check.sh was
considered extra clean-up for #leftoverbits at least by me.

But after seeing you used the ${A-${B?}} dance, which is more
advanced than the #leftoverbits clean-up, I thought you were
interested in using such a construct that pursues parameter
expansion mastery, and that was the primary reason why the
demonstration in the message you are responding to was added.

I personally do not care too deeply which one to use wrt the
readability, but

	R=${A-${B?}}
	if test -z "$R"
	then
		error
	fi

looks strange and inconsistent by spreading the error check to two
places.  The code would be better off if it were

	R=${A-$B}
	if test -z "$R"
	then
		error
	fi

(or with R=${A:-$B}) instead.  Then it makes it clear that the
author wanted to take care of the error case with the if part.
karthik nayak July 22, 2024, 7:22 a.m. UTC | #4
Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Thanks Junio for explaining with examples, really nice of you! I'm on
>> the fence with this, even the existing change from the previous more
>> verbose code. I know this is shorter, but it is always more readable to
>> use the longer version with 'test'. I find it hard to remember the
>> specifics.
>
> You'd never remember unless you practice, but it boils down to one
> question: is it reasonable to expect that most developers who need
> to touch this code find it worth to learn to read and write shell
> scripts well in this day and age?  The answer is probably no.
>
> As you may remember, this R=${A-${B?}} dance started at
>
>   https://lore.kernel.org/git/xmqqwmlpb8er.fsf@gitster.g/
>
> where I said:
>
>     ...
>     in a separate "after the dust settles" clean-up #leftoverbits topic.
>
>     We could replace the first 7 lines with a single-liner
>
>        R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA?}}
>
>     if we wanted to, but all of that will be mere clean-up changes.
>
> Even the longhand to set a single R with if/elif cascade so that we
> can have a single location that invokes ci/run-style-check.sh was
> considered extra clean-up for #leftoverbits at least by me.
>
> But after seeing you used the ${A-${B?}} dance, which is more
> advanced than the #leftoverbits clean-up, I thought you were
> interested in using such a construct that pursues parameter
> expansion mastery, and that was the primary reason why the
> demonstration in the message you are responding to was added.
>
> I personally do not care too deeply which one to use wrt the
> readability, but
>
> 	R=${A-${B?}}
> 	if test -z "$R"
> 	then
> 		error
> 	fi
>
> looks strange and inconsistent by spreading the error check to two
> places.  The code would be better off if it were
>
> 	R=${A-$B}
> 	if test -z "$R"
> 	then
> 		error
> 	fi
>
> (or with R=${A:-$B}) instead.  Then it makes it clear that the
> author wanted to take care of the error case with the if part.

Yeah fair enough, we are deep into it and might and makes sense to make
it more consistent. I'll incorporate your suggestion and send in a new
version.
diff mbox series

Patch

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 817266226e..320b78b9ae 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -118,8 +118,19 @@  check-whitespace:
   image: ubuntu:latest
   before_script:
     - ./ci/install-dependencies.sh
+  # Since $CI_MERGE_REQUEST_TARGET_BRANCH_SHA is only defined for merged
+  # pipelines, we fallback to $CI_MERGE_REQUEST_DIFF_BASE_SHA, which should
+  # be defined in all pipelines.
   script:
-    - ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
+    - |
+      R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA?}}
+
+      if test -z "$R"
+      then
+        echo "CI_MERGE_REQUEST_DIFF_BASE_SHA should always exist!"
+        exit 1
+      fi
+      ./ci/check-whitespace.sh "$R"
   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}"