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@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" 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 >
