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

From: Karthik Nayak <karthik.188@gmail.com>
Date: Jul 12, 2024, 08:51 UTC
Message-ID: <CAOLa=ZSR=mMMnYNrpKc9ivOx4vbzJF7c5e3632NpX4XYFdLMmg@mail.gmail.com>
In-Reply-To: <xmqqikxbsy4d.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 31 quoted lines

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

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

Thanks both of you!