Re: [PATCH 8/8] check-whitespace: detect if no base_commit is provided
- From
Karthik Nayak <karthik.188@gmail.com>
- Date
- Jul 11, 2024, 08:27 UTC
- Message-ID
- <CAOLa=ZSUZGqxHbEtpR3fadOFf=YPVxbo8W7SwyB9j5am14EB7A@mail.gmail.com>
- In-Reply-To
- <xmqq8qy9z14t.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
Show 55 quoted lines
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Yeah, makes sense. I think I'll simply add in
>>
>> if ! git rev-parse --quiet --verify "${baseCommit}"
>> then
>> echo "Invalid <BASE_COMMIT> '${baseCommit}'"
>> exit 1
>> fi
>
> It would make the intent a lot clearer. Good.
>
>>>> +if test $? -ne 0
>>>> +then
>>>> + echo -n $gitLogOutput
>>>
>>> What is "-n" doing here? Why are you squashing run of spaces in the
>>> $gitLogOutput variable into a space by not "quoting" inside a dq-pair?
>>>
>>
>> I actually didn't know about this. Thanks for informing.
>>
>>>> + exit 1
>>>> +fi
>>>
>>> Looking for "--check" in
>>>
>>> $ git log --help
>>>
>>> tells me that the command exits with non-zero status if problems are
>>> found, so wouldn't that mean the cases with problems always exit
>>> early, bypassing the while loop with full of bash-ism that comes
>>> after this block?
>>>
>>
>> It should exist with a non-zero code, but since we're capturing it in
>> the while loop, it doesn't stop the slow.
>
> Sorry, but I am confused. The control would not even reach a "while
> loop", I am afraid.
>
> I was commenting on the exit status check done here:
>
> +gitLogOutput=$(git log --check --pretty=format:"---% h% s" "${baseCommit}"..)
> +if test $? -ne 0
> +then
> + echo -n $gitLogOutput
> + exit 1
> +fi
>
> Even though the output is captured in a variable, the exit status of
> "git log --check" is still seen by the shell and "if test $? = 0"
> next line say "ah, the thing exited with non-zero status so lets
> echo the whole thing and exit with 1", before it gets to the while
> loop we have below the above piece of code, no?My bad, I thought you were referring to the code before my changes. Yes, here you're right, we don't need the check since the shell would capture the non-zero status.