Re: [PATCH 8/8] check-whitespace: detect if no base_commit is provided

[Junio C Hamano <gitster@pobox.com>]

Karthik Nayak <karthik.188@gmail.com> writes:

> The 'check-whitespace' CI script exists gracefully if no base commit is

"exists" -> "exits"

> 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 ||.

>  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?

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