On 09/09/2017 01:17 PM, Jeff King wrote: > On Fri, Sep 08, 2017 at 03:51:50PM +0200, Michael Haggerty wrote: > [...] > So if we get what we want, we execute ":" which should be a successful > exit code. I think the `:` is superfluous even if we care about the exit code of the `case`. I'll remove it. >> + undefined) >> + # This is not correct; it means the deletion has happened >> + # already even though update-ref should not have been >> + # able to acquire the lock yet. >> + echo "$prefix/foo deleted prematurely" && >> + break >> + ;; > > But if we don't, we hit a "break". But we're not in a loop, so the break > does nothing. Is the intent to give a false value to the switch so that > we fail the &&-chain? If so, I'd think "false" would be the right thing > to use. It's more to the point, and from a few limited tests, it looks > like "break" will return "0" even outside a loop (bash writes a > complaint to stderr, but dash doesn't). > > Or did you just forget that you're not writing C and that ";;" is the > correct way to spell "break" here? :) An earlier version of the patch used a loop and needed the `break`. But when I removed the loop, I probably didn't notice the now-unneeded breaks because of what you said. I'll take them out. >> [...] >> + esac >out && >> [...] >> + test_must_be_empty out && > > The return value of "break" _doesn't_ matter, because you end up using > the presence of the error message. > > I think we could write this as just: > > case "$sha1" in > $D) > # good > ;; > undefined) > echo >&2 this is bad > false > ;; > esac && > > I'm OK with it either way (testing the exit code or testing the output), > but either way the "break" calls are doing nothing and can be dropped, I > think. Yes, using the exit code to decide success is simpler. I'll make that change, too. Thanks for your comments. Michael
