Re: [PATCH] fast-import: checkpoint: dump branches/tags/marks even if object_count==0
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 29, 2017, 03:51 UTC
- Message-ID
- <xmqqy3oyxiuo.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <0cb786584bd2669763c303f80072baa3693efc33.1506654123.git.e@nanocritical.com>
"Eric Rannaud" <e@nanocritical.com> writes:
Show 9 quoted lines
> 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