On Wed, Sep 27, 2017 at 8:48 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>> + 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.
>> + 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.
>> + 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.
>> + 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.
>> + 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.
