"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
