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:

> Junio, this last version addresses your last remark regarding the
> potential for the cat $input_file sequence to block when the FIFOs are
> unbuffered.
>
> The concern is mainly theoretical (*if* the shell function is used
> correctly): fast-import will not start writing to its own output until
> it has fully consumed its input (as the only command generating output
> should be the last one). Nevertheless, in this version the write is done
> in the background.

I agree that the concern is theoretical.  Without this fix, we may
not be able to feed the input fully up to the final 'progress
checkpoint' command---because the other side is not reading, we may
get stuck while attempting to write "checkpoint" or "progress
checkpoint", and may never get to read what fast-import says to get
us unstuck.

But if we wanted to solve the theoretical issue (i.e. the command
sequence the user of this helper shell function gives us _might_
trigger an output from fast-import) fully, I do not think
backgrounding the feeding side is sufficient.  The code that reads
fd#9 would need to become a while loop that reads and discards extra
output until we see the "progress checkpoint", at least, perhaps
like the attached patch.

But even with such a fix, we are still at the mercy of the caller of
the helper---the helper will get broken if the input happened to
have a "progress checkpoint" command itself.  There has to be a
"good enough" place to stop.

I think that your patch the last round that feeds fd#8 in the
foreground (i.e. fully trusting that the caller is sensibly giving
input that produces no output) is already a good place to stop.

Your patch this round that feeds fd#8 in the background, plus the
attached patch (i.e. not trusting the caller as much and allowing it
to use commands that outputs something, within reason), would also
be a good place to stop.

But I am not sure your patch this round alone is a good place to
stop.  It somehow feels halfway either way.

 t/t9300-fast-import.sh | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 74ba70874b..d47560b634 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -3159,18 +3159,17 @@ background_import_then_checkpoint () {
 		echo "progress checkpoint"
 	) >&8 &
 
-	error=0
-	if read output <&9
-	then
-		if ! test "$output" = "progress checkpoint"
+	error=1 ;# assume the worst
+	while read output <&9
+	do
+		if test "$output" = "progress checkpoint"
 		then
-			echo >&2 "no progress checkpoint received: $output"
-			error=1
+			error=0
+			break
 		fi
-	else
-		echo >&2 "failed to read fast-import output"
-		error=1
-	fi
+		# otherwise ignore cruft
+		echo >&2 "cruft: $output"
+	done
 
 	if test $error -eq 1
 	then
@@ -3186,6 +3185,17 @@ background_import_still_running () {
 	fi
 }
 
+test_expect_success PIPE 'V: checkpoint helper does not get stuck with extra output' '
+	cat >input <<-INPUT_END &&
+	progress foo
+	progress bar
+
+	INPUT_END
+
+	background_import_then_checkpoint "" input &&
+	background_import_still_running
+'
+
 test_expect_success PIPE 'V: checkpoint updates refs after reset' '
 	cat >input <<-\INPUT_END &&
 	reset refs/heads/V
-- 
2.14.2-820-gd7428e091c