Re: [PATCH v2 08/11] t1404: demonstrate two problems with reference transactions
- Date: Sun, 10 Sep 2017 07:07:27 +0200
- From: Michael Haggerty <mhagger@xxxxxxxxxxxx>
- Subject: Re: [PATCH v2 08/11] t1404: demonstrate two problems with reference transactions
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
> # good
> echo >&2 this is bad
> 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
Yes, using the exit code to decide success is simpler. I'll make that
Thanks for your comments.