Re: [PATCH 07/10] t1404: demonstrate two problems with reference transactions

[Michael Haggerty <mhagger@alum.mit.edu>]

On 08/30/2017 07:21 PM, Stefan Beller wrote:
> On Tue, Aug 29, 2017 at 1:20 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> [...]
>> +test_expect_failure 'no bogus intermediate values during delete' '
>> +       prefix=refs/slow-transaction &&
>> +       # Set up a reference with differing loose and packed versions:
>> +       git update-ref $prefix/foo $C &&
>> +       git pack-refs --all &&
>> +       git update-ref $prefix/foo $D &&
>> +       git for-each-ref $prefix >unchanged &&
>> +       # Now try to update the reference, but hold the `packed-refs` lock
>> +       # for a while to see what happens while the process is blocked:
>> +       : >.git/packed-refs.lock &&
>> +       test_when_finished "rm -f .git/packed-refs.lock" &&
>> +       {
>> +               sleep 1 &&
>> +               rm -f .git/packed-refs.lock &
>> +       } &&
>> +       pid1=$! &&
>> +       {
>> +               # Note: the following command is intentionally run in the
>> +               # background. We extend the timeout so that `update-ref`
>> +               # tries to acquire the `packed-refs` lock longer than it
>> +               # takes the background process above to delete it:
>> +               git -c core.packedrefstimeout=2000 update-ref -d $prefix/foo &
>> +       } &&
>> +       pid2=$! &&
>> +       ok=true &&
>> +       while kill -0 $pid2 2>/dev/null
> 
>     If sig is 0, then no signal is sent, but error checking is still
>     performed; this can be used to check for the existence of a
>     process ID or process group ID.
> 
> So the kill -0 is the idiomatic form of "while $pid2 is still alive"?
> ignoring errors due to the dev/null redirection?
> 
> And due to the nature of this test we have to have a busy
> loop, we cannot rate limit the cpu usage inside the loop
> via some shorter sleeps, as ideally we want to observe
> the ref at any time.

Correct on both counts.

I just noticed that there is a stray line `ok=true &&` from an earlier
draft. I'll remove that in v2.

>     In an ideal world this test would instruct the kernel to interrupt
>     the executing program (update-ref) at certain events such as
>     touching/writing/deleting files and in each interrupt we could
>     inspect the file system in a read only fashion.

A tool like `strace` could be used for tests like this, but it would be
terribly non-portable. (But I often use strace manually to check that
the ordering of filesystem events is correct.)

>> +       do
>> +               sha1=$(git rev-parse --verify --quiet $prefix/foo || echo undefined) &&
>> +               case "$sha1" in
>> +               $D)
>> +                       # This is OK; it just means that nothing has happened yet.
>> +                       : ;;
>> +               undefined)
>> +                       # This is OK; it means the deletion was successful.
>> +                       : ;;
>> +               $C)
>> +                       # This value should never be seen. Probably the loose
>> +                       # reference has been deleted but the packed reference
>> +                       # is still there:
>> +                       echo "$prefix/foo incorrectly observed to be C" &&
>> +                       break
>> +                       ;;
>> +               *)
>> +                       # WTF?
>> +                       echo "$prefix/foo unexpected value observed: $sha1" &&
>> +                       break
>> +                       ;;
>> +               esac
>> +       done >out &&
>> +       wait $pid1 &&
>> +       wait $pid2 &&
> 
> oh, you use explicit pids here to check each exit code.
> 
>> If anybody has suggestions for better ways to test these things,
>> please speak up :-)
> 
> I don't think I'd have a satisfactory answer to that, as the timing is inherent
> to the things we test. In other software projects that are less low level, I
> would have suggested to use a time/clock mock, which can be stopped
> and then inspection can be performed at defined states.

I just realized that, given that the main goal here is to check the
value of the reference while `update-ref` is waiting on the
`packed-refs` lock, we can do the test without a busy loop. Instead, we
roughly

	: >.git/packed-refs.lock &&
	{
		git -c core.packedrefstimeout=2000 update-ref -d $prefix/foo &
	} &&
	pid2=$! &&
	sleep 1 &&
	# Verify that update-ref is still running:
	kill -0 $pid2 &&
	# ...verify that the reference still has its old value...
	rm -f .git/packed-refs.lock &&
	wait $pid2 &&
	# ...verify that the reference is now gone...

It's true that this version wouldn't discover incorrect transitional
values of the reference that happen at other times, but that was very
unlikely anyway given the speed disparity between C and shell. I'll make
this change in v2.

Michael