Re: [PATCH 1/1] fast-import: checkpoint: dump branches/tags/marks even if object_count==0

[Junio C Hamano <gitster@pobox.com>]

"Eric Rannaud" <e@nanocritical.com> writes:

> Any comments on the testing strategy with a background fast-import?
>
> To summarize:
> - fast-import is started in the background with a command stream that
>   ends with "checkpoint\nprogress checkpoint\n". fast-import keeps
>   running after reaching the last command (we don't want fast-import to
>   terminate).
> - In the meantime, the test is waiting to read "progress checkpoint" in
>   the output of fast-import, then it checks the testing conditions.
> - Finally, the test ensures that fast-import is still running in the
>   background (and thus that it has just observed the effect of
>   checkpoint, and not the side effects of fast-import terminating).
> - After 10 sec, no matter what, the background fast-import is sent
>   "done" and terminates.
>
> I tried to make sure that the test runs quickly and does not (typically) sleep.
> Upon failure, the test may take up to 10 sec to fully terminate.

Hmmmm, it certainly looks a bit too brittle with many tweakables
like 10, 50, 55, etc. that heavily depend on the load on the system.

Sorry for asking you to go through the hoops.

> diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
> index 67b8c50a5ab4..b410bf3a3a7a 100755
> --- a/t/t9300-fast-import.sh
> +++ b/t/t9300-fast-import.sh
> @@ -3120,4 +3120,121 @@ test_expect_success 'U: validate root delete result' '
>  	compare_diff_raw expect actual
>  '
>  
> +###
> +### series V (checkpoint)
> +###
> +
> +# To make sure you're observing the side effects of checkpoint *before*
> +# fast-import terminates (and thus writes out its state), check that the
> +# fast-import process is still running using background_import_still_running
> +# *after* evaluating the test conditions.
> +background_import_until_checkpoint () {
> +	options=$1
> +	input_file=$2
> +	( cat $input_file; sleep 10; echo done ) | git fast-import $options >V.output &
> +	echo $! >V.pid
> +
> +	# The loop will poll for approximately 5 seconds before giving up.
> +	n=0
> +	while ! test "$(cat V.output)" = "progress checkpoint"; do

Micronit.  Just like you have if/then on different lines, have
while/do on different lines, i.e.

	while test "$(cat V.output)" != "progress checkpoint"
	do

> +		if test $n -gt 55
> +		then
> +...

> +background_import_still_running () {
> +	if ! ps --pid "$(cat V.pid)"

"ps --pid" is probably not portable (I think we'd do "kill -0 $pid"
instead in our existing tests---and it should be kosher according to
POSIX [*1*, *2*]).

> +test_expect_success 'V: checkpoint updates refs after reset' '
> +	cat >input <<-INPUT_END &&

It is not wrong per-se but we quote INPUT_END when there is no
interpolation necessary in the body of here document to help
readers, like so:

	cat >input <<-\INPUT_END &&

> +	reset refs/heads/V
> +	from refs/heads/U
> +
> +	checkpoint
> +	progress checkpoint
> +	INPUT_END

> +test_expect_success 'V: checkpoint updates refs and marks after commit' '
> +	cat >input <<-INPUT_END &&

This we do want interpolation and the above is correct.


[Footnotes]

*1* http://pubs.opengroup.org/onlinepubs/9699919799/utilities/kill.html 
    lists '0' as an allowed signal number to be sent, and defers to the
    description of the kill() function to explain what happens.

*2* http://pubs.opengroup.org/onlinepubs/9699919799/functions/kill.html
    says """If sig is 0 (the null signal), error checking is
    performed but no signal is actually sent. The null signal can be
    used to check the validity of pid."""