Junio C Hamano <gitster@xxxxxxxxx> writes:
> 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?
>
Agreed, that a comment would be nice here. Will add if I reroll!
In this case A ("$CI_MERGE_REQUEST_DIFF_BASE_SHA") is known to be usable
always [1].
[1]: https://docs.gitlab.com/ee/ci/variables/predefined_variables.html
>>> 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.
Thanks both of you!
Attachment:
signature.asc
Description: PGP signature
