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