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

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

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

> diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
> index 67b8c50a5ab4..9aa3470d895b 100755
> --- a/t/t9300-fast-import.sh
> +++ b/t/t9300-fast-import.sh
> @@ -3120,4 +3120,133 @@ 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
> +
> +	mkfifo V.input
> +	exec 8<>V.input
> +	rm V.input
> +
> +	mkfifo V.output
> +	exec 9<>V.output
> +	rm V.output
> +
> +	cat $input_file >&8

It probably is a good idea to quote "$input_file" in case other
people later use a full path to the file or something; for now this
is OK.

fd#8 at this point does not have a reader; unless the contents of
the $input_file is small enough, wouldn't this "cat" block until
somebody else comes and reads from it to drain?  Should we instead
start fast-import first in the background, arrange it to be killed
when we are done with it, and then start feeding the input?

> +	git fast-import $options <&8 >&9 &
> +	echo $! >V.pid
> +	test_when_finished "kill $(cat V.pid) || true"

This '|| true' is here because the process might already have died
on its own, which sounds like a sensible precaution.

> +	error=0
> +	if read output <&9
> +	then
> +		if ! test "$output" = "progress checkpoint"
> +		then
> +			echo >&2 "no progress checkpoint received: $output"
> +			error=1
> +		fi
> +	else
> +		echo >&2 "failed to read fast-import output"
> +		error=1
> +	fi

And we expect "progress checkpoint" would be the first and only
output after fast-import consumes all the input stream up to the
"progress" thing we feed, so this is not "read and discard until
we see 'progress checkpoint'" but is "read one and that must be
'progress checkpoint'".  Makes sense to me.

If this script is (and will be in the future) all about issuing a
checkpoint command and observing its effect, we can reasonably
expect that the input file _must_ end with "checkpoint" followed by
"progress checkpoint", no?  If that is the case, perhaps feeding
these two from this helper function to >&8, instead of forcing the
caller to prepare the input file to always end with these two, may
be a better organization.

> +	exec 8>&-
> +	exec 9>&-

These are to make sure that nobody (after fast-import dies) has
these file descriptors hanging open for writing.  Makes one wonder
what happens to the reader side of the file descriptor, though ;-)

Before we return from this function, we expect (as the comment
before the function says) that fast-import is still running, waiting
further input.  Wouldn't closing the other side of the pipe here
like these make it notice that there is no more data by causing
read_next_command() find EOF?  IOW, is "use import_until_checkout,
test the outcome and then make sure import_still_running reports that
the outcome was not due to the process terminating and flushing"
somewhat racy?

Or are we closing these file descriptors for different reason
(i.e. not to tell fast-import we are done feeding it input) and I am
reading the code incorrectly?  Puzzled.

> +	if test $error -eq 1
> +	then
> +		exit 1
> +	fi
> +}
> +
> +background_import_still_running () {
> +	if ! kill -0 "$(cat V.pid)"
> +	then
> +		echo >&2 "background fast-import terminated too early"
> +		exit 1
> +	fi
> +}

I suspect these "exit 1" above should be "false", to give the calling
test_expect_success a chance to notice the failure and react to it.

> +test_expect_success 'V: checkpoint updates refs after reset' '
> +	cat >input <<-\INPUT_END &&
> +	reset refs/heads/V
> +	from refs/heads/U
> +
> +	checkpoint
> +	progress checkpoint
> +	INPUT_END
> +
> +	background_import_until_checkpoint "" input &&
> +	test "$(git rev-parse --verify V)" = "$(git rev-parse --verify U)" &&
> +	background_import_still_running
> +'