From: Karthik Nayak Date: Thu, 11 Jul 2024 08:27:41 GMT Subject: Re: [PATCH 8/8] check-whitespace: detect if no base_commit is provided Message-ID: In-Reply-To: Junio C Hamano writes: > Karthik Nayak writes: > >> Yeah, makes sense. I think I'll simply add in >> >> if ! git rev-parse --quiet --verify "${baseCommit}" >> then >> echo "Invalid '${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.