Junio C Hamano <gitster@xxxxxxxxx> writes: > Karthik Nayak <karthik.188@xxxxxxxxx> 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@xxxxxxxxx> >> --- >> .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}"
Attachment:
signature.asc
Description: PGP signature
