| Message ID | 20240708092317.267915-9-karthik.188@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | clang-format: add more rules and enable on CI | expand |
On Mon, Jul 8, 2024 at 2:24 AM Karthik Nayak <karthik.188@gmail.com> wrote: > diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh > index db399097a5..ab023f9519 100755 > --- a/ci/check-whitespace.sh > +++ b/ci/check-whitespace.sh > @@ -9,12 +9,19 @@ baseCommit=$1 > outputFile=$2 > url=$3 > > -if test "$#" -ne 1 && test "$#" -ne 3 > +if { test "$#" -ne 1 && test "$#" -ne 3; } || test -z "$1" > then The braces `{ ... }` here are unnecessary as `&&`will bind first (sh and bash give both operators the same precedence but then use left associativity). Dropping them drops the need for the semicolon. Of course some might prefer this to be explicit. The `test` command has AND and OR operators of its own, which give `-a` (AND) higher precedence than `-o` (OR). In addition, `$#` can only expand to an integer value, so quotes are not required, and the whole thing can be written as: if test $# -ne 1 -a $# -ne 3 -o -z "$1" (which is what I would do myself, unless I wanted a separate error message for an empty "$1"). It's fine as is though. Chris
On 7/8/24 12:18 PM, Chris Torek wrote: > The `test` command has AND and OR operators of its own, > which give `-a` (AND) higher precedence than `-o` (OR). In > addition, `$#` can only expand to an integer value, so quotes > are not required, and the whole thing can be written as: > > if test $# -ne 1 -a $# -ne 3 -o -z "$1" > > (which is what I would do myself, unless I wanted a separate > error message for an empty "$1"). It's fine as is though. Hi, https://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html states "The XSI extensions specifying the -a and -o binary primaries and the '(' and ')' operators have been marked obsolescent." suggesting "&&" being preferred over "-a". > Chris >
On Mon, Jul 8, 2024 at 3:35 AM Georg Pfuetzenreuter <georg@syscid.com> wrote: > https://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html states > > "The XSI extensions specifying the -a and -o binary primaries and the > '(' and ')' operators have been marked obsolescent." > > suggesting "&&" being preferred over "-a". That's annoying, I wonder why they did that. It does make the "test" parser a bit tricky, especially with empty expansions, but empty expansions are already a problem requiring careful quoting in the first place... Chris
Chris Torek <chris.torek@gmail.com> writes: > On Mon, Jul 8, 2024 at 3:35 AM Georg Pfuetzenreuter <georg@syscid.com> wrote: >> https://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html states >> >> "The XSI extensions specifying the -a and -o binary primaries and the >> '(' and ')' operators have been marked obsolescent." >> >> suggesting "&&" being preferred over "-a". > > That's annoying, I wonder why they did that. It does make > the "test" parser a bit tricky, especially with empty expansions, > but empty expansions are already a problem requiring careful > quoting in the first place... > > Chris Thanks both, I think with this, it makes sense to keep it as is.
Chris Torek <chris.torek@gmail.com> writes: > On Mon, Jul 8, 2024 at 3:35 AM Georg Pfuetzenreuter <georg@syscid.com> wrote: >> https://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html states >> >> "The XSI extensions specifying the -a and -o binary primaries and the >> '(' and ')' operators have been marked obsolescent." >> >> suggesting "&&" being preferred over "-a". > > That's annoying, I wonder why they did that. Consult Documentation/CodingGuidelines? - We do not write our "test" command with "-a" and "-o" and use "&&" or "||" to concatenate multiple "test" commands instead, because the use of "-a/-o" is often error-prone. E.g. test -n "$x" -a "$a" = "$b" is buggy and breaks when $x is "=", but test -n "$x" && test "$a" = "$b" does not have such a problem.
Karthik Nayak <karthik.188@gmail.com> writes: > The 'check-whitespace' CI script exists gracefully if no base commit is "exists" -> "exits" > provided or if an invalid revision is provided... > ... > Let's fix the variable used in the GitLab CI. 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 | 2 +- > ci/check-whitespace.sh | 13 ++++++++++--- > 2 files changed, 11 insertions(+), 4 deletions(-) > > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml > index 65fd261e5e..36199893d8 100644 > --- a/.gitlab-ci.yml > +++ b/.gitlab-ci.yml > @@ -119,7 +119,7 @@ check-whitespace: > before_script: > - ./ci/install-dependencies.sh > script: > - - ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA" > + - ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA" > rules: > - if: $CI_PIPELINE_SOURCE == 'merge_request_event' > > diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh > index db399097a5..ab023f9519 100755 > --- a/ci/check-whitespace.sh > +++ b/ci/check-whitespace.sh > @@ -9,12 +9,19 @@ baseCommit=$1 > outputFile=$2 > url=$3 > > -if test "$#" -ne 1 && test "$#" -ne 3 > +if { test "$#" -ne 1 && test "$#" -ne 3; } || test -z "$1" You can just add || test -z "$1" after the existing one, making the thing A && B || C which evaulates left to right with the same precedence for && and ||. > then > echo "USAGE: $0 <BASE_COMMIT> [<OUTPUT_FILE> <URL>]" > exit 1 > fi > > +gitLogOutput=$(git log --check --pretty=format:"---% h% s" "${baseCommit}"..) That is a large string to hold in a variable for a potentially large series with lots of breakages. I didn't quite read the reasoning behind this change in the proposed log message. Under what condition do you expect the command to exit with non-zero status? $basecommit being a non-empty string but does not name a valid commit object or something, in which case shouldn't "git log --oneline $baseCommit.." or something simpler should suffice? > +if test $? -ne 0 > +then > + echo -n $gitLogOutput What is "-n" doing here? Why are you squashing run of spaces in the $gitLogOutput variable into a space by not "quoting" inside a dq-pair? > + exit 1 > +fi Looking for "--check" in $ git log --help tells me that the command exits with non-zero status if problems are found, so wouldn't that mean the cases with problems always exit early, bypassing the while loop with full of bash-ism that comes after this block? > problems=() > commit= > commitText= > @@ -58,7 +65,7 @@ do > echo "${dash} ${sha} ${etc}" > ;; > esac > -done <<< "$(git log --check --pretty=format:"---% h% s" "${baseCommit}"..)" > +done <<< "$gitLogOutput" > > if test ${#problems[*]} -gt 0 > then > @@ -67,7 +74,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 of more of the commits." > echo "Run the following command to resolve whitespace issues:" > echo "git rebase --whitespace=fix ${goodParent}"
On 24/07/08 11:23AM, Karthik Nayak wrote: > The 'check-whitespace' CI script exists 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. s/exists/exits If no base commit is provided, we already fail. Here is an example GitLab CI job demonstrating this: https://gitlab.com/gitlab-org/git/-/jobs/7289543498#L2370 If the base commit does not exist though, it currently prints that error occured but still exits with 0. Makes sense to update and fail the job accordingly. > > 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. The CI for this project is configured to use merged pipelines. So $CI_MERGE_REQUEST_TARGET_BRANCH_SHA is defined. The downside with using $CI_MERGE_REQUEST_DIFF_BASE_SHA is that it will include other commits in the check that are not part of the MR, but are included in the merge for merge pipelines. With this change, the job can now fail due to unrelated changes. If we feel inclined to also support regular pipelines, one option would be to simply fallback to $CI_MERGE_REQUEST_DIFF_BASE_SHA if a merge pipeline is not in use. GitLab CI pipeline showing $CI_MERGE_REQUEST_TARGET_BRANCH_SHA defined: https://gitlab.com/gitlab-org/git/-/jobs/7289331488#L2371 > > Let's fix the variable used in the GitLab CI. 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 | 2 +- > ci/check-whitespace.sh | 13 ++++++++++--- > 2 files changed, 11 insertions(+), 4 deletions(-) > > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml > index 65fd261e5e..36199893d8 100644 > --- a/.gitlab-ci.yml > +++ b/.gitlab-ci.yml > @@ -119,7 +119,7 @@ check-whitespace: > before_script: > - ./ci/install-dependencies.sh > script: > - - ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA" > + - ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA" > rules: > - if: $CI_PIPELINE_SOURCE == 'merge_request_event' > > diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh > index db399097a5..ab023f9519 100755 > --- a/ci/check-whitespace.sh > +++ b/ci/check-whitespace.sh > @@ -9,12 +9,19 @@ baseCommit=$1 > outputFile=$2 > url=$3 > > -if test "$#" -ne 1 && test "$#" -ne 3 > +if { test "$#" -ne 1 && test "$#" -ne 3; } || test -z "$1" I might be misunderstanding, but this additional check seems redundant to me. > then > echo "USAGE: $0 <BASE_COMMIT> [<OUTPUT_FILE> <URL>]" > exit 1 > fi > > +gitLogOutput=$(git log --check --pretty=format:"---% h% s" "${baseCommit}"..) > +if test $? -ne 0 > +then > + echo -n $gitLogOutput > + exit 1 > +fi > + > problems=() > commit= > commitText= > @@ -58,7 +65,7 @@ do > echo "${dash} ${sha} ${etc}" > ;; > esac > -done <<< "$(git log --check --pretty=format:"---% h% s" "${baseCommit}"..)" > +done <<< "$gitLogOutput" > > if test ${#problems[*]} -gt 0 > then > @@ -67,7 +74,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 of more of the commits." There is another preexisting typo: s/one of/one or/ > echo "Run the following command to resolve whitespace issues:" > echo "git rebase --whitespace=fix ${goodParent}" > > -- > 2.45.1 >
Junio C Hamano <gitster@pobox.com> writes: > Karthik Nayak <karthik.188@gmail.com> writes: > >> The 'check-whitespace' CI script exists gracefully if no base commit is > > "exists" -> "exits" > Will fix. >> provided or if an invalid revision is provided... >> ... >> Let's fix the variable used in the GitLab CI. 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 | 2 +- >> ci/check-whitespace.sh | 13 ++++++++++--- >> 2 files changed, 11 insertions(+), 4 deletions(-) >> >> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml >> index 65fd261e5e..36199893d8 100644 >> --- a/.gitlab-ci.yml >> +++ b/.gitlab-ci.yml >> @@ -119,7 +119,7 @@ check-whitespace: >> before_script: >> - ./ci/install-dependencies.sh >> script: >> - - ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA" >> + - ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA" >> rules: >> - if: $CI_PIPELINE_SOURCE == 'merge_request_event' >> >> diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh >> index db399097a5..ab023f9519 100755 >> --- a/ci/check-whitespace.sh >> +++ b/ci/check-whitespace.sh >> @@ -9,12 +9,19 @@ baseCommit=$1 >> outputFile=$2 >> url=$3 >> >> -if test "$#" -ne 1 && test "$#" -ne 3 >> +if { test "$#" -ne 1 && test "$#" -ne 3; } || test -z "$1" > > You can just add || test -z "$1" after the existing one, making the > thing A && B || C which evaulates left to right with the same > precedence for && and ||. > Well, I prefer making it explicit so one does not have to remember the precedence ordering, but it could just be my lack of shell knowledge. I'm okay with this change, I'll add it in the next version. >> then >> echo "USAGE: $0 <BASE_COMMIT> [<OUTPUT_FILE> <URL>]" >> exit 1 >> fi >> >> +gitLogOutput=$(git log --check --pretty=format:"---% h% s" "${baseCommit}"..) > > That is a large string to hold in a variable for a potentially large > series with lots of breakages. I didn't quite read the reasoning > behind this change in the proposed log message. Under what > condition do you expect the command to exit with non-zero status? > $basecommit being a non-empty string but does not name a valid > commit object or something, in which case shouldn't "git log > --oneline $baseCommit.." or something simpler should suffice? > Yeah, makes sense. I think I'll simply add in if ! git rev-parse --quiet --verify "${baseCommit}" then echo "Invalid <BASE_COMMIT> '${baseCommit}'" exit 1 fi instead >> +if test $? -ne 0 >> +then >> + echo -n $gitLogOutput > > What is "-n" doing here? Why are you squashing run of spaces in the > $gitLogOutput variable into a space by not "quoting" inside a dq-pair? > I actually didn't know about this. Thanks for informing. >> + exit 1 >> +fi > > Looking for "--check" in > > $ git log --help > > tells me that the command exits with non-zero status if problems are > found, so wouldn't that mean the cases with problems always exit > early, bypassing the while loop with full of bash-ism that comes > after this block? > It should exist with a non-zero code, but since we're capturing it in the while loop, it doesn't stop the slow. A consequence of which is that it'll print the stderr from the `git log` failing, but the script itself will still exit with a zero exit code. This marks a success on the CI. >> problems=() >> commit= >> commitText= >> @@ -58,7 +65,7 @@ do >> echo "${dash} ${sha} ${etc}" >> ;; >> esac >> -done <<< "$(git log --check --pretty=format:"---% h% s" "${baseCommit}"..)" >> +done <<< "$gitLogOutput" >> >> if test ${#problems[*]} -gt 0 >> then >> @@ -67,7 +74,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 of more of the commits." >> echo "Run the following command to resolve whitespace issues:" >> echo "git rebase --whitespace=fix ${goodParent}"
Justin Tobler <jltobler@gmail.com> writes: > On 24/07/08 11:23AM, Karthik Nayak wrote: >> The 'check-whitespace' CI script exists 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. > > s/exists/exits > > If no base commit is provided, we already fail. Here is an example > GitLab CI job demonstrating this: > https://gitlab.com/gitlab-org/git/-/jobs/7289543498#L2370 > > If the base commit does not exist though, it currently prints that error occured > but still exits with 0. Makes sense to update and fail the job accordingly. > This example is running on the 'edb990d9', whose parent's '8d9bcf0a' parent '57fdb00c' contains my changes, specifically the `|| test -z "$1"` section. You can check this on master locally though. $ ./ci/check-whitespace.sh "" fatal: ..: '..' is outside repository at '/home/karthik/git' $ echo $? 0 or for invalid value: $ ./ci/check-whitespace.sh "random" fatal: ambiguous argument 'random..': unknown revision or path not in the working tree. Use '--' to separate paths from revisions, like this: 'git <command> [<revision>...] -- [<file>...]' $ echo $status 0 [snip] > > I might be misunderstanding, but this additional check seems redundant to me. > It is required, as commented above >> then >> echo "USAGE: $0 <BASE_COMMIT> [<OUTPUT_FILE> <URL>]" >> exit 1 >> fi >> >> +gitLogOutput=$(git log --check --pretty=format:"---% h% s" "${baseCommit}"..) >> +if test $? -ne 0 >> +then >> + echo -n $gitLogOutput >> + exit 1 >> +fi >> + >> problems=() >> commit= >> commitText= >> @@ -58,7 +65,7 @@ do >> echo "${dash} ${sha} ${etc}" >> ;; >> esac >> -done <<< "$(git log --check --pretty=format:"---% h% s" "${baseCommit}"..)" >> +done <<< "$gitLogOutput" >> >> if test ${#problems[*]} -gt 0 >> then >> @@ -67,7 +74,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 of more of the commits." > There is another preexisting typo: > > s/one of/one or/ > Thanks. will add
Karthik Nayak <karthik.188@gmail.com> writes: > Yeah, makes sense. I think I'll simply add in > > if ! git rev-parse --quiet --verify "${baseCommit}" > then > echo "Invalid <BASE_COMMIT> '${baseCommit}'" > exit 1 > fi It would make the intent a lot clearer. Good. >>> +if test $? -ne 0 >>> +then >>> + echo -n $gitLogOutput >> >> What is "-n" doing here? Why are you squashing run of spaces in the >> $gitLogOutput variable into a space by not "quoting" inside a dq-pair? >> > > I actually didn't know about this. Thanks for informing. > >>> + exit 1 >>> +fi >> >> Looking for "--check" in >> >> $ git log --help >> >> tells me that the command exits with non-zero status if problems are >> found, so wouldn't that mean the cases with problems always exit >> early, bypassing the while loop with full of bash-ism that comes >> after this block? >> > > It should exist with a non-zero code, but since we're capturing it in > the while loop, it doesn't stop the slow. Sorry, but I am confused. The control would not even reach a "while loop", I am afraid. I was commenting on the exit status check done here: +gitLogOutput=$(git log --check --pretty=format:"---% h% s" "${baseCommit}"..) +if test $? -ne 0 +then + echo -n $gitLogOutput + exit 1 +fi Even though the output is captured in a variable, the exit status of "git log --check" is still seen by the shell and "if test $? = 0" next line say "ah, the thing exited with non-zero status so lets echo the whole thing and exit with 1", before it gets to the while loop we have below the above piece of code, no?
Junio C Hamano <gitster@pobox.com> writes: > Karthik Nayak <karthik.188@gmail.com> writes: > >> Yeah, makes sense. I think I'll simply add in >> >> if ! git rev-parse --quiet --verify "${baseCommit}" >> then >> echo "Invalid <BASE_COMMIT> '${baseCommit}'" >> exit 1 >> fi > > It would make the intent a lot clearer. Good. > >>>> +if test $? -ne 0 >>>> +then >>>> + echo -n $gitLogOutput >>> >>> What is "-n" doing here? Why are you squashing run of spaces in the >>> $gitLogOutput variable into a space by not "quoting" inside a dq-pair? >>> >> >> I actually didn't know about this. Thanks for informing. >> >>>> + exit 1 >>>> +fi >>> >>> Looking for "--check" in >>> >>> $ git log --help >>> >>> tells me that the command exits with non-zero status if problems are >>> found, so wouldn't that mean the cases with problems always exit >>> early, bypassing the while loop with full of bash-ism that comes >>> after this block? >>> >> >> It should exist with a non-zero code, but since we're capturing it in >> the while loop, it doesn't stop the slow. > > Sorry, but I am confused. The control would not even reach a "while > loop", I am afraid. > > I was commenting on the exit status check done here: > > +gitLogOutput=$(git log --check --pretty=format:"---% h% s" "${baseCommit}"..) > +if test $? -ne 0 > +then > + echo -n $gitLogOutput > + exit 1 > +fi > > Even though the output is captured in a variable, the exit status of > "git log --check" is still seen by the shell and "if test $? = 0" > next line say "ah, the thing exited with non-zero status so lets > echo the whole thing and exit with 1", before it gets to the while > loop we have below the above piece of code, no? My bad, I thought you were referring to the code before my changes. Yes, here you're right, we don't need the check since the shell would capture the non-zero status.
Karthik Nayak <karthik.188@gmail.com> writes: >> I was commenting on the exit status check done here: >> >> +gitLogOutput=$(git log --check --pretty=format:"---% h% s" "${baseCommit}"..) >> +if test $? -ne 0 >> +then >> + echo -n $gitLogOutput >> + exit 1 >> +fi >> >> Even though the output is captured in a variable, the exit status of >> "git log --check" is still seen by the shell and "if test $? = 0" >> next line say "ah, the thing exited with non-zero status so lets >> echo the whole thing and exit with 1", before it gets to the while >> loop we have below the above piece of code, no? > > My bad, I thought you were referring to the code before my changes. Yes, > here you're right, we don't need the check since the shell would capture > the non-zero status. OK. Because in the next round, you'd be checking the error condition of "git rev-parse $baseCommit" or something that is specifically designed to check the validity of user input, and not the error result from the actual "log --check", the above becomes moot ;-) Thanks.
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 65fd261e5e..36199893d8 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -119,7 +119,7 @@ check-whitespace: before_script: - ./ci/install-dependencies.sh script: - - ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA" + - ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA" rules: - if: $CI_PIPELINE_SOURCE == 'merge_request_event' diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh index db399097a5..ab023f9519 100755 --- a/ci/check-whitespace.sh +++ b/ci/check-whitespace.sh @@ -9,12 +9,19 @@ 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 fi +gitLogOutput=$(git log --check --pretty=format:"---% h% s" "${baseCommit}"..) +if test $? -ne 0 +then + echo -n $gitLogOutput + exit 1 +fi + problems=() commit= commitText= @@ -58,7 +65,7 @@ do echo "${dash} ${sha} ${etc}" ;; esac -done <<< "$(git log --check --pretty=format:"---% h% s" "${baseCommit}"..)" +done <<< "$gitLogOutput" if test ${#problems[*]} -gt 0 then @@ -67,7 +74,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 of more of the commits." echo "Run the following command to resolve whitespace issues:" echo "git rebase --whitespace=fix ${goodParent}"
The 'check-whitespace' CI script exists 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 fix the variable used in the GitLab CI. 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 | 2 +- ci/check-whitespace.sh | 13 ++++++++++--- 2 files changed, 11 insertions(+), 4 deletions(-)