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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 11, 2024, 16:16 UTC
Message-ID: <xmqqikxbsy4d.fsf@gitster.g>
In-Reply-To: <zq2x3k5gshs5jgx6zglfdzu4kx6vrrzugfvd4w4pjfh6uyjc4r@4x2zt2ftfmsq>

Justin Tobler <jltobler@gmail.com> writes:

Show 9 quoted lines

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

Show 6 quoted lines

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