Re: [PATCH v2 08/11] t1404: demonstrate two problems with reference transactions

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Sep 10, 2017, 05:07 UTC
Message-ID: <cdfea8a5-fc86-095d-7f5f-89a8f922cac9@alum.mit.edu>
In-Reply-To: <20170909111753.pidf26f5koaewyho@sigill.intra.peff.net>

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.

Show 17 quoted lines

>> +	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.

Show 23 quoted lines

>> [...]
>> +	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