| 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 |
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
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.
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!
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 --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}"
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(-)