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

"Eric Rannaud" [off-list ref] writes:

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?

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

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.

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.

On Wed, Sep 27, 2017 at 8:48 PM, Junio C Hamano [off-list ref] wrote:

quoted

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

Right.

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?

Good point, I will swap the order.

quoted

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

I added a comment.

quoted

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

Agreed. Renamed the function background_import_then_checkpoint to
reflect the change.

quoted

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

Closing 8 and 9 was just housekeeping on my part. But you raise a good
point: what happens then to the stdin of fast-import?

Doesn't fast-import get a copy of 8 (open for both reading and
writing), as a child process, and exec 8>&- only closes the copy of
the file descriptor in the parent shell, so the named pipe remains
open for writing somewhere (in the fast-import process itself, in
fact), therefore fast-import will not find EOF on its stdin?

But in any case, it is sensible to delay the closing of 8 and 9 to
test_when_finished.

quoted

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

True.

Will follow-up with an updated patch.

The checkpoint command cycles packfiles if object_count != 0, a sensible
test or there would be no pack files to write. Since 820b931012, the
command also dumps branches, tags and marks, but still conditionally.
However, it is possible for a command stream to modify refs or create
marks without creating any new objects.

For example, reset a branch (and keep fast-import running):

	$ git fast-import
	reset refs/heads/master
	from refs/heads/master^

	checkpoint

but refs/heads/master remains unchanged.

Other example: a commit command that re-creates an object that already
exists in the object database.

The man page also states that checkpoint "updates the refs" and that
"placing a progress command immediately after a checkpoint will inform
the reader when the checkpoint has been completed and it can safely
access the refs that fast-import updated". This wasn't always true
without this patch.

This fix unconditionally calls dump_{branches,tags,marks}() for all
checkpoint commands. dump_branches() and dump_tags() are cheap to call
in the case of a no-op.

Add tests to t9300 that observe the (non-packfiles) effects of
checkpoint.

Signed-off-by: Eric Rannaud <redacted>
---
 fast-import.c          |   6 +--
 t/t9300-fast-import.sh | 126 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 129 insertions(+), 3 deletions(-)


Updated to include Junio's latest remarks.

Also adding the necessary PIPE prereq, as pointed out by Ramsay Jones.

diff --git a/fast-import.c b/fast-import.c
index 35bf671f12c4..d5e4cf0bad41 100644
--- a/fast-import.c
+++ b/fast-import.c

@@ -3189,10 +3189,10 @@ static void checkpoint(void)
 	checkpoint_requested = 0;
 	if (object_count) {
 		cycle_packfile();
-		dump_branches();
-		dump_tags();
-		dump_marks();
 	}
+	dump_branches();
+	dump_tags();
+	dump_marks();
 }
 
 static void parse_checkpoint(void)

diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 67b8c50a5ab4..b8d394548520 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh

@@ -3120,4 +3120,130 @@ test_expect_success 'U: validate root delete result' '
 	compare_diff_raw expect actual
 '
 
+###
+### series V (checkpoint)
+###
+
+# The commands in input_file should not produce any output on the file
+# descriptor set with --cat-blob-fd (or stdout if unspecified).
+#
+# 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_then_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
+
+	git fast-import $options <&8 >&9 &
+	echo $! >V.pid
+	# We don't mind if fast-import has already died by the time the test
+	# ends.
+	test_when_finished "exec 8>&-; exec 9>&-; kill $(cat V.pid) || true"
+
+	cat "$input_file" >&8
+	echo "checkpoint" >&8
+	echo "progress checkpoint" >&8
+
+	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
+
+	if test $error -eq 1
+	then
+		false
+	fi
+}
+
+background_import_still_running () {
+	if ! kill -0 "$(cat V.pid)"
+	then
+		echo >&2 "background fast-import terminated too early"
+		false
+	fi
+}
+
+test_expect_success PIPE 'V: checkpoint updates refs after reset' '
+	cat >input <<-\INPUT_END &&
+	reset refs/heads/V
+	from refs/heads/U
+
+	INPUT_END
+
+	background_import_then_checkpoint "" input &&
+	test "$(git rev-parse --verify V)" = "$(git rev-parse --verify U)" &&
+	background_import_still_running
+'
+
+test_expect_success PIPE 'V: checkpoint updates refs and marks after commit' '
+	cat >input <<-INPUT_END &&
+	commit refs/heads/V
+	mark :1
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data 0
+	from refs/heads/U
+
+	INPUT_END
+
+	background_import_then_checkpoint "--export-marks=marks.actual" input &&
+
+	echo ":1 $(git rev-parse --verify V)" >marks.expected &&
+
+	test "$(git rev-parse --verify V^)" = "$(git rev-parse --verify U)" &&
+	test_cmp marks.expected marks.actual &&
+	background_import_still_running
+'
+
+# Re-create the exact same commit, but on a different branch: no new object is
+# created in the database, but the refs and marks still need to be updated.
+test_expect_success PIPE 'V: checkpoint updates refs and marks after commit (no new objects)' '
+	cat >input <<-INPUT_END &&
+	commit refs/heads/V2
+	mark :2
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data 0
+	from refs/heads/U
+
+	INPUT_END
+
+	background_import_then_checkpoint "--export-marks=marks.actual" input &&
+
+	echo ":2 $(git rev-parse --verify V2)" >marks.expected &&
+
+	test "$(git rev-parse --verify V2)" = "$(git rev-parse --verify V)" &&
+	test_cmp marks.expected marks.actual &&
+	background_import_still_running
+'
+
+test_expect_success PIPE 'V: checkpoint updates tags after tag' '
+	cat >input <<-INPUT_END &&
+	tag Vtag
+	from refs/heads/V
+	tagger $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data 0
+
+	INPUT_END
+
+	background_import_then_checkpoint "" input &&
+	git show-ref -d Vtag &&
+	background_import_still_running
+'
+
 test_done

-- 
2.14.1

On Wed, Sep 27, 2017 at 10:07:41PM -0700, Eric Rannaud wrote:

The checkpoint command cycles packfiles if object_count != 0, a sensible
test or there would be no pack files to write. Since 820b931012, the
command also dumps branches, tags and marks, but still conditionally.
However, it is possible for a command stream to modify refs or create
marks without creating any new objects.

For example, reset a branch (and keep fast-import running):

	$ git fast-import
	reset refs/heads/master
	from refs/heads/master^

	checkpoint

but refs/heads/master remains unchanged.

Other example: a commit command that re-creates an object that already
exists in the object database.

The man page also states that checkpoint "updates the refs" and that
"placing a progress command immediately after a checkpoint will inform
the reader when the checkpoint has been completed and it can safely
access the refs that fast-import updated". This wasn't always true
without this patch.

This fix unconditionally calls dump_{branches,tags,marks}() for all
checkpoint commands. dump_branches() and dump_tags() are cheap to call
in the case of a no-op.

Add tests to t9300 that observe the (non-packfiles) effects of
checkpoint.

Signed-off-by: Eric Rannaud <redacted>
---
 fast-import.c          |   6 +--
 t/t9300-fast-import.sh | 126 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 129 insertions(+), 3 deletions(-)


Updated to include Junio's latest remarks.

Also adding the necessary PIPE prereq, as pointed out by Ramsay Jones.

Cygwin doesn't have the PIPE prereq; I've just confirmed that the
previous version of this patch has t9300 failing on Cygwin, but this
version passes.

On Thu, Sep 28, 2017 at 5:59 AM, Adam Dinwoodie [off-list ref] wrote:

On Wed, Sep 27, 2017 at 10:07:41PM -0700, Eric Rannaud wrote:

quoted

Also adding the necessary PIPE prereq, as pointed out by Ramsay Jones.

Cygwin doesn't have the PIPE prereq; I've just confirmed that the
previous version of this patch has t9300 failing on Cygwin, but this
version passes.

What's the preferred solution here? I can avoid using named pipes entirely:

	read_checkpoint () {
		if read output
		then
			if ! test "$output" = "progress checkpoint"
			then
				echo >&2 "no progress checkpoint received: $output"
				echo 1 > V.result
			else
				echo 0 > V.result
			fi
		else
			echo >&2 "failed to read fast-import output"
			echo 1 > V.result
		fi
	}
	
	# The commands in input_file should not produce any output on the file
	# descriptor set with --cat-blob-fd (or stdout if unspecified).
	#
	# 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_then_checkpoint () {
		options=$1
		input_file=$2
	
		rm -f V.result
	
		( cat "$input_file"
		echo "checkpoint"
		echo "progress checkpoint"
		sleep 3600 &
		echo $! >V.pid
		wait ) | git fast-import $options | read_checkpoint &
	
		# We don't mind if the pipeline has already died by the time the test
		# ends.
		test_when_finished "kill $(cat V.pid) || true"
	
		while ! test -f V.result
		do
			# Try to sleep less than a second, if supported.
			sleep .1 2>/dev/null || sleep 1
		done
		return $(cat V.result)
	}

Do we like this better?