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

From: Karthik Nayak <karthik.188@gmail.com>
Date: Jul 10, 2024, 14:12 UTC
Message-ID: <CAOLa=ZRHq4WarzBiNu9gxWxT=Fe8D0TZ00pLngXuqFUZf1j==w@mail.gmail.com>
In-Reply-To: <tl66b35ytbri4dep3cnppdexebblep6avyuhj4moc4mnsolnp2@xpqqwyl4oo2y>

Justin Tobler <jltobler@gmail.com> writes:

Show 15 quoted lines

> On 24/07/08 11:23AM, Karthik Nayak wrote:
>> The 'check-whitespace' CI script exists gracefully if no base commit is
>> provided or if an invalid revision is provided. This is not good because
>> if a particular CI provides an incorrect base_commit, it would fail
>> successfully.
>
> s/exists/exits
>
> If no base commit is provided, we already fail. Here is an example
> GitLab CI job demonstrating this:
> https://gitlab.com/gitlab-org/git/-/jobs/7289543498#L2370
>
> If the base commit does not exist though, it currently prints that error occured
> but still exits with 0. Makes sense to update and fail the job accordingly.
>

This example is running on the 'edb990d9', whose parent's '8d9bcf0a' parent '57fdb00c' contains my changes, specifically the `|| test -z "$1"` section. You can check this on master locally though.

    $ ./ci/check-whitespace.sh ""
    fatal: ..: '..' is outside repository at '/home/karthik/git'

    $ echo $?
    0

or for invalid value:

    $ ./ci/check-whitespace.sh "random"
    fatal: ambiguous argument 'random..': unknown revision or path not
in the working tree.
    Use '--' to separate paths from revisions, like this:
    'git <command> [<revision>...] -- [<file>...]'

    $ echo $status
    0

[snip]

>
> I might be misunderstanding, but this additional check seems redundant to me.
>

It is required, as commented above

Show 34 quoted lines

>>  then
>>  	echo "USAGE: $0 <BASE_COMMIT> [<OUTPUT_FILE> <URL>]"
>>  	exit 1
>>  fi
>>
>> +gitLogOutput=$(git log --check --pretty=format:"---% h% s" "${baseCommit}"..)
>> +if test $? -ne 0
>> +then
>> +	echo -n $gitLogOutput
>> +	exit 1
>> +fi
>> +
>>  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."
> There is another preexisting typo:
>
> s/one of/one or/
>

Thanks. will add