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

[Junio C Hamano <gitster@xxxxxxxxx>]

Justin Tobler <jltobler@xxxxxxxxx> 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.