Re: [PATCH 8/8] check-whitespace: detect if no base_commit is provided
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 8, 2024, 17:35 UTC
- Message-ID
- <xmqqbk37hjma.fsf@gitster.g>
- In-Reply-To
- <20240708092317.267915-9-karthik.188@gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
> The 'check-whitespace' CI script exists gracefully if no base commit is
"exists" -> "exits"
Show 37 quoted lines
> 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 ||.
Show 6 quoted lines
> 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?
Show 20 quoted lines
> 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}"